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

generalization is incomplete for aliases #8

Open
lcnr opened this issue Apr 17, 2023 · 0 comments
Open

generalization is incomplete for aliases #8

lcnr opened this issue Apr 17, 2023 · 0 comments
Labels
A-incomplete incorrectly return `NoSolution`, unsound during coherence A-normalization S-unsound

Comments

@lcnr
Copy link
Contributor

lcnr commented Apr 17, 2023

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 emit AliasRelate in this case instead. example

2. ?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#105787

3. ?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 a NormalizesTo 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 mentioning z so if we then equate the substs in AliasRelate we end up pulling down the universe of ?y to 0 as well which would be incomplete. Should instead emit AliasRelate(?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 proving Trait 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 return NoSolution. 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

  • We cannot just return a universe error here as <?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.
  • For reasons listed in previous headers we cannot generalize ?1.0 to for<'a> fn(<?0.0 as Trait<'a, ?3.0>>::Assoc)
  • We can't generalize ?1.0 to fn(?3.0) with an obligation to prove alias-relate(<?0.0 as Trait<'^0_0, ?2.1>>::Assoc, eq, ?3.0).
    • Escaping bound vars bad actually
  • We can't generalize ?1.0 to fn(?4.0) and enter the binder and emit an alias-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 like for<'a> fn(&'a u32)
    • Even if ?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 original fn(?4.0)

Solutions:

  • We could generalize entire type with the binder to an infer var, instead of just the projection, this would result in generalizing for<'a> fn(<?0.0 as Trait<'a>>::Assoc) to ?2.0 and emitting a relate(for<'a> fn(<?0.0 as Trait<'a>>::Assoc, eq, ?2.0) obligation and instantiating ?1.0 with ?2.0.
    • This has the potential to block type inference in undesirable ways that may be too backwards incompatible to justify.
  • Another another potential solution may be to implement HKT(?) so that we can generalize for<'a> fn(?0.0 as Trait<'a>>::Assoc) to for<'a> fn(?2.0<'a>) and instantiate ?1.0 with that, while emitting an obligation for<'a> alias-relate(<?0.0 as Trait<'a>>::Assoc, eq, ?2.0<'a>).
    • This is probably a lot of work and annoying to do just to solve some (probably?) niche unsoundness
  • Make universe errors downgrade to ambiguity instead of NoSolution during coherence as it does not matter if we are incomplete during hir typeck.
    • This will result in coherence not being able to use universe errors as a way of telling that impls do not overlap. This is currently a future compat lint but it triggers on code patterns we would ideally like to accept.
  • Generalize the alias to an AliasKind::Ambiguous in coherence, which will result in ambiguity when attempting to prove goals involving it

There have been various large comments on PRs about soundness of generalization: one, two

@lcnr lcnr added S-unsound A-incomplete incorrectly return `NoSolution`, unsound during coherence A-normalization labels Apr 17, 2023
@lcnr lcnr changed the title generalization is incomplete for projection generalization is incomplete for aliases May 5, 2023
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`
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
A-incomplete incorrectly return `NoSolution`, unsound during coherence A-normalization S-unsound
Projects
None yet
Development

No branches or pull requests

1 participant