Skip to content

trait solver: Include implied outlives assumptions - #162238

Open
Dnreikronos wants to merge 3 commits into
rust-lang:mainfrom
Dnreikronos:trait_solver/implied_outlives_assumptions
Open

trait solver: Include implied outlives assumptions#162238
Dnreikronos wants to merge 3 commits into
rust-lang:mainfrom
Dnreikronos:trait_solver/implied_outlives_assumptions

Conversation

@Dnreikronos

@Dnreikronos Dnreikronos commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

View all comments

Part of rust-lang/project-assumptions-on-binders#19

Split out of #161988 after @BoxyUwU pointed out that these are about which assumptions we keep, not really about reflexive region constraints.

I went back through where each piece comes from and found two gaps. Inside a binder we kept Ty: 'a, but the region relation only knew about explicit region clauses. That means something like &'b T: 'a did not also give us 'b: 'a. At the root it was a slightly different version of the same problem: known_type_outlives has the explicit where clauses, while implied bounds from things like &'b self live in region_bound_pairs, so constraint destructuring never saw them.

Assumptions::new now pulls the free region components out of type outlives clauses and adds those edges to the region relation. I think doing it there is the cleanest spot. All callers get the same view of an assumption, and the original type clauses stay around for placeholder and alias cases. Regions bound inside the type are ignored because they do not name anything we can use outside that binder.

The root path now adds its implied type bounds to the same assumption set before destructuring. The regression uses an implied I: 'b from a receiver and a separate 'b: 'a relation, so it covers this without leaning on the reflexive fix from #161988. There are also binder checks for a reference and a higher-ranked function type. Those caught an easy testing trap here: a green direct constraint could have depended on the other PR, so the checks look at the lifted candidates instead.

Personally, I think splitting this was the right call. It is really a change to how assumption data is built, and that is easier to reason about on its own than under the reflexive constraint fix.

cc @BoxyUwU, this is the pair of changes you asked me to pull out.

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Sep 3, 2026
@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

r? @jackh726

rustbot has assigned @jackh726.
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 75 candidates
  • Random selection from 20 candidates

@BoxyUwU

BoxyUwU commented Sep 3, 2026

Copy link
Copy Markdown
Member

r? me

@rustbot rustbot assigned BoxyUwU and unassigned jackh726 Sep 3, 2026
@Dnreikronos
Dnreikronos marked this pull request as draft September 3, 2026 13:12
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2026
@Dnreikronos
Dnreikronos force-pushed the trait_solver/implied_outlives_assumptions branch from 158056d to 29846e5 Compare September 3, 2026 14:43
@Dnreikronos

Dnreikronos commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Pushed some more changes after staring at this a bit longer, quick summary of what I did and why.

Borrowck had the same gap I fixed at the regionck root. Its known_type_outlives_obligations only has the explicit where clauses too, and the implied bounds sit right next to it in region_bound_pairs, so now those get passed in as well. While doing that I noticed GenericKind::to_ty already does what my hand written match did, so both paths now share a small helper that uses it. I folded this into the existing root commit since it's really the same fix, which is why the force push.

I couldn't come up with a test for the borrowck half. As far as I can tell borrowck still gets an empty solver constraint tree, so any test passes with or without this. Once the type-op constraints from #161423 reach borrowck it becomes testable and I'd add the regression over there. If you'd rather I drop this part until then, that's fine too.

The last commit is the one I'm not sure belongs here, so I kept it separate and easy to drop. The free region map stores its edges as 'sub <= 'sup, while the relation in Assumptions gets read the other way, 'longer: 'shorter. That mismatch is older than this PR and nothing notices it today because the root only ever looks at type_outlives. But Assumptions::new now merges the edges it derives from type outlives clauses into that same relation, so the regionck path would end up with edges pointing both ways in one relation. Felt wrong to leave that in once I'd seen it, so I invert the edges before building the assumptions and wrote down the expected direction on the field. No test here for the same reason as above, nothing at the root reads the relation yet.

