-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't ICE because recomputing overflow goals during find_best_leaf_obligation causes inference side-effects #124871
Conversation
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized: BestObligation<'tcx>::visit_goal
always returns ControlFLow::Break
, doesn't it? This means that in the for nested_goals
loops, we always just use the first nested ImplWhereBound
of the loop 🤔 this is surprising and means the impl_where_bound_count
is actually unused?
candidates.retain(|candidate| candidate.result().is_ok()); | ||
} | ||
false => { | ||
candidates.retain(|candidate| candidate.result().is_err()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this retain
feels off: I would expecct us to never fail if there are any non-err candidates 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's probably useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls remove it or change it to an assert then 🤔
Not if we have an ambig impl where bound then an err impl where bound. |
…ligation causes inference side-effects
d3f570b
to
d3e510e
Compare
@bors r=lcnr rollup |
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#124871 (Don't ICE because recomputing overflow goals during find_best_leaf_obligation causes inference side-effects) - rust-lang#125018 (Update linker-plugin-lto.md to include LLVM 18) - rust-lang#125130 (rustdoc-json-types: Document `Id`) - rust-lang#125170 (Uplift `FnSig` into `rustc_type_ir` (redux)) - rust-lang#125172 (Fix assertion when attempting to convert `f16` and `f128` with `as`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#124871 - compiler-errors:overflowo, r=lcnr Don't ICE because recomputing overflow goals during find_best_leaf_obligation causes inference side-effects See the comments for more info. Reprocessing overflowed obligations may cause *other* goals to go from ambig -> pass/fail, causing an ICE. This suppresses that error, but makes all the overflow obligations messages back to their root obl. That kinda sucks, but 🤷 Fixes rust-lang#124834 Fixes rust-lang#124845 r? lcnr
See the comments for more info. Reprocessing overflowed obligations may cause other goals to go from ambig -> pass/fail, causing an ICE. This suppresses that error, but makes all the overflow obligations messages back to their root obl. That kinda sucks, but 🤷
Fixes #124834
Fixes #124845
r? lcnr