trait solver: Capture binder region constraints while relating

[Dnreikronos]

View all comments

Fixes #157859

The issue repro is a pretty good example of how this slipped through: the principal upcast path looked fixed, but other relation paths could still drop binder-region constraints. Fwiw, I think centralizing this in the relation code is the cleaner move here, instead of chasing every object-upcast call site one by one.

This records NextGen region constraints from normal relation, structural alias equality, and the small goal-returning relation helper used by object candidate code. ReVars keep their concrete outlives edges too, so we don't flatten useful info into plain ambiguity. Added the original issue repro and a projection-bound variant, e.g. the kind of path I'd expect to regress later if this only lived in the principal upcast code.

[rustbot]

r? @mejrs

rustbot has assigned @mejrs.
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, types
• compiler, types expanded to 73 candidates
• Random selection from 21 candidates

[mejrs]

r? @lcnr

[lcnr]

r? BoxyUwU

[rustbot]

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

[Dnreikronos]

@BoxyUwU fwiw, i think the ci failure is still compatible with your suggestion. the direct registration path lgtm to me, but the current version is probably registering from too many relation contexts.

the bad bit seems to be solverrelating::regions(): under -zassumptions-on-binders it always pushes regionoutlives into the solver constraint storage. some of those relation calls happen under enter_forall_with_empty_assumptions, so later placeholder handling has no assumptions to discharge the constraint and we hit the max_universe(...) < u assert.

imo the fix is not to go back to manually bubbling a list everywhere. i'd rather keep direct registration, but only in contexts where the solver owns the binder assumptions, or keep the constraints local while relating a binder and pull them out before registering. idk which shape is cleaner yet, but unconditional registration in regions() seems too wide.

[Dnreikronos]

follow-up after pushing 0d93b4c:

i tried to keep the direct-registration shape, but idk, it still looked too broad in practice. buffering constraints around binder relation did not clear the reported failures for me.

imo the safer fix is to keep the region constraints local while relating, then register the collected next-gen constraint only from the solver-owned relation entrypoints. ltm this still follows the direction boxy was pointing at, without letting solverrelating::regions() register constraints from the empty-assumption binder probes.

i checked the same failure cases locally:

• x.py check compiler/rustc_type_ir compiler/rustc_next_trait_solver
• x.py test tests/ui/assumptions_on_binders/alias_outlives.rs tests/ui/assumptions_on_binders/type_relation_binders_inside_solver-1.rs tests/ui/assumptions_on_binders/type_relation_binders_inside_solver-2.rs tests/ui/assumptions_on_binders/type_relation_binders_inside_solver-3.rs tests/ui/assumptions_on_binders/principal-upcast-region-eq-issue-157859.rs tests/ui/assumptions_on_binders/trait-upcast-projection-region-eq.rs
• x.py test tidy

[BoxyUwU]

some of those relation calls happen under enter_forall_with_empty_assumptions, so later placeholder handling has no assumptions to discharge the constraint and we hit the max_universe(...) < u assert.

in theory that shouldn't cause us to hit an assert. if we have empty assumptions then we'll rewrite the constraints to false and (eventually) get an error 🤔 if we were doing this in a enter_forall_without_assumptions it would make sense to me we get ICEs. though I wouldn't expect it to be due to the max_universe(..) < n assertion 🤔 I would expect it to be due to missing entries in the assumptions list in the InferCtxt.

i would like to fully understand what's going on with the ICEs when always registering the next gen region constraints :3 can you look more into what's going on there, if you could push the code that causes those ICEs to happen so I can see the CI failure too that would be useful :3

[Dnreikronos]

i looked into the unconditional direct-registration version locally.

imo it confirms the failure mode from my earlier guess: the new repro tests pass, but the broader assumptions-on-binders set fails.

specifically:

• alias_outlives.rs hits the max_universe(infcx, constraint.clone()) < u assertion
• the type_relation_binders_inside_solver-* tests either ice or reject code that should pass

so idk, direct registration from solverrelating::regions() still feels too broad to me. lgtm in principle, but only if we can keep it out of binder probes with empty assumptions. ltm the current local-collection shape is the safer boundary for now: collect while relating, then register from the solver-owned relation entrypoints.

