Unable to use auto_impl with async-trait about auto_impl HOT 17 CLOSED

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. 👍

from auto_impl.

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

from auto_impl.

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;
}

from auto_impl.

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.

from auto_impl.

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

from auto_impl.

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!

from auto_impl.

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

from auto_impl.

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

from auto_impl.

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`.

from auto_impl.

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

from auto_impl.

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?

from auto_impl.

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.

from auto_impl.

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.

from auto_impl.

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!

from auto_impl.

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

from auto_impl.

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?

from auto_impl.

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

from auto_impl.