Skip to content

AliasBound + ParamEnv candidate eq modulo regions #90

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

Closed
lcnr opened this issue Feb 8, 2024 · 8 comments
Closed

AliasBound + ParamEnv candidate eq modulo regions #90

lcnr opened this issue Feb 8, 2024 · 8 comments
Labels
blocks-crater Blocks running a crater run for breakage w/ new solver

Comments

@lcnr
Copy link
Contributor

lcnr commented Feb 8, 2024

fixed by preferring where-bounds over alias-bounds

pub trait QueryDb<'d>: Sized {
    type DynDb: HasQueryGroup<Self::Group>;

    type Group: QueryGroup;
}

pub trait QueryGroup: Sized {
    type DynDb: HasQueryGroup<Self>;
}

pub trait HasQueryGroup<G>
where
    G: QueryGroup,
{
}

pub trait EqualDynDb<'d, IQ: QueryDb<'d>>: QueryDb<'d> {}

impl<'d, IQ, Q> EqualDynDb<'d, IQ> for Q
where
    Q: QueryDb<'d, DynDb = IQ::DynDb, Group = IQ::Group>,
    Q::DynDb: HasQueryGroup<IQ::Group>,
    IQ: QueryDb<'d>,
{
}

fn main() {}

results in

error[E0283]: type annotations needed: cannot satisfy `Q: EqualDynDb<'d, IQ>`
  --> src/main.rs:28:40
   |
28 | impl<'d, IQ, Q> EqualDynDb<'d, IQ> for Q
   |                                        ^
   |
   = note: cannot satisfy `Q: EqualDynDb<'d, IQ>`
   = help: the trait `EqualDynDb<'_, IQ>` is implemented for `Q`

error: aborting due to 1 previous error

minimized from salsa

feels related to #89 but also affects nightly

@lcnr

This comment was marked as outdated.

@lcnr lcnr changed the title impl_wf_check fails AliasBound Mar 8, 2024
@lcnr lcnr changed the title AliasBound AliasBound + ParamEnv candidate eq modulo regions Mar 8, 2024
@lcnr
Copy link
Contributor Author

lcnr commented Mar 8, 2024

minimized

trait Bound<'a> {}
trait Trait<'a> {
    type Assoc: Bound<'a>;
}

fn foo<'a, T>()
where
    T: Trait<'a>,
    <T as Trait<'a>>::Assoc: Bound<'a>,
{}

checking that foo is wf tries to prove exists<'0, '1> <T as Trait<'0>>::Assoc: Bound<'1> after uniquification. The AliasBound candidate ends up requiring '0 == '1 while the where bound candidate equates '0 and '1 with the lts from the <T as Trait<'a>>::Assoc: Bound<'a>, where-bound, which are also uniquified to different lifetimes: <T as Trait<'2>>::Assoc: Bound<'3>

Thanks to @BoxyUwU for actually figuring this out ❤️

@lcnr
Copy link
Contributor Author

lcnr commented Mar 8, 2024

notes:

We first considered that this should be solvable by not uniquifying regions in the ParamEnv, only doing so for the goal. This does not solve this issue, as inside of the query we'd still have a where bound <T as Trait<'env_a>>::Assoc: Bound<'env_a> for a goal <T as Trait<'0>>::Assoc: Bound<'1>. So both the AliasBound and the ParamEnv goal still result in non-trivial region constraints.

We already dedup equal clauses in the param env, which is why T: Trait<'a> + Trait<'a> does not result in ambiguity and neither does T: Trait<'a> + SuperTrait<'a>. We could therefore use structural equality checks to throw out where-bounds when constructing the param-env if they are equal to alias-bounds of their self-type. This must only be done if the self-type is a rigid alias. It does add some complexity.

Alternatively: open a PR to salsa removing the trivial-bound 😁 and check whether this pattern is more widespread

@lcnr
Copy link
Contributor Author

lcnr commented Mar 8, 2024

further thought: this pattern is likely also possible when checking that the impl item where-bounds are implied by the instantiated trait item where-bounds. should get a test for that. In this case it is not possible to simply remove the trivial where-bound

@BoxyUwU BoxyUwU self-assigned this Mar 12, 2024
@lcnr
Copy link
Contributor Author

lcnr commented Mar 14, 2024

from @BoxyUwU this affects compute_predicate_entailment as well https://rust.godbolt.org/z/En4s6W1qd

@BoxyUwU BoxyUwU removed their assignment Mar 30, 2024
@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 30, 2024

We should make sure we write a test that alias bounds from a nested alias as the self type also are factored into this (i.e. decide whether we want to filter those out too or not and then have a test that will fail if we change that behaviour). Something like <<T as OtherRigid<'a>>::OtherAssoc> as Rigid>::Assoc: Bound<'a> where OtherRigid is defined with a type OtherAssoc: Rigid<Assoc: Bound<'a>>

@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 30, 2024

