Skip to content

builtin object candidates: ReplaceProjectionWith relate failures #171

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

Open
lcnr opened this issue Apr 10, 2025 · 2 comments · May be fixed by rust-lang/rust#139774
Open

builtin object candidates: ReplaceProjectionWith relate failures #171

lcnr opened this issue Apr 10, 2025 · 2 comments · May be fixed by rust-lang/rust#139774
Assignees

Comments

@lcnr
Copy link
Contributor

lcnr commented Apr 10, 2025

encountered during try builds when compiling wasmparser.

pub trait Trait: Super {}
pub trait Super {
    type Output;
}

fn bound<T: Trait + ?Sized>() {}
fn visit_simd_operator<V: Super + ?Sized>() {
    bound::<dyn Trait<Output = <V as Super>::Output>>();
}

compiles on stable, results in the following ICE with the new solver

thread 'rustc' panicked at /home/lcnr/rust/compiler/rustc_next_trait_solver/src/solve/assembly/structural_traits.rs:930:26:
expected to be able to unify goal projection with dyn's projection: NoSolution
@compiler-errors
Copy link
Member

compiler-errors commented Apr 10, 2025

Above example shouldn't ICE if we don't walk into the dyn Trait type itself (which we shouldn't need to, of course, but we do b/c of the way the folder is set up today), but that's insufficient b/c this still ICEs:

trait Other<X> {}

trait Foo<T: Foo<T>>: Other<<T as Foo<T>>::Assoc> {
    type Assoc;
}

impl<T> Foo<T> for T {
    type Assoc = ();
}

impl<T: ?Sized> Other<()> for T {}

fn is_foo<T: Foo<()> + ?Sized>() {}

fn main() {
    is_foo::<dyn Foo<(), Assoc = ()>>();
}

It should be sufficient just to check that the self type of the projection to be replaced is Self (the trait object itself) b/c any other recursive mentions of <Self as Trait>::Assoc should be disallowed by the logic in rust-lang/rust#126090.

@compiler-errors
Copy link
Member

compiler-errors commented Apr 14, 2025

A more interesting example that shows that we can't just use structural relation:

trait Other<T> {}
impl<T> Other<T> for T {}

trait Super<T> {
    type Assoc;
}

trait Mirror {
    type Assoc;
}
impl<T> Mirror for T {
    type Assoc = T;
}

trait Foo<A, B>: Super<<A as Mirror>::Assoc, Assoc = A> {
    type FooAssoc: Other<<Self as Super<<A as Mirror>::Assoc>>::Assoc>;
}

fn is_foo<T, U>(x: &(impl Foo<T, U> + ?Sized)) {}

fn test(x: &dyn Foo<i32, u32, FooAssoc = i32>) {
    is_foo::<i32, u32>(x);
}

fn main() {}

b/c we'll need to emit <i32 as Mirror>::Assoc == i32 goals when relating the alias w/ its replacement.

@compiler-errors compiler-errors self-assigned this Apr 14, 2025
@lcnr lcnr moved this from unknown to in progress in -Znext-solver=globally Apr 16, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 17, 2025
…r=lcnr

Fix replacing supertrait aliases in `ReplaceProjectionWith`

The new solver has a procedure called `predicates_for_object_candidate`, which elaborates the super-bounds and item-bounds that are required to hold for a dyn trait to implement something via a built-in object impl.

In that procedure, there is a folder called `ReplaceProjectionWith` which is responsible for replacing projections that reference `Self`, so that we don't encounter cycles when we then go on to normalize those projections in the process of proving these super-bounds.

That folder had a few problems: Firstly, it wasn't actually checking that this was a super bound originating from `Self`. Secondly, it only accounted for a *single* projection type def id, but trait objects can have multiple (i.e. `trait Foo<A, B>: Bar<A, Assoc = A> + Bar<B, Assoc = B>`).

To fix the first, it's simple enough to just add an equality check for the self ty. To fix the second, I implemented a matching step that's very similar to the `projection_may_match` check we have for upcasting, since on top of having multiple choices, we need to deal with both non-structural matches and ambiguity.

This probably lacks a bit of documentation, but I think it works pretty well.

Fixes rust-lang/trait-system-refactor-initiative#171

r? lcnr
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 17, 2025
…r=lcnr

Fix replacing supertrait aliases in `ReplaceProjectionWith`

The new solver has a procedure called `predicates_for_object_candidate`, which elaborates the super-bounds and item-bounds that are required to hold for a dyn trait to implement something via a built-in object impl.

In that procedure, there is a folder called `ReplaceProjectionWith` which is responsible for replacing projections that reference `Self`, so that we don't encounter cycles when we then go on to normalize those projections in the process of proving these super-bounds.

That folder had a few problems: Firstly, it wasn't actually checking that this was a super bound originating from `Self`. Secondly, it only accounted for a *single* projection type def id, but trait objects can have multiple (i.e. `trait Foo<A, B>: Bar<A, Assoc = A> + Bar<B, Assoc = B>`).

To fix the first, it's simple enough to just add an equality check for the self ty. To fix the second, I implemented a matching step that's very similar to the `projection_may_match` check we have for upcasting, since on top of having multiple choices, we need to deal with both non-structural matches and ambiguity.

This probably lacks a bit of documentation, but I think it works pretty well.

Fixes rust-lang/trait-system-refactor-initiative#171

r? lcnr
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 17, 2025
…r=lcnr

Fix replacing supertrait aliases in `ReplaceProjectionWith`

The new solver has a procedure called `predicates_for_object_candidate`, which elaborates the super-bounds and item-bounds that are required to hold for a dyn trait to implement something via a built-in object impl.

In that procedure, there is a folder called `ReplaceProjectionWith` which is responsible for replacing projections that reference `Self`, so that we don't encounter cycles when we then go on to normalize those projections in the process of proving these super-bounds.

That folder had a few problems: Firstly, it wasn't actually checking that this was a super bound originating from `Self`. Secondly, it only accounted for a *single* projection type def id, but trait objects can have multiple (i.e. `trait Foo<A, B>: Bar<A, Assoc = A> + Bar<B, Assoc = B>`).

To fix the first, it's simple enough to just add an equality check for the self ty. To fix the second, I implemented a matching step that's very similar to the `projection_may_match` check we have for upcasting, since on top of having multiple choices, we need to deal with both non-structural matches and ambiguity.

This probably lacks a bit of documentation, but I think it works pretty well.

Fixes rust-lang/trait-system-refactor-initiative#171

r? lcnr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: in progress
Development

Successfully merging a pull request may close this issue.

2 participants