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

Errors when using an associated type defined in a supertrait #87

Open
lukenels opened this issue Nov 2, 2022 · 6 comments
Open

Errors when using an associated type defined in a supertrait #87

lukenels opened this issue Nov 2, 2022 · 6 comments

Comments

@lukenels
Copy link

lukenels commented Nov 2, 2022

I'm having some trouble getting auto_impl to work when a trait defines a method that takes an argument of some associated type, where the associated type is defined in a supertrait:

 use auto_impl::auto_impl;

pub trait Foo {
    type MyType;
}

#[auto_impl(Arc)]
pub trait Bar: Foo {
    fn bar(&self, value: Self::MyType);
}

When I try to compile this, I get the following error:

error[E0308]: mismatched types
  --> src/main.rs:11:19
   |
9  | #[auto_impl(Arc)]
   | -----------------
   | |
   | this type parameter
   | arguments to this function are incorrect
10 | pub trait Bar: Foo {
11 |     fn bar(&self, value: Self::MyType);
   |                   ^^^^^ expected type parameter `T`, found struct `Arc`
   |
   = note: expected associated type `<T as Foo>::MyType`
              found associated type `<Arc<T> as Foo>::MyType`
note: associated function defined here
  --> src/main.rs:11:8
   |
11 |     fn bar(&self, value: Self::MyType);
   |        ^^^

For more information about this error, try `rustc --explain E0308`.
error: could not compile `autoimpl-demo` due to previous error

I've tried a few different things (e.g., using <Self as Foo>::MyType instead, putting auto_impl on both traits, etc.) but can't seem to get it to work. Any tips / workarounds? Thanks!


> rustc --version
rustc 1.64.0 (a55dd71d5 2022-09-19)
@KodrAus
Copy link
Member

KodrAus commented May 1, 2023

Hi @lukenels 👋

I imagine you would have gotten to the bottom of this long ago by now 🙂 But I think the problem here is a missing bound on the generated code.

If we expand your example, it looks something like this:

pub trait Foo {
    type MyType;
}

pub trait Bar: Foo {
    fn bar(&self, value: Self::MyType);
}

const _: () = {
    extern crate alloc;
    impl<T: Bar + ?Sized> Bar for alloc::sync::Arc<T>
    where
        alloc::sync::Arc<T>: Foo,
    {
        fn bar(&self, value: Self::MyType) {
            T::bar(self, value)
        }
    }
};

But we can't call T::bar with the type of Arc<T>::MyType, because we don't know what that type actually is. We can add a bound to the generated impl that tells us T::MyType and Arc<T>::MyType are the same:

pub trait Foo {
    type MyType;
}

pub trait Bar: Foo {
    fn bar(&self, value: Self::MyType);
}

const _: () = {
    extern crate alloc;
    
    impl<T: Bar + ?Sized> Bar for alloc::sync::Arc<T>
    where
        alloc::sync::Arc<T>: Foo<MyType = <T as Foo>::MyType>, // <- bound added here
    {
        fn bar(&self, value: Self::MyType) {
            T::bar(self, value)
        }
    }
};

I don't think we'd be able to generate that bound automatically, because we don't have the definition of Foo available to us to know it has associated types on it. I think this is one of those cases where you'd either need to implement the trait manually, or use something more feature-full like impl-tools to do it.

@lukenels
Copy link
Author

lukenels commented May 2, 2023

Hi, KodrAus, thanks for looking into this for me! I ended up just working around the issue by implementing Bar for Arc<T> manually (as you mentioned). But I'm still interested to see if there's a way to tweak the way supertraits are encoded that would get this example to work.

Your explanation of why this doesn't work with the current implementation of auto_impl makes sense—in general the implementation of trait Foo for the types T and Arc<T> need not make the same choice for the associated type Foo::MyType.

One thing I noticed though while playing around with the generated code is that, if trait Foo is also #[auto_impl(Arc)], then the auto_impl for Bar would work if the where clause of the generated implementation bound the base type T rather than Arc<T>.

As a concrete example, if we add #[auto_impl(Arc)] to trait Foo and swap out alloc::sync::Arc<T>: Foo for T: Foo in the generated code, it will succeed to compile as it becomes known to the compiler that Arc<T>::MyType == T::MyType:

use auto_impl::auto_impl;

#[auto_impl(Arc)]
pub trait Foo {
    type MyType;
}

pub trait Bar: Foo {
    fn bar(&self, value: Self::MyType);
}

const _: () = {
    extern crate alloc;

    impl<T: Bar + ?Sized> Bar for alloc::sync::Arc<T>
    where
        T: Foo,
    {
        fn bar(&self, value: Self::MyType) {
            T::bar(self, value)
        }
    }
};

There are some other advantages to this encoding as well. For example, if I don't implement Foo for Arc<T> (using auto_impl or otherwise), then #[auto_impl(Arc)] for Bar will fail to compile immediately rather than the current encoding which produces an implementation that compiles but generally won't be usable due to the Arc<T>: Foo constraint.

But it's hard for me to tell if this encoding is trading something else off (such as the ability to use auto_impl(Arc) for Bar but have a different, custom implementation of Foo for Arc<T>. That seems like it'd be a pretty rare / confusing use case, but could theoretically be possible.

What do you think? Are there other reasons to prefer the current encoding of supertrait constraints that I'm not thinking of, and are there any examples of code where the current encoding works but this possible alternate one wouldn't?

@KodrAus
Copy link
Member

KodrAus commented May 3, 2023

Ah interesting, it's been a while since I've been really immersed in this code so can't tell at a glance whether we'd be swapping one set of failure modes for another, but I think it would be worth trying and seeing what it does with our test suite 🤔

@lukenels
Copy link
Author

lukenels commented May 3, 2023

@KodrAus I've submitted a (draft) pull request implementing the change here: #91. The change (and the update to the unit tests) is pretty simple, but it is a functional change to how auto_impl works so I'm not sure whether it would have broader impact. Do you have a way to build all of the public crates that depend on auto_impl to see if this change would affect them?

@KodrAus
Copy link
Member

KodrAus commented May 3, 2023

Thanks for taking the time to work on this!

Do you have a way to build all of the public crates that depend on auto_impl to see if this change would affect them?

Something like crater, but for ecosystem libraries is something I’ve wanted for a long time 🙂

Doing a quick search on GitHub shows up a bunch of use-cases: https://github.com/search?q=%23%5Bauto_impl&type=code

I’ll have a dig through them and see if I can spot any using supertraits.

Once we’ve got a clear idea of what might break and what is now possible we can decide whether we want to go ahead with this, and whether we should consider it a new feature in a new major version bump, or more like a bug fix.

@lukenels
Copy link
Author

lukenels commented May 4, 2023

Awesome! Let me know what you end up finding out and then I can un-draft the PR if you still think it's the right approach.

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

No branches or pull requests

2 participants