Small thing I noticed while writing comments is that the destructuring in Assumptions::new is redundant for the solver path, since that one already elaborates before building the relation. It still matters for the callers that build assumptions straight from where clauses, the test harness and the root paths. I'd rather have one place that guarantees the shape than trust every caller to remember to elaborate, but happy to move it into the harness if you prefer.

cc @BoxyUwU :)

@Dnreikronos
Dnreikronos force-pushed the trait_solver/implied_outlives_assumptions branch from 29846e5 to 84c1b90 Compare September 3, 2026 16:15
@Dnreikronos
Dnreikronos marked this pull request as ready for review September 4, 2026 02:07
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2026
@BoxyUwU

BoxyUwU commented Sep 4, 2026

Copy link
Copy Markdown
Member

Hmm okay this all makes sense to me 🤔 I think we should definitely wait for #161423 so that the borrowck path can actually be tested.

I'd rather have one place that guarantees the shape than trust every caller to remember to elaborate

this makes complete sense to me 🤔 can you remove the elaboration logic from the solver's logic for computing assumptions and instead rely on Assumptions::new to do it.

The other thing is that I'd actually prefer for the test suite to not rely on elaboration and instead hand write out all of the relevant requirements. When I'm looking at the custom test suite DSL I want to be able to see exactly what assumptions we have and not worry about there being "extra" ones hidden behind the scenes.

Could you make it so that we have an Assumptions::new_unelaborated or something and use that from the test suite :3

// Regions bound inside the type are ignored, but free regions still contribute outlives edges.
core::test_binder_constraints! {
impl<'b, 'd: 'b + 'static> {
forall<'a> where for<'c> fn(&'c (), &'b u8): 'a {

@BoxyUwU BoxyUwU Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not be even letting you write such where clauses on forall since they don't do anything without elaboration (and I don't think we want to elaborate these).

cc rust-lang/project-assumptions-on-binders#39

View changes since the review

@Dnreikronos Dnreikronos Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, make sense. Moving the harness to new_unelaborated makes it pretty obvious, the clause does nothing now, which is why both of those tests are gone.

Didn't touch the rejecting part in the DSL though. Felt like it belongs in the assumptions-on-binders issue you linked and not here, but tell me if you want it in this PR.

// The type outlives assumptions are still kept around as they are required for proving
// placeholder and alias outlives.
//
// This mirrors `elaborate`, in particular in how it deals with binders: they're simply

@BoxyUwU BoxyUwU Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use elaborate directly here?

View changes since the review

@Dnreikronos Dnreikronos Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to elaborate, good call. Bonus is we keep the derived type outlives now too, so Vec<T>: 'a gives us T: 'a. My old loop was throwing that away.

One catch, and it's why the signature changed more than you probably expected. This has to take clauses, not just the outlives ones. Trait clauses imply outlives through supertraits, so T: Bound<'a> with trait Bound<'c>: 'static is evidence for T: 'static, and WF collection definitely puts trait clauses in reqs. The old solver code got away with dropping traits because its catch-all arm ran after elaborate. Filter to outlives first and they're gone before elaborate ever sees them.

Didn't catch this by reading it, to be honest. The change just felt off to me, so I hacked this function to elaborate only the non-outlives inputs and print whatever came out:

PROBE-LOST: [TraitClause(<!0 as Bound<'a>>)] => [OutlivesClause(!0, 'static)]

So it's new(cx, clauses, region_outlives) now. I think that's better anyway, not just me patching around a bug. If one place is supposed to guarantee the shape, give it the whole clause set. Otherwise every caller still has to think about what's safe to drop first, which is the thing we were trying to get away from.

One bit I'm not sure about. The universe filter runs before elaboration now instead of after. So with something like (&'b u8, &'c u8): 'a where 'b is in a higher universe, the whole clause lands in 'b's bucket and 'c: 'a stops showing up in 'c's. Before, each derived edge went to its own universe. I spent a while trying to write something that actually breaks because of it and got nowhere, so I left it. Can add a universe aware constructor if you want the old behaviour back, it's small.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The universe filter runs before elaboration now instead of after

slightly sus but we can fix that once we have a test for it 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually nvm i misread this :3 would very much like to avoid having assumptions for a universe where the max_universe of the assumption is smaller than the universe itself

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could move the universe checks into Assumptions::new that would be great

Comment thread compiler/rustc_infer/src/infer/outlives/obligations.rs
@Dnreikronos
Dnreikronos force-pushed the trait_solver/implied_outlives_assumptions branch 2 times, most recently from 319f6ab to 6a1dfdd Compare September 7, 2026 16:27
@rustbot

This comment has been minimized.

@BoxyUwU

BoxyUwU commented Sep 8, 2026

Copy link
Copy Markdown
Member

cool!

@bors r+

@rust-bors

rust-bors Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 6a1dfdd has been approved by BoxyUwU

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.

Reason for tree closure: spurious failures

@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 Sep 8, 2026
@BoxyUwU

BoxyUwU commented Sep 8, 2026

Copy link
Copy Markdown
Member

@bors r-

ah wait actually

@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 8, 2026
@rust-bors

rust-bors Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

This pull request was unapproved.

View changes since this unapproval

@Dnreikronos
Dnreikronos force-pushed the trait_solver/implied_outlives_assumptions branch from 6a1dfdd to fe739c6 Compare September 8, 2026 15:18
@rustbot

rustbot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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.

),
region_outlives.freeze(),
// Everything here is in the root universe already.
None,

