Unable to use auto_impl with async-trait

[ebkalderon]

I was trying to use auto_impl to delegate the implementation of Box<T> where T: Trait and Trait was a trait annotated with the async-trait procedural macro. I was using it like this:

#[async_trait]
#[auto_impl(Box)]
trait Foo: Sync {
    async fn bar(&self) -> Result<u32, Error>;
}

The ordering of the above macros matters, as it ensures that async_trait transforms Foo into the following Rust code before auto_impl parses it:

#[auto_impl(Box)]
trait Foo: Sync {
    fn bar<'life0, 'async_trait>(
        &'life0 self
    ) -> Pin<Box<dyn Future<Output = Result<u32, Error>> + Send + 'async_trait>> where
        'life0: 'async_trait,
        Self: 'async_trait;
}

Provided that the Sync bound shown above is present as a supertrait of Foo, the above code should be object safe. I expected #[auto_impl(Box)] to emit the following implementation:

impl<T: Foo + ?Sized> Foo for Box<T>
where
    Box<T>: Sync

However, it emitted the following implementation instead:

impl<T: Foo> Foo for Box<T>
where
    Box<T>: Sync

This meant that the following code was able to compile with a manual Box implementation but failed under the auto_impl-generated implementation:

fn assert_impl<T: Foo>() {}

fn foo() {
    assert_impl::<Box<dyn Foo>>(); // ERROR: Not object safe with `auto_impl`
}

Do you have an idea as to why the ?Sized bound was not applied in this case?

See ebkalderon/tower-lsp#109 for the original pull request that inspired this issue.

[ebkalderon]

I also just had a realization that perhaps the code generation might not work anyway, since async-trait also performs transformations on the method bodies of implementations, not just in the trait definitions.

If you could confirm whether this is even possible to do with auto_impl, I would greatly appreciate it. Otherwise, I wouldn't mind closing ebkalderon/tower-lsp#109 and stick to a manual trait implementation in this case.

[ebkalderon]

I've closed ebkalderon/tower-lsp#109 for now, but I would still like to know if this is possible.

[LukasKalbertodt]

Hi there!

Turns out we just didn't release a new version for quite some time! This PR you linked is not yet released.

I just tested with the current master and both example code snippets you posted (the one with #[async_trait] and the one already expanded) both emit the following impl:

impl<T: Foo + ?::std::marker::Sized + 'async_trait> Foo for ::std::boxed::Box<T>

I guess I can release a new version soon!

[LukasKalbertodt]

@ebkalderon Could you test your crate with our master branch and check if it works for you? Then I can release I think.

[ebkalderon]

Thanks a lot for clarifying, @LukasKalbertodt! Sure, I'll try it out on my local clone right now.

[ebkalderon]

Hmm, I've revised the commit on the use-auto-impl-box branch of tower-lsp to test out master of auto_impl, and it's failing to compile with the following error:

error[E0261]: use of undeclared lifetime name `'async_trait`
  --> src/lib.rs:98:1
   |
98 | #[auto_impl(Box)]
   | ^^^^^^^^^^^^^^^^^ undeclared lifetime

error: aborting due to previous error

For more information about this error, try `rustc --explain E0261`.
error: could not compile `tower-lsp`.

[ebkalderon]

I think the generated code should be corrected from this:

impl<T: Foo + ?::std::marker::Sized + 'async_trait> Foo for ::std::boxed::Box<T>

to this:

impl<'async_trait, T: Foo + ?::std::marker::Sized + 'async_trait> Foo for ::std::boxed::Box<T>

Playground link

[LukasKalbertodt]

Interesting, thanks for checking.

In this case, we can simply remove the + 'async_trait bound from the impl header. But I am not sure if we can always do that! Check #11 for more information.There, my example in the top comments doesn't compile. But if we replace the Self: Clone bound with a 'static bound, it does compile!

trait Foo {
    fn foo(&self)
    where
        Self: 'static;
}

impl<'a, T: 'a + Foo> Foo for &'a T {
    fn foo(&self)
    where
        Self: 'static
    {
        (**self).foo()
    }
}

I don't quite know why it's allowed. First it seemed like this might be a bug and could allow for unsafety. But of course, as always, I couldn't find a way to trigger unsafety. But my reasoning is: inside foo, the method can rely on Self: 'static. But in that one foo implementation, we are not sure that T is actually 'static. So that seems wrong.

It would be great if we can just ignore lifetime constraints on Self. That would make it easy to make your example compile. The change wouldn't be fairly easy too. However, I would like to understand why this works before changing anything. Does anyone understand this?

