trait_solver: Add minimal coroutine binder assumptions - #162159
trait_solver: Add minimal coroutine binder assumptions#162159Dnreikronos wants to merge 6 commits into
Conversation
| } | ||
|
|
||
| impl AssumptionsOnBinders { | ||
| pub fn is_enabled(self) -> bool { |
There was a problem hiding this comment.
I think I would expect to call this any_is_enabled to make it clear that this isn't full AoB
There was a problem hiding this comment.
yep, i just adjusted. I also added is_full next to it so all three states have a name, otherwise people end up writing !is_min_coroutines() and hoping that means what they want.
| @@ -2715,7 +2715,11 @@ impl<'tcx> TyCtxt<'tcx> { | |||
| } | |||
|
|
|||
| pub fn assumptions_on_binders(self) -> bool { | |||
There was a problem hiding this comment.
similarly I feel like this should either be assumptions_on_binders_any and we should have another method like assumptions_on_binders_full
There was a problem hiding this comment.
Done, on TyCtxt and on Interner. Everything that was already calling it became _any, because all of those are gating the shared machinery: the constraint storage, the NextGen response, the regionck and borrowck destructuring, the fast path bail. Only three places actually want _full.
| self.placeholder_assumptions_for_next_solver | ||
| .borrow_mut() | ||
| .insert(u, Some(rustc_type_ir::region_constraint::Assumptions::empty())); | ||
| let assumptions = (!self.tcx.assumptions_on_binders_min_coroutines()) |
There was a problem hiding this comment.
This means we do it even if no AoB is enabled 🤔 should be a tcx.assumptions_on_binders_full()
There was a problem hiding this comment.
Yeah, you're right, that was wrong. It's _full().then(...) now, so with the flag off we store None for that universe. In practice nothing reads the map unless abby is on at all, so it wasn't doing damage, but it read like it was.
| /// | ||
| /// The minimal coroutine mode keeps these intact until region checking so that enabling the | ||
| /// mode does not strengthen the eager leak check. | ||
| TypeOutlives(I::Ty, Region<I>, S), |
There was a problem hiding this comment.
This feels somewhat unfortunate and undesirable to me 🤔 Though I understand why you've added it. I imagine this overlaps slightly with rust-lang/project-assumptions-on-binders#18 where there we'll need to decide how to handle type outlives constraints in a unified manner between old and new style 🤔
I'll need to spend some time thinking about what to do here.
In general it feels like things get quite significantly complicated by moving un-destructured TypeOutlives constraints into LeafRegionConstraint. An alternative could be to instead when min-coroutines is enabled use the normal type_outlives storage that we do on stable 🤔
I don't know whether that's actually a good idea or not 😅 I do just need to sit down and figure out what the plan is for type outlives constraints I think 🤔
There was a problem hiding this comment.
Yep, fair, I wasn't liking it either, so I went and tried deleting it.
The idea was that destructuring a type outlives constraint gives you its components, and anything the assumptions can prove about the whole they can prove about the parts. That holds for params and placeholders. It doesn't hold for composites or aliases.
The case that changed my mind is that assumption is T: 'a, goal is (T,): 'a. Left whole, nothing matches it, so it gets kept and goes to the root. Destructured, T: 'a falls back out, matches, and gets dropped. That's the eager leak check getting stronger, which is what rust-lang/project-assumptions-on-binders#23 is about. I built it both ways instead of arguing from the code, and it does flip, so there's a test now that fails if the leaf goes away.
Aliases are worse in theory but you can't see it yet. Alias: 'r destructures into an or of item bounds, the via-env leaf and the components, and the minimal drop step just says false for via-env, so an exact Alias: 'r assumption has nothing left to match against. Except that matching is already broken anyway: the assumption comes in unnormalized and the goal comes in normalized, so the two never compare equal. There's already a FIXME sitting on that. So today it fails either way. I left that case in the same test with a note, so it starts failing the moment someone fixes normalization.
On using the stable type_outlives storage instead, I don't think that works. The goals we need the witness binder to discharge are type outlives goals themselves, the T: 'w falling out of the auto trait impls on the witness types. Put those in stable storage and the binder assumptions never see them, so the mode ends up doing nothing.
My take is that the leaf is ugly but it's the smallest thing that's right today. Once
rust-lang/project-assumptions-on-binders#30 is in I'd rather do what lcnr suggested on rust-lang/project-assumptions-on-binders#23, destructure eagerly again and mark the leaves as not visible for the leak check, and then this variant can just go. If you'd rather wait for that than take the leaf now, fine by me, just say Boxy :)
| for c in and.0 { | ||
| match c { | ||
| Ambiguity(()) | RegionOutlives(..) => rewritten_constraints.push(Or::new_leaf(c)), | ||
| TypeOutlives(ty, region, ()) => { |
There was a problem hiding this comment.
this ought to be unreachable with min_coroutines right? because we never call this function unless we're full AoB, and if we're full AoB we've eagerly destructured things in compute_type_outlives_goal?
There was a problem hiding this comment.
It's reachable, annoyingly. The minimal path only kicks in when the universe has assumptions, and in this mode that's witness binders only. Everything else gets None, falls through to the normal path, and shows up in that arm with whole TypeOutlives leaves.
If we move non-witness binders to Some(empty), which I wrote up on rust-lang/project-assumptions-on-binders#33, this arm should become dead and I'd happily swap it for an unreachable!().
| self.infcx.insert_placeholder_assumptions(u, Some(Assumptions::empty())); | ||
| self.infcx.insert_placeholder_assumptions( | ||
| u, | ||
| (!self.cx().assumptions_on_binders_min_coroutines()).then(Assumptions::empty), |
There was a problem hiding this comment.
same stuff here about !min_coroutines applying to stable, it should be cx().assumptions_on_binders_full()
There was a problem hiding this comment.
Done, both sides use _full() now. Same mistake as the enter_forall_with_empty_assumptions one, same fix.
Keep type-outlives constraints intact while leaving a minimal-mode binder, then remove only leaves proven by that binder. Ordinary binders continue through the normal eager leak check.
The predicate was called `assumptions_on_binders`, which read as "the flag is on" and gave no way to ask which mode is active. Callers that gate the shared machinery want "any mode", while callers that gate the eager placeholder rewriting want "the full mode" specifically. Split it into `any_is_enabled`, `is_full` and `is_min_coroutines` on the option, and expose all three through `TyCtxt` and `Interner`.
Entering a binder with no assumptions recorded an empty assumption set whenever the minimal coroutine mode was off, which includes the case where assumptions on binders is disabled entirely. Storing `None` there instead keeps the map meaningful: a universe has an entry only when some mode actually computed one for it.
The shared binder tests only exercised region outlives constraints, and the two alias cases assert on the rewrite that the full mode performs, which the minimal mode deliberately skips. Add a case where an assumption names the component while the goal names the composite. Keeping the constraint whole leaves it for the root, and destructuring it would reduce it to the component and discharge it, so the two representations disagree. Also record that an assumption naming an alias exactly fails to discharge it, because assumptions are lowered without normalization and so compare unequal to the normalized goal.
16bb429 to
bed5ff5
Compare
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
|
☔ The latest upstream changes (presumably #162487) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
Implements rust-lang/project-assumptions-on-binders#33.
Based on #161306, so the diff is going to look noisy until that lands.
The full binder-assumptions mode does a lot when it leaves a binder, including rewriting placeholder constraints into lower universes. For coroutine witnesses we only need a much smaller bit of that. The main thing I wanted to avoid was making a "minimal" flag that quietly got most of the full behavior anyway.
min_coroutinesonly builds assumptions for coroutine-witness binders. When it leaves one, it drops a constraint if that binder directly proves it and keeps the rest for the root context. Ordinary HRTBs still take the normal eager leak-check path. Type-outlives constraints also stay whole until the root; pulling them apart earlier can make the mode accept more than intended.Fwiw I think this is a nicer first version because the rule is pretty easy to explain: witness binders get assumptions, other binders don't. It gets the async cases we're after without turning on the wider experiment. I added the existing async examples and a couple of focused tests for the edges that worried me, ordinary binders and unsatisfied constraints leaving the binder.