trait_solver: Preserve concrete candidates in ambiguous ORs - #162442
trait_solver: Preserve concrete candidates in ambiguous ORs#162442Dnreikronos 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.
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