Skip to content

Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait is not implemented#156417

Merged
rust-bors[bot] merged 2 commits into
rust-lang:mainfrom
jakubadamw:issue-137190-reapplied
Jun 4, 2026
Merged

Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait is not implemented#156417
rust-bors[bot] merged 2 commits into
rust-lang:mainfrom
jakubadamw:issue-137190-reapplied

Conversation

@jakubadamw
Copy link
Copy Markdown
Contributor

@jakubadamw jakubadamw commented May 10, 2026 •
edited
Loading

This is a second incarnation of #152287, which was reverted in #152738 as it had exposed another underlying unsoundness (#153596, exhibited indirectly in #152735), which was recently fixed in #155749.

It’s the same fix and the same set of tests. Regression tests for the unsoundness itself were already added in #155749.

Closes #137190.
Closes #135470.

jakubadamw added 2 commits May 10, 2026 22:04
@jakubadamw
Fix an ICE in the vtable iteration for a trait reference in const eva…
786777f 
…l when a supertrait not implemented

compiler/rustc_trait_selection/src/traits/vtable.rs@`vtable_entries`:
The impossible predicates check in `vtable_entries` used
`instantiate_own` which only includes the method's own where-clauses,
not the parent trait's bounds. Replace it with
`instantiate_and_check_impossible_predicates` which also checks the
trait ref itself, so unsatisfied supertrait bounds are caught
and the method is marked `Vacant` instead of ICEing.
@jakubadamw
Move tests from tests/crashes to tests/ui now that they don't ICE
3541645
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 10, 2026

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 10, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 10, 2026

r? @adwinwhite

rustbot has assigned @adwinwhite.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 18 candidates

@jakubadamw jakubadamw mentioned this pull request May 10, 2026
Closed
@adwinwhite
Copy link
Copy Markdown
Contributor

adwinwhite commented May 11, 2026

This should be sound now according to comment.
But I'm not familiar with the cause of the previous revert.

So r? @lcnr or @ShoyuVanilla ?

@rustbot rustbot assigned lcnr and unassigned adwinwhite May 11, 2026
@lcnr
Copy link
Copy Markdown
Contributor

lcnr commented Jun 4, 2026

@bors r+ rollup

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jun 4, 2026

📌 Commit 3541645 has been approved by lcnr

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2026
@JonathanBrouwer JonathanBrouwer mentioned this pull request Jun 4, 2026
Merged
rust-bors Bot pushed a commit that referenced this pull request Jun 4, 2026
@bors
Auto merge of #157440 - JonathanBrouwer:rollup-XlcBgjV, r=JonathanBro…
688ab44 
…uwer

Rollup of 8 pull requests

Successful merges:

 - #156281 (Emit nofree attribute)
 - #157305 (Eagerly decide whether relaxed bounds are allowed or not)
 - #148713 (rustc_borrowck: fix async closure error note to report AsyncFn rather than Fn)
 - #156266 (Don't ICE in has_self_borrows when coroutine captures-by-ref ty is still inferred)
 - #156417 (Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait is not implemented)
 - #156956 (Support generic params in `Lift_Generic`)
 - #157140 ( rustc_target: Use rustc_abi instead of cfg_abi to detect powerpcspe )
 - #157423 (Refactor/expand rustc_attr_parsing docs)
@rust-bors rust-bors Bot merged commit 730bc39 into rust-lang:main Jun 4, 2026
11 checks passed
@rustbot rustbot added this to the 1.98.0 milestone Jun 4, 2026
@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Jun 5, 2026

@rust-timer build 0465a9f

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Copy Markdown
Collaborator

rust-timer commented Jun 5, 2026

Finished benchmarking commit (0465a9f): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.8% [0.3%, 1.2%] 19
Improvements ✅
(primary)		 -0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)		 -0.2% [-0.6%, -0.1%] 6
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 1

Max RSS (memory usage)

Results (primary -0.7%, secondary -1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 3.9% [1.1%, 6.4%] 3
Regressions ❌
(secondary)		 3.8% [1.6%, 5.9%] 2
Improvements ✅
(primary)		 -5.3% [-6.3%, -3.9%] 3
Improvements ✅
(secondary)		 -4.8% [-7.3%, -3.5%] 3
All ❌✅ (primary) -0.7% [-6.3%, 6.4%] 6

Cycles

Results (primary -3.6%, secondary -3.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 2.3% [2.3%, 2.3%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -3.6% [-6.6%, -2.0%] 107
Improvements ✅
(secondary)		 -3.5% [-7.0%, -1.9%] 89
All ❌✅ (primary) -3.6% [-6.6%, 2.3%] 108

Binary size

Results (primary -0.0%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.0% [-0.1%, -0.0%] 8
Improvements ✅
(secondary)		 -0.1% [-0.1%, -0.0%] 35
All ❌✅ (primary) -0.0% [-0.1%, -0.0%] 8

Bootstrap: 512.944s -> 513.337s (0.08%)
Artifact size: 398.65 MiB -> 400.72 MiB (0.52%)

@rustbot rustbot added the perf-regression Performance regression. label Jun 5, 2026
@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Jun 5, 2026

Caused the perf regression in #157440

@jakubadamw
Copy link
Copy Markdown
Contributor Author

jakubadamw commented Jun 5, 2026 •
edited
Loading

@JonathanBrouwer, oh dear, sorry about that. Should we revert the PR before I look into optimising it?

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Jun 5, 2026

Given that the perf regression is relatively minor and only in secondary benchmarks and that this PR fixes a bug I don't think reverting makes sense.

If you could take a look at what causes the perf regression that would be awesome, but no hurry and if not we should just accept the minor regression :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@lcnr lcnr

Labels

perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.98.0

Development

Successfully merging this pull request may close these issues.

ICE: Failed to resolve instance for <() as Supertrait<()>>::method ICE: invalid CoerceUnsized impl_source: Err(FulfillmentError)

6 participants

@jakubadamw @rustbot @adwinwhite @lcnr @JonathanBrouwer @rust-timer