-
Notifications
You must be signed in to change notification settings - Fork 0
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
generalization is incomplete for aliases #8
Labels
Comments
lcnr
added
S-unsound
A-incomplete
incorrectly return `NoSolution`, unsound during coherence
A-normalization
labels
Apr 17, 2023
lcnr
changed the title
generalization is incomplete for projection
generalization is incomplete for aliases
May 5, 2023
20 tasks
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Oct 26, 2023
generalize: handle occurs check failure in aliases mostly fixes rust-lang#105787, except for the `for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc) eq ?x` case in rust-lang/trait-system-refactor-initiative#8. r? `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Dec 4, 2023
…rors generalize: handle occurs check failure in aliases mostly fixes rust-lang#105787, except for the `for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc) eq ?x` case in rust-lang/trait-system-refactor-initiative#8. r? `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Dec 5, 2023
…rors generalize: handle occurs check failure in aliases mostly fixes rust-lang#105787, except for the `for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc) eq ?x` case in rust-lang/trait-system-refactor-initiative#8. r? `@compiler-errors`
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this issue
Dec 5, 2023
generalize: handle occurs check failure in aliases mostly fixes #105787, except for the `for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc) eq ?x` case in rust-lang/trait-system-refactor-initiative#8. r? `@compiler-errors`
This was referenced Jan 26, 2024
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Feb 6, 2024
modify alias-relate to also normalize ambiguous opaques allows a bunch of further cleanups and generally simplifies the type system. To handle rust-lang/trait-system-refactor-initiative#8 we'll have to add a some additional complexity to the `(Alias, Infer)` branches in alias-relate, so removing the opaque type special case here is really valuable. It does worsen `deduce_closure_signature` and friends even more as they now receive an inference variable which is only constrained via an `AliasRelate` goal. These probably have to look into alias relate goals somehow. Leaving that for a future PR as this is something we'll have to tackle regardless. r? `@compiler-errors`
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Feb 8, 2024
modify alias-relate to also normalize ambiguous opaques allows a bunch of further cleanups and generally simplifies the type system. To handle rust-lang/trait-system-refactor-initiative#8 we'll have to add a some additional complexity to the `(Alias, Infer)` branches in alias-relate, so removing the opaque type special case here is really valuable. It does worsen `deduce_closure_signature` and friends even more as they now receive an inference variable which is only constrained via an `AliasRelate` goal. These probably have to look into alias relate goals somehow. Leaving that for a future PR as this is something we'll have to tackle regardless. r? `@compiler-errors`
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Feb 8, 2024
modify alias-relate to also normalize ambiguous opaques allows a bunch of further cleanups and generally simplifies the type system. To handle rust-lang/trait-system-refactor-initiative#8 we'll have to add a some additional complexity to the `(Alias, Infer)` branches in alias-relate, so removing the opaque type special case here is really valuable. It does worsen `deduce_closure_signature` and friends even more as they now receive an inference variable which is only constrained via an `AliasRelate` goal. These probably have to look into alias relate goals somehow. Leaving that for a future PR as this is something we'll have to tackle regardless. r? ``@compiler-errors``
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Feb 13, 2024
modify alias-relate to also normalize ambiguous opaques allows a bunch of further cleanups and generally simplifies the type system. To handle rust-lang/trait-system-refactor-initiative#8 we'll have to add a some additional complexity to the `(Alias, Infer)` branches in alias-relate, so removing the opaque type special case here is really valuable. It does worsen `deduce_closure_signature` and friends even more as they now receive an inference variable which is only constrained via an `AliasRelate` goal. These probably have to look into alias relate goals somehow. Leaving that for a future PR as this is something we'll have to tackle regardless. r? `@compiler-errors`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Feb 13, 2024
Rollup merge of rust-lang#120549 - lcnr:errs-showcase, r=compiler-errors modify alias-relate to also normalize ambiguous opaques allows a bunch of further cleanups and generally simplifies the type system. To handle rust-lang/trait-system-refactor-initiative#8 we'll have to add a some additional complexity to the `(Alias, Infer)` branches in alias-relate, so removing the opaque type special case here is really valuable. It does worsen `deduce_closure_signature` and friends even more as they now receive an inference variable which is only constrained via an `AliasRelate` goal. These probably have to look into alias relate goals somehow. Leaving that for a future PR as this is something we'll have to tackle regardless. r? `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Feb 26, 2024
avoid generalization inside of aliases The basic idea of this PR is that we don't generalize aliases when the instantiation could fail later on, either due to the *occurs check* or because of a universe error. We instead replace the whole alias with an inference variable and emit a nested `AliasRelate` goal. This `AliasRelate` then fully normalizes the alias before equating it with the inference variable, at which point the alias can be treated like any other rigid type. We now treat aliases differently depending on whether they are *rigid* or not. To detect whether an alias is rigid we check whether `NormalizesTo` fails. While we already do so inside of `AliasRelate` anyways, also doing so when instantiating a query response would be both ugly/difficult and likely inefficient. To avoid that I change `instantiate_and_apply_query_response` to relate types completely structurally. This change generally removes a lot of annoying complexity, which is nice. It's implemented by adding a flag to `Equate` to change it to structurally handle aliases. We currently always apply constraints from canonical queries right away. By providing all the necessary information to the canonical query, we can guarantee that instantiating the query response never fails, which further simplifies the implementation. This does add the invariant that *any information which could cause instantiating type variables to fail must also be available inside of the query*. While it's acceptable for canonicalization to result in more ambiguity, we must not cause the solver to incompletely structurally relate aliases by erasing information. This means we have to be careful when merging universes during canonicalization. As we only generalize for type and const variables we have to make sure that anything nameable by such a type or const variable inside of the canonical query is also nameable outside of it. Because of this we both stop merging universes of existential variables when canonicalizing inputs, we put all uniquified regions into a higher universe which is not nameable by any type or const variable. I will look into always replacing aliases with inference variables when generalizing in a later PR unless the alias references bound variables. This should both pretty much fix rust-lang/trait-system-refactor-initiative#4. This may allow us to merge the universes of existential variables again by changing generalize to not consider their universe when deciding whether to generalize aliases. This requires some additional non-trivial changes to alias-relate, so I am leaving that as future work. Fixes rust-lang/trait-system-refactor-initiative#79. While it would be nice to decrement universe indices when existing a `forall`, that was surprisingly difficult and not necessary to fix this issue. I am really happy with the approach in this PR think it is the correct way forward to also fix the remaining cases of rust-lang/trait-system-refactor-initiative#8.
lnicola
pushed a commit
to lnicola/rust-analyzer
that referenced
this issue
Apr 7, 2024
generalize: handle occurs check failure in aliases mostly fixes #105787, except for the `for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc) eq ?x` case in rust-lang/trait-system-refactor-initiative#8. r? `@compiler-errors`
RalfJung
pushed a commit
to RalfJung/rust-analyzer
that referenced
this issue
Apr 27, 2024
generalize: handle occurs check failure in aliases mostly fixes #105787, except for the `for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc) eq ?x` case in rust-lang/trait-system-refactor-initiative#8. r? `@compiler-errors`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
when generalizing a projection we have to be careful. We're using
?ident.universe
for inference variables in a given universe.given that this is already an issue in on stable we could add a test for this and stabilize even with this incompleteness.
1.
?x
sub<() as Trait<?y>>::Assoc
must not generalize
?x
to<() as Trait<?y>>::Assoc
as?x
may get inferred to a subtype of the normalized projection. Otherwise this would be incomplete. Should emitAliasRelate
in this case instead. example2.
?x
eq<?x as Trait>::Assoc
✔️this must not result in a universe error as the alias may be normalized to
?x
. rust-lang/rust#1057873.
?x
eq<<T as Id<?x>>::Id as Unnormalizable>::Assoc
✔️This may hold, as
?x
can be constrained to<T as Unnormalizable>::Assoc
: example. We cannot simply emit aNormalizesTo
goal if equating fails the occurs check, as we could also normalize a nested projection instead.4.
?x.0
eq<() as Trait<?y.1>>::Assoc
✔️This must not generalize
?x.0
to<() as Trait<?z.1>>::Assoc
because that should result in a universe error but it must also not relate it to<() as Trait<?z.0>>::Assoc
because the projection may not actually normalize to a type mentioningz
so if we then equate the substs inAliasRelate
we end up pulling down the universe of?y
to0
as well which would be incomplete. Should instead emitAliasRelate(?x.0, <() as Trait<?y.1>>::Assoc, Equate)
.5.
<?x.1 as Trait>::Assoc
eq?y.0
✔️Roughly the same setup as the previous issue but with a different potential unsoundness.
If we generalize
?y.0
to<?z.0 as Trait<'a>>::Assoc
(the self ty ending up in a lower universe than?x.1
) this is potentially incomplete if we have a param env candidate such as!T.1: Trait
and no other candidates for provingTrait
as we would consider ourselves unable to normalize<?z.0 as Trait>::Assoc
as?z.0 eq !T.1
would give a universe error and we would returnNoSolution
. This is wrong if the assoc type does not actually mention the placeholder as we would be able to instantiate?y.0
with the assoc type's normalized form.This could likely only occur with
non_lifetime_binders
that support where clauses and it would also likely have to be slighltly more complex of a reproduction to workaround the fact that?0: Trait
are always ambiguous.6.
for<'a> fn(<<?x as OtherTrait>::Assoc as Trait<'a>>::Assoc)
eq?x
TODO: explain why this is an issue
7.
for<'a> fn(<?0.0 as Trait<'a, ?2.1>>::Assoc)
eq?1.0
<?0.0 as Trait<'a, ?2.1>>::Assoc
may normalize to something that does not name the placeholder in a higher universe than?1.0
. This would then be unsound in coherence as we are erroring when there is nothing actually wrong.?1.0
tofor<'a> fn(<?0.0 as Trait<'a, ?3.0>>::Assoc)
?1.0
tofn(?3.0)
with an obligation to provealias-relate(<?0.0 as Trait<'^0_0, ?2.1>>::Assoc, eq, ?3.0)
.?1.0
tofn(?4.0)
and enter the binder and emit analias-relate(<?0.0 as Trait<!3.2, ?2.1>>::Assoc, eq, ?4.0)
.?4.0
could not be instantiated with something containing the!3.2
placeholder even though we would like to end up with?1.0
potentially being able to be resolved to something likefor<'a> fn(&'a u32)
?4.0
could somehow be instantiated with something containing!3.2
we would have no way of converting that placeholder back into a bound var for the originalfn(?4.0)
Solutions:
for<'a> fn(<?0.0 as Trait<'a>>::Assoc)
to?2.0
and emitting arelate(for<'a> fn(<?0.0 as Trait<'a>>::Assoc, eq, ?2.0)
obligation and instantiating?1.0
with?2.0
.for<'a> fn(?0.0 as Trait<'a>>::Assoc)
tofor<'a> fn(?2.0<'a>)
and instantiate?1.0
with that, while emitting an obligationfor<'a> alias-relate(<?0.0 as Trait<'a>>::Assoc, eq, ?2.0<'a>)
.NoSolution
during coherence as it does not matter if we are incomplete during hir typeck.AliasKind::Ambiguous
in coherence, which will result in ambiguity when attempting to prove goals involving itThere have been various large comments on PRs about soundness of generalization: one, two
The text was updated successfully, but these errors were encountered: