trait solver: Include implied outlives assumptions - #162238
Conversation
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? me |
158056d to
29846e5
Compare
|
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 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 Small thing I noticed while writing comments is that the destructuring in cc @BoxyUwU :) |
29846e5 to
84c1b90
Compare
|
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.
this makes complete sense to me 🤔 can you remove the elaboration logic from the solver's logic for computing assumptions and instead rely on 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 |
| // 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 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
why not use elaborate directly here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The universe filter runs before elaboration now instead of after
slightly sus but we can fix that once we have a test for it 🤔
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If you could move the universe checks into Assumptions::new that would be great
319f6ab to
6a1dfdd
Compare
This comment has been minimized.
This comment has been minimized.
|
cool! @bors r+ |
|
@bors r- ah wait actually |
|
This pull request was unapproved. |
6a1dfdd to
fe739c6
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. |
| ), | ||
| region_outlives.freeze(), | ||
| // Everything here is in the root universe already. | ||
| None, |
There was a problem hiding this comment.
Is there a reason to accept an Option here instead of passing in the root universe
There was a problem hiding this comment.
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.
fe739c6 to
822cbdf
Compare
|
@bors r+ rollup :3 |
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: 'adid not also give us'b: 'a. At the root it was a slightly different version of the same problem:known_type_outliveshas the explicit where clauses, while implied bounds from things like&'b selflive inregion_bound_pairs, so constraint destructuring never saw them.Assumptions::newnow 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: 'bfrom a receiver and a separate'b: 'arelation, 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.