trait_selection: Keep type-op region constraints in borrowck

[Dnreikronos]

View all comments

I ran into this while working on #158588. borrowck_env_fail still had a FIXME because the function body wasn't reporting the outlives error. The next solver creates the region constraint, but the canonical type-op path doesn't put it in QueryResponse. Borrowck never sees it, so the type op looks fine and the error is lost.

With -Zassumptions-on-binders, these type ops now run locally on borrowck's InferCtxt. The fast path still runs first. That leaves the constraint in the same inference context borrowck reads later.

I prefer this over adding more data to the old canonical response. The next solver already caches its work, and teaching the old query path about these constraints felt like extra machinery for something we can avoid. Running locally is pretty boring, but I think that's a good thing here. The old FIXME is now the regression test.

[rustbot]

r? @davidtwco

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

[BoxyUwU]

r? BoxyUwU

[BoxyUwU]

I think this is a partial revert of #160982 which we don't want. I think that teaching scrape_region_constraints to handle the new kind of constraints is exactly correct. The whole point of that function is to return the region constraints so it not doing that under -Zassumptions-on-binders seems like a clear bug to me :3

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Dnreikronos]

Sup,@BoxyUwU :)

Yeah, scrape_region_constraints makes more sense here. I went with the local type op first because it made the new constraints survive, but after tracing it a bit more I realized, as you said, that was just dodging the query path from #160982.

The next solver was storing the tree in the temporary query InferCtxt, but the query result only knew about the old region constraints. So when that context went away, the tree went with it. I changed QueryRegionConstraints to carry the tree too. It stays unspanned in the cached result, and the caller adds the local span when it registers it. scrape_region_constraints now returns the constraints made by that op without messing with whatever the caller already had.

I also found another place dropping the same tree during implied-bound normalization, before lexical regionck. I fixed that too and added regressions for that path and the borrowck case.

I like this version a lot more. It fixes the point where the data was lost and keeps the cache behavior from #160982. The local version worked, but mostly by going around the problem, and that felt wrong once I understood the full path.

@rustbot ready

[BoxyUwU]

why is this necessary for new style constraints but not old style?

View changes since the review

[Dnreikronos]

The difference is that the old obligations and assumptions are taken before the op and checked to be empty, so everything collected afterward belongs to that op. We cannot make that same check for the solver tree. Borrowck may already have constraints from an earlier type op in the same InferCtxt, and those stay there until the end of typeck. If we took the full tree afterward, this response would also contain the caller's older constraints. Clearing it first would lose them. The helper parks the old tree, captures what this op made, then restores the old one.

I do not love the extra swap, but I think this part is a real difference in how the constraints are stored and when they are consumed.

[BoxyUwU]

hmm I wouldn't expect there to be any kind of difference between how we move region constraints around. Do you know where that difference is originating from?

I noticed here in one of the functions you register the constraint to the Infcx instead of to the MirTypeckRegionConstraints: https://github.com/rust-lang/rust/pull/161423/changes#diff-22902f0e92a4c4111643ef62f169a481590d128657daab37c126d41c0b44985cR83.

Is that the underlying cause of this or is that downstream of whatever is causing the difference here? I would expect that we should extend MirTypeckRegionConstraints to also store new-style constraints and then we can push new constraints to that instead of into the InferCtxt

[Dnreikronos]

Yeah, I think you're right, Boxy

The old style constraints get converted into MirTypeckRegionConstraints inside convert_all, so the InferCtxt only ever holds what the current op just made and a take is enough. I was putting the new style tree back into the InferCtxt instead, and nothing drains it there, borrowck reads it once at the end of typeck. So by the second type op the first op's constraints are still sitting in the store. Taking would have put them in the wrong response and clearing would have dropped them, so I parked them and put them back. That's me working around a hole I made myself one file over, which is why it looked weird, because it was weird.

Did it your way. MirTypeckRegionConstraints has a solver_constraints field now, convert_all ands into that like it does with everything else, and scrape_region_constraints just takes from the InferCtxt next to the other two takes. with_fresh_solver_region_constraints is gone.

Two things fell out of it. The destructuring at the end of typeck pulls the tree out of MirTypeckRegionConstraints and passes it in, since borrowck doesn't keep it in the InferCtxt anymore, so destructure_solver_region_constraints takes the constraint as an argument. Regionck still reads it off the InferCtxt like before. And now the store really is empty in both spots where we already assert there are no leftover region obligations, so I added the same assert there instead of leaving it as folklore.

I like this a lot more. The swap worked, but only because I knew exactly who was writing to that store and when, and that's not an invariant anyone should have to carry around in their head.

Ran the ui tests for borrowck, nll, impl-trait, higher-ranked and coroutines on top of the assumptions-on-binders ones, the new asserts hold.

One thing I didn't touch is constraints registered after typeck is done, from the opaque type handling in region_infer, still go nowhere. That was already true before this, they were being registered into the InferCtxt after we had already read it. Now they just sit in the field instead. Happy to look at it, but it felt like a different PR.

[BoxyUwU]

this generally feels kinda off, why doesn't this just work the same way as or region_obligations/region_assumptions?

View changes since the review

[Dnreikronos]

Yeah, agreed. I changed make_query_region_constraints to take the solver tree together with region_obligations and region_assumptions. The normal query path and scrape_region_constraints now build QueryRegionConstraints through the same function, and callers that cannot produce a solver tree pass the empty one.

The temporary swap is still needed so we do not mix a solver tree already registered by the caller into this op's response, but the query response side no longer bolts the field on afterward. I think this reads much better.

[BoxyUwU]

what's with all of these checks for !constraint.is_true() is this necessary or just a perf opt? if it's a perf opt I'd prefer we drop it.

View changes since the review

[Dnreikronos]

The checks do matter with the current representation, but putting them at every caller was the wrong place. true is And([]). Pushing that into the store turns it into And([And([])]); it still means true, but is_true() only recognizes the empty outer And, so an empty query starts looking nonempty.

I moved that check into the store instead. Registering a trivial tree now does nothing and does not add an undo-log entry, so the callers can register unconditionally. There is a unit test for the empty-query case too.

[BoxyUwU]

Is this still the case after rebasing on my PR to always store stuff in canonical form?

[Dnreikronos]

Nope, not anymore. With your PR in, And::new/Or::new keep things canonical, so build_and(true, x) really is x and is_true() still holds after it. The And([And([])]) I was dodging can't happen now.

So it's gone. The callers stopped checking last round and the early return in register_type_outlives_constraint_inner was the last piece of it, all it saves at this point is a realloc and an undo entry.

I did keep the small unit test, but it's pinning a different thing now: registering a trivially true constraint has to leave the query response empty, which is the canonical form guarantee doing the work. Reworded the comment to say that instead.

[BoxyUwU]

@rustbot author

[rustbot]

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

[Dnreikronos]

@rustbot ready

[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.