Repository navigation
trait_selection: Resolve inference variables after deep normalization - #161407
Dnreikronos wants to merge 2 commits into
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.
6a30ba2 to
98a31c2
Compare
|
r? lcnr Deep normalization is interesting. From the comment of that function When do we encounter aliases without any infer vars, whose normalization is ambiguous, then we normalize something else, and suddenly it works? Or is that a "we normalize and stay ambig, normalize the alias containing this alias, and this then succeeds and returns something which doesn't reference the infer var from the inner alias"? 🤔 Can you say more about why this example with nested inherent assoc types has this behaior or what exactly is going on here? |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
sup @lcnr :) I traced this again, and my explanation in the PR body is wrong. This test doesn't hit ambiguity at all. I saw the ambiguity handling in The input to The inner alias normalizes first: Then it tries the outer alias with that result folded into it: That returns Since the fold failed, Fulfillment later checks those obligations. The outer obligation still contains the original nested alias, and type relating the higher-ranked equality creates projection goals for that inner alias. This time it succeeds and constrains So your second guess was basically right. The difference is that the first attempt isn't ambiguous. It returns The actual bug is simpler than the path that gets us there. We build The old deep normalization code resolved each result after proving its obligation. The newer code proves the obligations together at the end, and the final resolve was lost in that change. I still think resolving after fulfillment is the right fix. The part that still looks odd to me is the IAT behavior. Eagerly normalizing the inner alias makes the outer goal return I was also wrong about the bad function body keeping the goal ambiguous. A valid body takes the same fallback path, but the test doesn't reach the MIR assertion. I'll fix that explanation, the code comment, and probably the test name. My vote is to keep this PR focused on the missing resolve and investigate the IAT behavior separately. Does that split make sense to you, or do you want the IAT part understood first? |
The fallback normalization path can build a value from fresh inference variables and return their obligations separately. Fulfillment may later constrain those variables without rebuilding the folded value. Resolve the value after fulfillment so deep normalization does not return inference nodes that the context has already constrained.
The failure is only observable through a debug assertion because the caller resolves the value before returning. Run the test only with compiler debug assertions enabled. Use a const function and an intentionally invalid body to reach the MIR normalization path in metadata-only UI tests.
98a31c2 to
306495f
Compare
|
sup, @lcnr o/ I just pushed the cleanup..the code comment now describes the fallback instead of ambiguity, and I renamed the test and fixed its explanation about the body error. The path is the one from my last commen, so if you could, please, re-validate: the eager fold normalizes the inner IAT, the outer goal gets I still think the resolve belongs in deep normalization. That is where the variable is created and where its obligations are run, so returning before applying the fulfillment result feels like the wrong boundary. I kept the strange higher-ranked IAT behavior separate. I still do not have a satisfying answer for why the eager goal fails while the fallback relation works, and I do not want to pretend the missing resolve explains that part. If you would rather have that understood before this lands, I can dig into it first. Otherwise I think a separate follow-up is cleaner. @rustbot ready |
|
So the bug here is that the I feel like this fix papers over a deeper type system bug and we should not do this change ideally. can you look into what's necessary to make the |
|
@rustbot author |
…normalization_failure, r=lcnr trait_selection: Preserve eager normalization failures Fixes rust-lang#160875 This started from rust-lang#161407 and the Zulip discussion around eager normalization failures. The nested inherent associated type case takes two paths. Eager normalization handles the inner alias first, then the outer alias fails the leak check. The old fallback throws that attempt away and retries the original aliases. That retry can succeed because the lifetime constraints come back through nested goals and the parent leak check does not see them. My first take was to resolve the inference variable after fulfillment. That made the debug assertion go away, but it was only hiding the failed relation. I do not think we should return a value from a path that already failed the leak check. `normalize_with_universes` now returns the failed obligation. Deep normalization passes that failure back instead of falling into recovery. Regular `normalize` stays infallible and keeps the fallback behavior because its callers already expect obligations. The fallback still has to rebuild those obligations. For higher ranked aliases, `ReplaceAliasWithInfer` leaves the alias in the folded value, replaces bound vars with placeholders only for the `Projection` obligation, and does not put the fresh inference term in the returned type. I missed that part in my first version and dropped too much. I think this is the cleanest split. Deep normalization gets the strict behavior it promises, while regular normalization still recovers in the same general way as before. The UI tests cover the nested IAT case and the affected diagnostics.
…normalization_failure, r=lcnr trait_selection: Preserve eager normalization failures Fixes rust-lang#160875 This started from rust-lang#161407 and the Zulip discussion around eager normalization failures. The nested inherent associated type case takes two paths. Eager normalization handles the inner alias first, then the outer alias fails the leak check. The old fallback throws that attempt away and retries the original aliases. That retry can succeed because the lifetime constraints come back through nested goals and the parent leak check does not see them. My first take was to resolve the inference variable after fulfillment. That made the debug assertion go away, but it was only hiding the failed relation. I do not think we should return a value from a path that already failed the leak check. `normalize_with_universes` now returns the failed obligation. Deep normalization passes that failure back instead of falling into recovery. Regular `normalize` stays infallible and keeps the fallback behavior because its callers already expect obligations. The fallback still has to rebuild those obligations. For higher ranked aliases, `ReplaceAliasWithInfer` leaves the alias in the folded value, replaces bound vars with placeholders only for the `Projection` obligation, and does not put the fresh inference term in the returned type. I missed that part in my first version and dropped too much. I think this is the cleanest split. Deep normalization gets the strict behavior it promises, while regular normalization still recovers in the same general way as before. The UI tests cover the nested IAT case and the affected diagnostics.
Rollup merge of #162618 - Dnreikronos:trait_solver_preserve_normalization_failure, r=lcnr trait_selection: Preserve eager normalization failures Fixes #160875 This started from #161407 and the Zulip discussion around eager normalization failures. The nested inherent associated type case takes two paths. Eager normalization handles the inner alias first, then the outer alias fails the leak check. The old fallback throws that attempt away and retries the original aliases. That retry can succeed because the lifetime constraints come back through nested goals and the parent leak check does not see them. My first take was to resolve the inference variable after fulfillment. That made the debug assertion go away, but it was only hiding the failed relation. I do not think we should return a value from a path that already failed the leak check. `normalize_with_universes` now returns the failed obligation. Deep normalization passes that failure back instead of falling into recovery. Regular `normalize` stays infallible and keeps the fallback behavior because its callers already expect obligations. The fallback still has to rebuild those obligations. For higher ranked aliases, `ReplaceAliasWithInfer` leaves the alias in the folded value, replaces bound vars with placeholders only for the `Projection` obligation, and does not put the fresh inference term in the returned type. I missed that part in my first version and dropped too much. I think this is the cleanest split. Deep normalization gets the strict behavior it promises, while regular normalization still recovers in the same general way as before. The UI tests cover the nested IAT case and the affected diagnostics.
…tion_failure, r=lcnr trait_selection: Preserve eager normalization failures Fixes rust-lang/rust#160875 This started from rust-lang/rust#161407 and the Zulip discussion around eager normalization failures. The nested inherent associated type case takes two paths. Eager normalization handles the inner alias first, then the outer alias fails the leak check. The old fallback throws that attempt away and retries the original aliases. That retry can succeed because the lifetime constraints come back through nested goals and the parent leak check does not see them. My first take was to resolve the inference variable after fulfillment. That made the debug assertion go away, but it was only hiding the failed relation. I do not think we should return a value from a path that already failed the leak check. `normalize_with_universes` now returns the failed obligation. Deep normalization passes that failure back instead of falling into recovery. Regular `normalize` stays infallible and keeps the fallback behavior because its callers already expect obligations. The fallback still has to rebuild those obligations. For higher ranked aliases, `ReplaceAliasWithInfer` leaves the alias in the folded value, replaces bound vars with placeholders only for the `Projection` obligation, and does not put the fresh inference term in the returned type. I missed that part in my first version and dropped too much. I think this is the cleanest split. Deep normalization gets the strict behavior it promises, while regular normalization still recovers in the same general way as before. The UI tests cover the nested IAT case and the affected diagnostics.
Fixes #160875
Deep normalization can return a value that still contains an inference variable after fulfillment has already resolved it. The fallback replaces the outer alias with a fresh variable and collects an obligation. Fulfillment updates the inference context later, but it doesn't rebuild the folded value, so the caller can still see
fn(?3t)after the context knows its final type.Resolve the value after fulfillment and add a UI test for nested inherent associated types. The test runs with compiler debug assertions because the caller resolves the value before returning, which hides the bug from normal output. I think deep normalization is the right place for the fix since it creates the variable and runs its obligation. After lcnr's review, I traced the fallback end to end and updated the comments and test name around the actual path: the eager outer goal returns
NoSolution, thennormalize_with_universesretries the original type withReplaceAliasWithInfer. The invalid body only reaches the MIR assertion. The odd higher-ranked IAT behavior looks separate to me, so I'd rather investigate it outside this fix.