trait_solver: Preserve concrete candidates in ambiguous ORs - #162442
Dnreikronos wants to merge 9 commits into
Conversation
rustbot
added
S-waiting-on-review
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
A reflexive `'a: 'a` leaf is always satisfied, so filter it out in `And::new` instead of pattern matching for it in the places which happen to build such a constraint. An AND which ends up empty is trivially true, which makes the OR containing it true. This is how a reflexive candidate discharges a root type outlives constraint, so `destructure_type_outlives_constraints_in_root` no longer has to look at region outlives leaves at all.
70b6e52 to
da7324e
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. |
|
r? types |
rustbot
added
the
T-types
|
r? me |
| let mut ambiguities = | ||
| constraint.or_constraint.0.iter().map(|and| and.0.iter().find(|c| c.is_ambig())); | ||
| if let Some(Some(ambig)) = ambiguities.next() | ||
| && ambiguities.all(|ambig| ambig.is_some()) |
There was a problem hiding this comment.
I think in theory this all should never actually do anything. No two ands will be the same, which means we'll only ever have one element in the OR which is And(Ambig).
which means we're really just checking for if the or_constraintis Or(And(Ambig)) but if it's that then it should just get moved out into the and_constraint and replaced with Or(And()) when constructing the region constraint 🤔
Does just deleting all of this logic for the or_constraint in this function do what we want? 😅
There was a problem hiding this comment.
Yep, deleting that block works...
I removed the OR logic locally and kept just the and_constraint check.
I added the all check because I wanted mixed ORs to keep their concrete candidates while still propagating ambiguity when every alternative depended on it. What I missed was that new_from_or already pulls shared leaves into and_constraint. So if every alternative contains Ambiguity, the remaining check already catches it, even if those alternatives have other constraints too.
That also means a mixed OR can stay intact until the root assumptions can check the concrete candidate. If that candidate fails, the ambiguous alternative is still there. I really like more this version... and the extra scan was checking something the canonical form already tells us.
I added a case where both OR alternatives become ambiguous to check that we still reject it.
Shared ambiguous leaves already move into the outer AND during canonicalization. Keep the AND check and cover an OR whose alternatives both become ambiguous.
|
I think this PR is now superceded by #162935 |
yeah I think you're right, adwin's PR just removes the reason I did it in the first place was that when one side of an OR was ambiguous we threw the whole OR away, even when the other side was something we could actually prove later at the root. so I only let ambiguity through when every side was ambiguous. getting rid of the whole thing like adwin did is a lot cleaner though, and honestly I like it more than my version the one thing I'd like to keep is the test. from what I can tell the ambiguity it hits (the alias with the non-lifetime binder) is still there after #162935, and adwin mentioned in the description they weren't sure what to do with that case yet, so it seems useful to have something that covers it. the passing cases also need the "empty AND makes the OR true" part from #161988, since the root still treats an OR like an AND of everything in it. so I think it only fully works with both PRs in. I haven't run it on top of adwin's branch yet though, so I might be wrong on this so my plan is to close this one and move the test into #161988, or open a small follow-up once both have landed if that's easier for you to review. does that sound ok? |
|
Opening a small follow up PR seems nice to me :) |
|
closing in favor of #163219 |
rustbot
removed
the
S-waiting-on-review
6 participants
Stacked on #161988. That PR contains the first six commits in this branch, covering reflexive region constraints and the canonicalization needed around them. The only new work here is the final mixed-OR fix. Once #161988 lands, that is all that remains.
propagate_ambiguityused to collapse an entire OR when one alternative was ambiguous. That could throw away a concrete candidate before the root assumptions got a chance to check it. Now ambiguity propagates only when every alternative is ambiguous. A mixed OR keeps its concrete candidates, but still stays ambiguous if none of those candidates can be proved.I kept the dependency visible because these fixes are related, but they are easier to review separately. The regression test covers both OR orders and the AND case where ambiguity is required. I like this split because it keeps the evaluator from turning a candidate that might work into
Ambigjust because another branch is unclear.This is the follow-up Boxy asked to split out in this comment. cc @BoxyUwU