Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't emit alias-relate goal if projections are structurally equal after resolving vars #109617

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 25, 2023

Before emitting an alias-relate goal in super_combine_tys, first check that the projections are not equal after calling resolve_vars_if_possible.

This is because we don't eagerly resolve infer vars in the solver, so we may equate <?0 as Trait>::Projection == <Rigid as Trait>::Projection even though we already know that ?0 = Rigid, and end up spitting out trivial alias-relate goals that become non-trivial after canonicalization (since each var is canonicalized separately).

This is mostly problematic when Rigid mentioned above has region vars (or type vars, for that matter, but I couldn't think of a test exercising this that doesn't already end up with ambiguity), because we canonicalize all region variables separately, and alias-relate goals have 3 separate branches that can be used to satisfy it, and these branches may all be true but return different sets of region constraints.

We can fix this by resolving vars before checking if types are equal, and only emitting an alias-relate goal if the projections are not exactly equal.

A more detailed explanation motivated by a failing test is below.

r? @lcnr


See the UI test. For brevity, Filter is std::iter::Filter and Iter is std::slice::Iter.

  • We evaluate Filter<Iter<'?0r, Attr>, for<'a, 'b> fn(&'a &'b Attr) -> bool> normalizes-to &'?1r Attr.
  • To do this, we set up the normalizes-to hack, which gives us Filter<Iter<'?0r, Attr>, for<'a, 'b> fn(&'a &'b Attr) -> bool> normalizes-to ?2t. Canonicalize that, etc. Not super relevant.
  • We then proceed to assemble candidates for the projection goal. Only one candidate matches here -- the Iterator for Filter impl in std::iter:
    • Our trait ref is Filter<Iter<'?0r, Attr>, for<'a, 'b> fn(&'a &'b Attr) -> bool>: Iterator, and we instantiate a fresh impl trait ref: Filter<?3t, ?4t>: Iterator
    • Equate them, this sets ?3t = Iter<'?0r, Attr> and ?4t = for<'a, 'b> fn(&'a &'b Attr) -> bool.
    • Instantiate the projection term. Notably, we do not resolve vars in the impl substs before doing this (since we don't really resolve vars in candidate assembly1).
    • The term from the impl ends up being <?3t as Iterator>::Item.
    • We equate this term with the unconstrained normalizes-to hack RHS of the projection, i.e. do ?1t = <?3t as Iterator>::Item.
      • Because we're equating a type var against a type, we enter into CombineFields::instantiate.
      • We generalize <?3t as Iterator>::Item. This is not really relevant to the bug here -- generalization doesn't actually do anything meaningful because ?1t can name the region '?0r, but...
      • More importantly, since the generalizer resolves vars as it folds the type, this is basically equivalent to doing a resolve-vars-if-possible call on the type, so we get back the type we passed in, but with ?3t resolved to Iter<'?0r, Attr>.
      • We then equate the "generalized" type (just the type, but with its vars resolved) with the type we passed in, <Iter<'?0r, Attr> as Iterator>::Item and <?3t as Iterator>::Item respectively. This ends up emitting an alias-relate goal, even though the resolved LHS is structurally equal to the RHS after resolving vars...
      • This is because this short-circuit in Equate never gets triggered:
        fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> {
        if a == b {
        return Ok(a);
        }
        since the types are not equal (at least from what we can see here...).
      • This ends up getting canonicalized to <Iter<'^0r, Attr> as Iterator>::Item == <Iter<'^1r, Attr> as Iterator>::Item, which ends up being ambiguous because the different alias-relate branches (RHS normalizes-to LHS, LHS normalizes-to RHS, and LHS equate-substs RHS) end up having different region constraints... :/
    • The rest of the normalizes-to doesn't matter because of the ambiguity above.

Perhaps if we processed region constraints before canonicalization (or compressed and "normalized" them in some way), that canonicalized alias-relate goal <Iter<'^0r, Attr> as Iterator>::Item == <Iter<'^1r, Attr> as Iterator>::Item may not end up being ambiguous, but for now it does.

Footnotes

  1. And this shouldn't matter for correctness in general, since all we're doing is equating things and evaluating nested goals.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Mar 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 25, 2023

Some changes occurred to the core trait solver

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the dont-alias-eq-if-types-r-equal branch from 846aa78 to 18d4b68 Compare March 25, 2023 23:20
@compiler-errors compiler-errors changed the title Don't emit alias-eq goal if projections are structurally equal after resolving vars Don't emit alias-relate goal if projections are structurally equal after resolving vars Mar 26, 2023
@BoxyUwU BoxyUwU self-assigned this Mar 27, 2023
@bors
Copy link
Contributor

bors commented Mar 29, 2023

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

@lcnr
Copy link
Contributor

lcnr commented Mar 31, 2023

i think that our goal was to instead stop uniquifying regions during canonicalization? 🤔 still want to write an explanatory doc for that

@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2023
@compiler-errors compiler-errors deleted the dont-alias-eq-if-types-r-equal branch August 11, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants