trait_solver: Reject invalid canonical responses

[Dnreikronos]

Fixes #159704

The next solver could apply a canonical response even when a caller-side inference var couldn't name one of its placeholders. That leaked into writeback and ICEd. Proof-tree replay could hit it too, including inside a generic caller.

Response application can fail now, and relation changes commit in one pass only if the whole response works. Failed proof trees keep an identity state built from the solver's canonical vars. IMO, rejecting the response here is cleaner than carrying invalid state until an unrelated diagnostic trips over it.

[rustbot]

Some changes occurred to the core trait solver

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

[rustbot]

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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 74 candidates
• Random selection from 21 candidates

[BoxyUwU]

I think we definitely do not want to make this fallible. There's also not really enough information in your description to explain what was going wrong and why, can you please explain more in detail the issue you're fixing. Specifically, why are we getting placeholders in the response.

Though, I think non_lifetime_binders is somewhat unmaintained right now and I'm not sure how much we want to complicate the solver to make it work better.. cc @lcnr

[Dnreikronos]

@BoxyUwU the placeholder is B from the for<B> in the reproducer. I should've explained that in the PR description.

At trivial(), A starts as ?A@U0. Entering for<B> turns B into !B@U1, and matching dyn Fn(A, B) against Fn(A, A) wants ?A = !B. So far that part makes sense to me.

It gets weird when this goes through the nested canonical query. Input canonicalization puts both values in the root universe, so the query no longer knows that ?A@U0 can't name !B@U1. The response then maps the canonical var for A to the canonical placeholder for B. Once we instantiate it in the outer query, B is back in U1, and ResponseRelating raw-assigns it to ?A@U0. That's how writeback ends up seeing trivial::<B>.

I made response application fallible because this is the first point where the original universe mismatch becomes visible again. Returning NoSolution there seemed safer to me than applying the constraint and crashing later. I still think applying it is wrong. What I hadn't considered was your point that a returned response is supposed to always be applicable to its input, and imo that means the response shouldn't have been built in the first place. idk yet where the right place to prevent it is.

The leak check doesn't catch this btw. Inside the nested query it only sees the canonical root-universe vars, so there's nothing for it to reject.

Given the state of non_lifetime_binders, preserving more universe info during input canonicalization might be too much machinery for this case. I'll look into whether there's a smaller way to keep the response invariant. If not, treating this as a known limitation may be the better option. ltm what you think.

[adwinwhite]

These two `response-universe-lowering tests already pass with the next solver on night 🤔

View changes since the review

[Dnreikronos]

yeah, you're right. both of these already pass on nightly. I added them more as guard tests around the response changes, not as the reproducer for #159704. The actual regression test is canonical-response-placeholder-leak-issue-159704.rs, and that one still ICEs without the fix.

The HRTB one came from the stage2 failure I hit with the first version of the check. I was rejecting region placeholders too, which broke the IrPrint bound. imo that one is still worth keeping since it caught an actual problem in this PR.

the other one mostly makes sure type/const inference vars still get their universes lowered. tbh I'm not that attached to it. idk if you think that coverage is useful here, ltm and I can drop it or just add a comment saying why it's there.

[lcnr]

I think we definitely do not want to make this fallible. There's also not really enough information in your description to explain what was going wrong and why, can you please explain more in detail the issue you're fixing. Specifically, why are we getting placeholders in the response.

Though, I think non_lifetime_binders is somewhat unmaintained right now and I'm not sure how much we want to complicate the solver to make it work better.. cc @lcnr

very little for now 🤔 I don't have a lot of capacity to engage with this atm

[rust-bors]

☔ The latest upstream changes (presumably #160879) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.