Errors when using an associated type defined in a supertrait about auto_impl HOT 6 OPEN

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.

from auto_impl.

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?

from auto_impl.

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 🤔

from auto_impl.

@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?

from auto_impl.

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.

from auto_impl.

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.

from auto_impl.