[ebkalderon]

Well, if you check the desugared version of LanguageServer (expand the trait definition in Docs.rs to see it), you will see that 'async_trait is a lifetime that is not declared on the trait itself, but is rather declared on each method individually, similar to Foo::bar() as seen in #65 (comment).

It doesn't make sense to me to take that method-bound lifetime and apply it to all of T.

[LukasKalbertodt]

Yeah it's true, the Self bounds are on the methods. I initially thought it was not necessary to put those bounds into the impl header, but check #11 again. The Clone bound in the impl header is simply necessary. Now I'm just trying to find out why the same restriction does not apply to lifetimes.

[ebkalderon]

I don't believe the two cases are directly comparable, despite sharing similar syntax. I think this might be another case of input and output lifetime elision being made explicit, but I'm not as familiar with the deeper intricacies of the elision rules to know for certain. For example, I wonder Foo::foo1() and Foo::foo2() are considered equivalent in this case:

trait Foo {
    fn foo1<'a, 'b: 'a>(&'b self) -> &'a str;

    fn foo2<'a, 'b>(&'b self) -> &'a str
    where
        'b: 'a,
        Self: 'b;
}

Hopefully someone more knowledgeable than me can answer this question!

[LukasKalbertodt]

I'm currently pretty busy, but I'll try to figure this out and fix this by the end of the month.

[ebkalderon]

No worries, @LukasKalbertodt. Take your time. Thanks for creating this awesome crate, by the way!

[LukasKalbertodt]

I finally managed to take a closer look. I discovered the bug in my thoughts (i.e. why I was confused).

In short: if we access a field (or something like that) of self, we cannot assume self.field implements SomeTrait even if Self: SomeTrait. That's why #11 was a bug that needed fixing. However, self.field always satisfies the same lifetime bounds as Self, or more. In other words: if we have Self: 'a, then self.field always satisfies 'a as well. That's why we don't need to add lifetime bounds on Self specified on methods to the T type in the impl header. I hope that explanation made some sense.

In any case, #67 should fix your problem. At least the following code compiles fine:

#[auto_impl(Box)]
trait Foo: Sync {
    fn bar<'life0, 'async_trait>(
        &'life0 self
    ) -> Pin<Box<dyn Future<Output = Result<u32, Error>> + Send + 'async_trait>> where
        'life0: 'async_trait,
        Self: 'async_trait;
}

[ebkalderon]

Awesome work, @LukasKalbertodt! That's a very subtle change, but it makes perfect sense. I really appreciate you looking into it.

I tried enabling it in tower-lsp by adding #[auto_impl(Box)] underneath the #[async_trait] attribute, but now I'm running into another issue where this static assertion nonetheless fails to compile:

fn _assert_object_safe() {
    fn assert_impl<T: LanguageServer>() {}
    assert_impl::<Box<dyn LanguageServer>>();
}

Error message:

error[E0277]: the size for values of type `(dyn LanguageServer + 'static)` cannot be known at compilation time
   --> src/lib.rs:748:5
    |
747 |     fn assert_impl<T: LanguageServer>() {}
    |                       -------------- required by this bound in `_assert_object_safe::assert_impl`
748 |     assert_impl::<Box<dyn LanguageServer>>();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: the trait `std::marker::Sized` is not implemented for `(dyn LanguageServer + 'static)`
    = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
    = note: required because of the requirements on the impl of `LanguageServer` for `std::boxed::Box<(dyn LanguageServer + 'static)>

This seems to be because auto_impl(Box) does not include a T: ?Sized bound on T, which my current hand-written implementation does. I was under impression the changes in #52 were supposed to handle this.

Any idea why the ?Sized bound isn't being added here?

[LukasKalbertodt]

@ebkalderon Are you sure you are not testing with an incorrect version of auto_impl? I'm the branch of #67 and this code compiles fine without errors:

#[auto_impl(Box)]
trait Foo: Sync {
    fn bar<'life0, 'async_trait>(
        &'life0 self
    ) -> Pin<Box<dyn Future<Output = Result<u32, Error>> + Send + 'async_trait>> where
        'life0: 'async_trait,
        Self: 'async_trait;
}

fn _assert_object_safe() {
    fn assert_impl<T: Foo>() {}
    assert_impl::<Box<dyn Foo>>();
}

And cargo expand also shows a ?Sized relaxation.

[ebkalderon]

Sorry for the delayed response, @LukasKalbertodt! Indeed I was. I had checked out an incorrect commit ref and didn't get the changes. It seems to work just fine now. 👍