Another noteworthy thing is that the solver doesnt assemble alias bounds for non rigid aliases so if we have a where clause like <T as Alias<'a>>::Assoc: Bound<'a> where the alias is normalizeable and Assoc has an item bound of Bound<'a> then filtering that where clause out is removing a bound that is not actually something that already gets assembled. The normalized form ought to just be able to prove the Bound<'a> on anyway as we would have had to have done that in an env with where clauses that all hold if <T as Alias<'a>>::Assoc is wf.

  • It is unclear to me whether we want the param_env query (or wherever we do this filtering) to be figuring out whether aliases in the env are rigid or not as this would require actually running solver logic with the unfiltered env which means that we do not always run the solver with these bounds removed
  • It probably is possible to write an example where the explicit bound on the non rigid alias is not able to be proved on the normalized form due to incompleteness(?).

@lcnr
Copy link
Contributor Author

lcnr commented Apr 25, 2024

This impacts core:

trait Pattern<'a> {
    type Searcher: Searcher<'a>;
}

trait Searcher<'a> {}

fn rsplit_terminator<'a, P>()
where
    P: Pattern<'a>,
    P::Searcher: Searcher<'a>,
{
}

@compiler-errors compiler-errors added the blocks-crater Blocks running a crater run for breakage w/ new solver label Apr 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 25, 2024
…r=<try>

`obligations_for_self_ty`: use `ProofTreeVisitor` for nested goals

As always, dealing with proof trees continues to be a hacked together mess. After this PR and rust-lang#124380 the only remaining blocker for core is rust-lang/trait-system-refactor-initiative#90. There is also a `ProofTreeVisitor` issue causing an ICE when compiling `alloc` which I will handle in a separate PR. This issue likely affects coherence diagnostics more generally.

The core idea is to extend the proof tree visitor to support visiting nested candidates without using a `probe`. We then simply recurse into nested candidates if they are the only potentially applicable candidate for a given goal and check whether the self type matches the expected one.

For that to work, we need to improve `CanonicalState` to also handle unconstrained inference variables created inside of the trait solver. This is done by extending the `var_values` of `CanoncalState` with each fresh inference variables. Furthermore, we also store the state of all inference variables at the end of each probe. When recursing into `InspectCandidates` we then unify the values of all these states.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 26, 2024
…r=<try>

`obligations_for_self_ty`: use `ProofTreeVisitor` for nested goals

As always, dealing with proof trees continues to be a hacked together mess. After this PR and rust-lang#124380 the only remaining blocker for core is rust-lang/trait-system-refactor-initiative#90. There is also a `ProofTreeVisitor` issue causing an ICE when compiling `alloc` which I will handle in a separate PR. This issue likely affects coherence diagnostics more generally.

The core idea is to extend the proof tree visitor to support visiting nested candidates without using a `probe`. We then simply recurse into nested candidates if they are the only potentially applicable candidate for a given goal and check whether the self type matches the expected one.

For that to work, we need to improve `CanonicalState` to also handle unconstrained inference variables created inside of the trait solver. This is done by extending the `var_values` of `CanoncalState` with each fresh inference variables. Furthermore, we also store the state of all inference variables at the end of each probe. When recursing into `InspectCandidates` we then unify the values of all these states.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 26, 2024
…r=compiler-errors

`obligations_for_self_ty`: use `ProofTreeVisitor` for nested goals

As always, dealing with proof trees continues to be a hacked together mess. After this PR and rust-lang#124380 the only remaining blocker for core is rust-lang/trait-system-refactor-initiative#90. There is also a `ProofTreeVisitor` issue causing an ICE when compiling `alloc` which I will handle in a separate PR. This issue likely affects coherence diagnostics more generally.

The core idea is to extend the proof tree visitor to support visiting nested candidates without using a `probe`. We then simply recurse into nested candidates if they are the only potentially applicable candidate for a given goal and check whether the self type matches the expected one.

For that to work, we need to improve `CanonicalState` to also handle unconstrained inference variables created inside of the trait solver. This is done by extending the `var_values` of `CanoncalState` with each fresh inference variables. Furthermore, we also store the state of all inference variables at the end of each probe. When recursing into `InspectCandidates` we then unify the values of all these states.

r? `@compiler-errors`
RalfJung pushed a commit to RalfJung/miri that referenced this issue Apr 27, 2024
…r-errors

`obligations_for_self_ty`: use `ProofTreeVisitor` for nested goals

As always, dealing with proof trees continues to be a hacked together mess. After this PR and #124380 the only remaining blocker for core is rust-lang/trait-system-refactor-initiative#90. There is also a `ProofTreeVisitor` issue causing an ICE when compiling `alloc` which I will handle in a separate PR. This issue likely affects coherence diagnostics more generally.

The core idea is to extend the proof tree visitor to support visiting nested candidates without using a `probe`. We then simply recurse into nested candidates if they are the only potentially applicable candidate for a given goal and check whether the self type matches the expected one.

For that to work, we need to improve `CanonicalState` to also handle unconstrained inference variables created inside of the trait solver. This is done by extending the `var_values` of `CanoncalState` with each fresh inference variables. Furthermore, we also store the state of all inference variables at the end of each probe. When recursing into `InspectCandidates` we then unify the values of all these states.

r? `@compiler-errors`
@lcnr lcnr closed this as completed Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocks-crater Blocks running a crater run for breakage w/ new solver
Projects
None yet
Development

No branches or pull requests

3 participants