i can push the failing experiment to a separate branch if seeing ci on that exact shape would help, but i'd rather not force-push it over this pr branch since the current version is passing.

checked with:

• x.py check compiler/rustc_type_ir compiler/rustc_next_trait_solver
• x.py test tests/ui/assumptions_on_binders/alias_outlives.rs tests/ui/assumptions_on_binders/type_relation_binders_inside_solver-1.rs tests/ui/assumptions_on_binders/type_relation_binders_inside_solver-2.rs tests/ui/assumptions_on_binders/type_relation_binders_inside_solver-3.rs tests/ui/assumptions_on_binders/principal-upcast-region-eq-issue-157859.rs tests/ui/assumptions_on_binders/trait-upcast-projection-region-eq.rs
• x.py test tidy

[BoxyUwU]

like I said, I don't understand why it's hitting that assertion 😅 can you explain why that assertion is getting hit. i can't properly evaluate this alternate approach without understanding why the other one doesn't work

[Dnreikronos]

yeah, my bad! i think i explained the empty-assumptions bit too confidently there.

i pushed the direct registration version again so ci can show the actual failure: 31be28b

idk yet why it ends up at the max_universe assert instead of just rewriting to false, so ltm the useful next step is to let ci capture the full logs on this exact shape.

[BoxyUwU]

@rustbot author

gonna do that while you figure out what's going on there, if you're having trouble we can chat about it over zulip and try figure it out together

[Dnreikronos]

@rustbot ready

[BoxyUwU]

Spent some time doing thinky think about this. I think the best way to move forward would be to fix the evaluation of OR and see if that lets you remove the normalize_equated_regions step you added 🤔 if it needs other fixes that would also be good to know.

I prefer fixing the OR thing over the other options as making PlaceholderReplacer work better feels like it's just working around OR evaluation being wrong 🤔

In an ideal world we'd land the OR fix as a separate PR first with its own self contained tests that rely on that fix. I think that's only worth it if you can come up with a test for that, otherwise i'd be happy to land it in this PR assuming it lets you remove normalize_equated_regions

[Dnreikronos]

Sup @BoxyUwU :)
I tried fixing the Or evaluation first, like you suggested, then removed the normalization again to see what was still missing.

There were two sides to the Or bug. Collapsing the whole thing as soon as we saw Ambiguity threw away candidates that a later universe could still prove. But just dropping the ambiguous branch was wrong too. If we have
Ambiguity ∨ remaining and remaining later becomes false, the result should still be ambiguous, not false. Otherwise we end up returning NoSolution even though the unknown branch may have worked.

Mixed Ors now keep both the ambiguity and any candidates that are still open. A true candidate still wins, and if every concrete candidate later becomes false, the ambiguity is still there. I pulled that logic into a small
combine_or helper so it could have a self contained test without needing a fake Interner. alias_outlives also covers the case where the concrete candidate succeeds.

After that I removed normalize_equated_region_vars again. alias_outlives was fine, but implied_higher_ranked_alias_outlives_assumption still went ambiguous with E0283.

What seems to happen there is that the current universe region var and the placeholder are equated inside the same And branch, but alias/env matching runs before the later region closure processes that equality. Without
normalization, the matcher still sees the region var and cannot use the higher ranked assumption. Normalizing first makes it see the placeholder instead. The normalization only replaces current universe region vars that
are actually equated with a non-var region, including equality through another region var.

I also tried teaching PlaceholderReplacer to rewrite those region vars. That went too far and bound both lifetimes, which is exactly the bad rewrite described in the test comment. The test started hard erroring instead of
going ambiguous. imo that makes PlaceholderReplacer the wrong place for this.

One other detail is the final regionck path. A mixed Or keeps its ambiguity while the solver is still processing it. When we finally register outlives constraints, we use the concrete candidate if one remains and only emit
the unknown implied bounds error when no concrete candidate is left.

So I think these are two separate fixes. The Or change preserves possible branches, while the normalization makes an existing equality visible early enough for alias matching. I don't love having an extra normalization
step here, but after tracing the order again, it feels cleaner than making PlaceholderReplacer rewrite more than it should.

Now that there is a self contained test for the Or behavior, would you prefer that part split into its own PR first, or keep both fixes here since this path needs both?

[rustbot]

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.