@BoxyUwU BoxyUwU Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to accept an Option here instead of passing in the root universe

View changes since the review

@Dnreikronos Dnreikronos Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good reason, just me being unsure. Changed it to a plain UniverseIndex and passing ROOT here.

What I was worried about is something reaching the root sitting above the root universe and getting filtered out. Borrowck was the one I couldn't talk myself out of, since its region_bound_pairs come through as region vars and I didn't know off the top of my head what universe those end up in.

So I measured it instead of guessing. Put a print in Assumptions::new for every call with None dumping the max universe of each clause after elaboration, then ran the whole binder suite. 180 calls on the root path, 55 clauses through it, none of them outside the root universe. So ROOT never drops anything and the Option wasn't buying me anything.

Ends up smaller too, and the is_none_or in the filter is gone.

;)

`Assumptions::new` now elaborates the clauses it is given, so callers
which build assumptions straight from where clauses no longer each have
to remember to do it themselves. A `Ty: 'a` clause also tells us that
every region component of `Ty` outlives `'a`, and that the components
themselves do, which placeholder and alias outlives need.

It takes clauses rather than only the outlives ones because trait
clauses imply outlives through their supertraits: `T: Bound<'a>` with
`trait Bound<'c>: 'static` is evidence for `T: 'static`. Narrowing the
input to outlives clauses would drop those before elaboration could
reach them.

The test harness keeps using `new_unelaborated` so that a `forall`'s
assumptions are exactly the ones written down in the test, with no
extra ones hidden behind the scenes.
`known_type_outlives` only holds the explicit `Ty: 'a` where clauses.
The implied bounds, e.g. `T: 'a` from a `&'a T` argument, are tracked
separately in `region_bound_pairs`, so both have to be passed in.
Without them we fail to prove `T: 'a` for a `&'a T` argument whenever
the only explicit bound on `T` mentions a different region.
`FreeRegionMap::relation` stores `'sub <= 'sup` edges while
`Assumptions::region_outlives` expects `'longer: 'shorter` ones. The
mismatch is not yet observable as nothing reads the region relation at
the root, but `Assumptions::new` merges edges derived from type
outlives clauses into the same relation, which would otherwise leave it
with mixed edge directions.
@Dnreikronos
Dnreikronos force-pushed the trait_solver/implied_outlives_assumptions branch from fe739c6 to 822cbdf Compare September 8, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants