Diagnostics ICE when replaying proof trees with next-solver#154329
Diagnostics ICE when replaying proof trees with next-solver#154329VicenteGusmao wants to merge 2 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
|
r? @TaKO8Ki rustbot has assigned @TaKO8Ki. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8c7b38e to
30c3bd2
Compare
|
r? lcnr |
|
this is a surprising failure, while revisiting goals can get different results with proof trees, instantiating the response generally shouldn't fail 🤔 can you provide some more detail how this test ends up returning some canonical state we cannot instantiate. What is the constraint we get when replying the proof tree but not while actually proving the goal please generally move fix #issuenum into the PR description instead of the title |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
The failure is not in the original proof of the goal, but in the diagnostics-only replay of a stored We save a canonical state while proving the goal, and later try to replay that state after more trait solving has happened in the same For this testcase, the ICE only happens with I have not yet isolated the exact replay mismatch for this testcase, i.e. the specific constraint that is present during replay but not during the original proof, but I can instrument that next if that would be useful. I have also moved the #issuenum from the title to the PR description. |
|
@rustbot ready |
This is a bit incorrect. We usually unify the What happens in the problematic code is the following: We have a failed root goal But as there aren't any orig value for rust/compiler/rustc_next_trait_solver/src/canonical/mod.rs Lines 323 to 336 in c753cef But this inserts a new ty var in the infcx's current universe rather than the universe of var kind bound to that canonical var value, which is a HR as being in rust/compiler/rustc_trait_selection/src/solve/delegate.rs Lines 158 to 167 in c753cef So, the error is created while trying to unify the ty var from root universe to the binder's universe and I guess we should fix |
|
so this is caused by a universe error 🤔 yeah, that's an annoying problem, so we should instantiate the canonical state and between that and unifying the var_values should we create the infer vars 🤔 unsure how to impl this as we don't support instantiating a response without guessing having a var_value guess first. I guess we could create infer vars which are just in the MAX_UNIVERSE. I think this shouldn't blow up anything |
|
Ahh, that makes sense — thanks for the clarification. You're right that my previous explanation was off. So the core issue seems to be that For possible fixes, I see two options:
I can try prototyping either approach. Do you have a preference for which direction better fits the solver design? |
hard to do as with the current setup we need to crate the variables before we enter create these new universes. so the second alternative should be easier |
|
in case you are using an LLM to generate/cleanup your messages, please don't. try to state your current understanding in your own words, otherwise it's hard to figure out your current understanding and what I actually need to respond to. It also makes it hard to know whether it's worth it to correct cases where the perspective/framing is slightly off. Even if writing it yourself is less structured or clear, that's still preferable to me. idk if there's a more complete writeup i can point u to here |
|
Okay, I'll keep my comments more direct. I'll try to implement the second approach. |
This comment has been minimized.
This comment has been minimized.
92d2729 to
0d5cae8
Compare
This comment has been minimized.
This comment has been minimized.
When diagnostics replay proof tree state, rebuilding a canonical state can fail to match the current inference state. With -Znext-solver=globally, this could panic while reporting an error. Make proof tree replay fallible in diagnostics and fall back to the current obligation when replay fails. Add a regression test for the higher-ranked PartialEq and PartialOrd case. Signed-off-by: Vicente Gusmão <[email protected]>
…ual universe mismatch during proof tree replay. Since my last implementation was not really fixing the problem I removed the functions: try_unify_query_var_values, try_instantiate_canonical_state and InspectCandidate::try_instantiate_nested_goals. I also reverted the diagnostic callers in derive_errors.rs to use instantiate_nested_goals again. For my new implementation, in the rustc_trait_selection I changed fresh_var_for_kind_with_span to fresh_var_for_kind_in_universe which now uses next_region_var_in_universe, next_ty_var_in_universe and next_const_var_in_universe. Also in instantiate_canonical_state it is now ensured that infcx has all the universes up to prev_universe + state.max_universe, and creates any replay-only fresh vars in that explicit universe. I also split out compute_query_response_instantiation_values_in_universe so replay uses that same explicit base universe when reconstructing placeholders from the canonical state. Signed-off-by: Vicente Gusmão <[email protected]>
0d5cae8 to
ee0a796
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
When diagnostics replay proof tree state, rebuilding a canonical state can fail to match the current inference state. With -Znext-solver=globally, this could panic while reporting an error, avoiding the panic.
Make proof tree replay fallible in diagnostics and fall back to the current obligation when replay fails. Add a regression test for the higher-ranked PartialEq and PartialOrd case. Fixes #151304.