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

Use auto_impl to delegate Box implementation #109

Closed
wants to merge 1 commit into from

Conversation

ebkalderon
Copy link
Owner

Changed

  • Use auto_impl to delegate implementation of Box<S> where S: LanguageServer + ?Sized.

It turns out that auto_impl can work with async-trait as long as the auto_impl invocation occurs first, such that the #[auto_impl] macro receives the desugared output of #[async_trait].

Original suggestion from #107 (comment).

CC @icsaszar

It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
@ebkalderon ebkalderon self-assigned this Feb 25, 2020
@ebkalderon
Copy link
Owner Author

ebkalderon commented Feb 25, 2020

Hmm, seems like merging this would be a breaking API change and a loss of functionality, since auto_impl is refusing to add the ?Sized bound to the generated impl.

fn assert_impl<T: LanguageServer>() {}

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

According to auto-impl-rs/auto_impl#54, adding ?Sized should be supported by the macro, and the async-trait README confirms that traits can be made object safe and unsized if the Sync bound is added, which it is. Not sure why it's not doing it, though. I'll look into this further when I have time.

@ebkalderon
Copy link
Owner Author

I had something of a realization that this may be impossible to get to work because the codegen for auto_impl doesn't know to wrap the method bodies in a Box::pin(async move { ... }) like async-trait does. I think I'll just close this pull request for now and stick with a manual Box implementation for this trait unless auto-impl-rs/auto_impl#65 can be resolved. If it is achievable, we can always reopen this PR.

@ebkalderon ebkalderon closed this Feb 25, 2020
@dtolnay
Copy link

dtolnay commented Feb 25, 2020

I don't know why wrapping the method body in Box::pin(async move { ... }) would be necessary. Returning the same pinned future constructed by the trait object should be fine like all other traits.

use std::future::Future;
use std::pin::Pin;

struct Error;

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

impl<T: Foo + ?Sized> Foo for Box<T>
where
    Box<T>: 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,
    {
        Foo::bar(&**self)
    }
}

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

@ebkalderon
Copy link
Owner Author

ebkalderon commented Feb 25, 2020

Good point, @dtolnay . I didn't think of that! I had imagined an async fn body with an .await in my head when picturing the async-trait codegen, and I didn't consider returning the Foo::bar() future itself without bothering to .await it inside of a Box::pin(async {}).

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

Successfully merging this pull request may close these issues.

2 participants