Remove async_trait dependency

[muzarski]

Ref: #771

Motivation

async-trait is an unstable crate and so needs to be removed from public API to stabilize our API.

Solution

Rust 1.75 async traits

In 1.75 release release of rust, it allows us to define traits with async functions. However, such traits are not object safe, and so they cannot be used in the trait objects (dyn Trait). Unfortunately, all of the traits using async_trait are actually used in the trait objects. This means that we cannot simply remove the async_trait macro and expect everything to compile.

Boxed futures

The other solution is to box the futures - the exact same thing that async_trait does for us.
This is why, for each of the trait that used async_trait we replace the signatures of async functions from:

#[async_trait]
trait Foo {
    async fn foo(&self, some_ref: &SomeType) -> ReturnType;
}

to:

trait Foo {
    fn foo<'a>(&'a self, some_ref: &'a SomeType) -> futures::BoxFuture<'_, ReturnType>;
}

Notice that some trait functions, apart from &self reference, accept some additional reference. In result, we need to introduce a lifetime annotation to the signatures so the compiler can deduce the lifetime parameter of the futures::BoxFuture. Unfortunately, implementors of such traits will need to introduce these lifetime annotations as well. I'm not sure if there is a better solution to the issue.

In addition, the implementors of the traits will need to explicitly box the returned futures like in the following example (or by using some std::pin::Pin utilities):

use futures::FutureExt;
struct Bar;
impl Foo for Bar {
    fn foo<'a>(&'a self, some_ref: &'a SomeType) -> futures::BoxFuture<'_, ReturnType> {
         async move { /* ... code that returns ReturnType */ }.boxed()
    }
}

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• I added relevant tests for new features and bug fixes.
• All commits compile, pass static checks and pass test.
• PR description sums up the changes and reasons why they should be introduced.
• I have provided docstrings for the public items that I want to introduce.
• I have adjusted the documentation in ./docs/source/.
• I added appropriate Fixes: annotations to PR description.

[muzarski]

v2: adjusted the docs so CI passes

[piodul]

There is a trick similar to what erased-serde crate does:

1. Use async fn in our traits
2. For each trait X that is not object safe, introduce an ErasedX trait which has the same methods but they return Pin<Box<dyn Future + 'a>> or whatever
3. Provide impl ErasedX for dyn X (or something in that fashion)
4. Accept Box<dyn ErasedX> in our interfaces, but tell users to implement X

This approach is actually suggested in one of the blog posts about async fn in traits: https://blog.rust-lang.org/inside-rust/2023/05/03/stabilizing-async-fn-in-trait.html#modeling-dynamic-dispatch . Other blog post (https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#dynamic-dispatch) mention that there was a plan to add a macro to the trait-variant crate that automatically generates stuff I mentioned in points 2 and 3.

However:

• We will have to significantly bump MSRV for that, though. 1.76 was released only recently, so 1.75 is pretty new. Perhaps at this point it is fine to bump MSRV like that, after 1.0 we will probably have to be more conservative.
• The trait-variant doesn't seem to have this functionality, so we would have to duplicate our traits manually.

I think we can keep the current approach with boxed futures. It's not very pretty but it works, and I think that users aren't going to implement our async traits too frequently so I think we can live with that as well. Personally, I don't think it's worth it to use async traits yet, perhaps there will be a better official solution to this problem in the future and we might switch to it in the next stable version after the solution appears.

[piodul]

By using the BoxFuture alias, now you are putting the futures crate into our API.

Although futures, despite it being a pre-1.0 crate, is actually pretty stable (0.3.0 was released 4 years ago), I think it's completely unnecessary and we could just use Pin<Box<dyn Future<Output = Result<SocketAddr, TranslationError>> + 'a>> here (or something in that fashion).

[muzarski]

v2:

• removed futures::BoxFuture from public API
• introduced a utility type alias BoxedFuture in our library
• changed usages of futures::FutureExt::boxed() to Box::pin()
• updated docs accordingly

[piodul]

It's not a pub item but it appears in public traits. How does it interact with the documentation? I'd rather users didn't encounter an opaque BoxFuture alias. Users should know what is the definition, that it requires the future to be Send etc. - I think this information should be available if you make BoxedFuture public.

[muzarski]

Yes, you are right. From what I've tested the users trying to implement the trait encounter an opaque type (BoxedFuture) and cannot import it (since the definition is pub(crate)).

[muzarski]

v3: made BoxedFuture alias public

[Lorak-mmk]

One question: will this require implementors to also drop async_trait and manually use features?
So if a user implemented AuthenticatorSession / AuthenticatorProvider, will those implementations still work?

[Lorak-mmk]

This is another case of the same problem of pub(crate) in pub item.
How does it look in generated documentation?

[muzarski]

Yeah, it's the same as for BoxedFuture alias. There is no problem in the docs:

However, when I use the driver in some user application and generate the definition using vscode hints, the generated function signature contains the opaque type:

struct Foo;

impl AuthenticatorProvider for Foo {
    fn start_authentication_session<'a>(
        &'a self,
        authenticator_name: &'a str,
    ) -> scylla::BoxedFuture<
        '_,
        std::prelude::v1::Result<AuthInitialResponseAndSession, scylla::authentication::AuthError>,
    > {
        todo!()
    }
}

What's interesting tho, is that the compiler can deduce the type in the error message when the function is not implemented:

not all trait items implemented, missing: `start_authentication_session`
  --> src/main.rs:14:1
   |
14 | impl AuthenticatorProvider for Foo {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `start_authentication_session` in implementation
   |
   = help: implement the missing item: `fn start_authentication_session(&'a self, _: &'a str) -> Pin<Box<(dyn Future<Output = std::result::Result<(Option<Vec<u8>>, Box<(dyn AuthenticatorSession + 'static)>), String>> + Send + 'a)>> { todo!() }`

I'm not sure why vscode generates the function declaration with an opaque type.

I think that we should make this alias public as well. However, i think we should come up with a more fitting name for this type alias (or introduce a struct instead). I only introduced this alias because clippy was complaining about the complexity of the returned type and I wanted the alias to be used internally - unfortunately, it appears in the public trait declaration.

[muzarski]

One question: will this require implementors to also drop async_trait and manually use features? So if a user implemented AuthenticatorSession / AuthenticatorProvider, will those implementations still work?

It won't work with current version - there is a problem with lifetime parameters/bounds. The async_trait implementation provides a bit different lifetime annotations/bounds than our trait function declarations. However, I just checked that we can play around a bit with the lifetimes in the declarations so the users can still implement it automatically using async_trait crate.

Let's look at the generated function signature using async_trait for some struct implementing AuthenticatorSession::evaluate_challenge:

fn evaluate_challenge<'life0, 'life1, 'async_trait>(
  &'life0 mut self,
  _token: Option<&'life1 [u8]>
) -> ::core::pin::Pin<Box<dyn ::core::future::Future<Output = Result<Option<Vec<u8>>, AuthError>> + ::core::marker::Send + 'async_trait>>
where
    'life0: 'async_trait,
    'life1: 'async_trait,
    Self: 'async_trait

This is almost equivalent to our function declaration. Almost, because our current function declaration does not have a Self: 'a bound. However, if I just simply add where Self: 'a bound to the declaration, the error is still here:

lifetime parameters or bounds on method `evaluate_challenge` do not match the trait declaration

But If we change the current function declaration from:

fn evaluate_challenge<'a>(
        &'a mut self,
        token: Option<&'a [u8]>,
) -> BoxedFuture<'_, Result<Option<Vec<u8>>, AuthError>>;

to the exact same declaration that async_trait would generate:

fn evaluate_challenge<'a, 'b, 'c>(
    &'a mut self,
    _token: Option<&'b [u8]>,
) -> BoxedFuture<'c, Result<Option<Vec<u8>>, AuthError>>
where
    'a: 'c,
    'b: 'c,
    Self: 'c;

(and do the same for AuthenticatorSession::success) the example using async_trait compiles.

WDYT? Should we change the trait definitions as presented above? I think that then we could leave async_trait in the docs/examples as well.

cc: @piodul

[piodul]

Actually, now that I think about it, do we have to do all of this at all? Technically speaking, async_trait is not a part of our library's interface. As long as it only generates code which does not introduce any types or traits that the generated code depends upon, it's technically not a part of the public interface. Users don't have to use async_trait at all, they can implement our traits directly without that crate.

With the current direction, I think we are only doing disservice to ourselves by removing dependency on async_trait right now because the public API will be completely the same and we will only make the code inside our project more complicated.

Unless my assumptions about async_trait not having public items is wrong, or unless we reconsider async traits, I think this PR can just be closed.

@Lorak-mmk @muzarski what do you think?

[Lorak-mmk]

Actually, now that I think about it, do we have to do all of this at all? Technically speaking, async_trait is not a part of our library's interface. As long as it only generates code which does not introduce any types or traits that the generated code depends upon, it's technically not a part of the public interface. Users don't have to use async_trait at all, they can implement our traits directly without that crate.

With the current direction, I think we are only doing disservice to ourselves by removing dependency on async_trait right now because the public API will be completely the same and we will only make the code inside our project more complicated.

Unless my assumptions about async_trait not having public items is wrong, or unless we reconsider async traits, I think this PR can just be closed.

@Lorak-mmk @muzarski what do you think?

You are right, in case of proc macros they don't themselves have any public items.
The problem is if new version of async_trait starts to generate code that is not compatible with previous version. In that case we would need to remove it's usage an expand the code manually as in @muzarski 's comment #932 (comment)
I don't see any stability guarantees in async_trait page, so we need to watch out for this.
If we are satisifed with the current code generated by async_trait then I think this can be closed.

[piodul]

You are right, in case of proc macros they don't themselves have any public items. The problem is if new version of async_trait starts to generate code that is not compatible with previous version. In that case we would need to remove it's usage an expand the code manually as in @muzarski 's comment #932 (comment) I don't see any stability guarantees in async_trait page, so we need to watch out for this. If we are satisifed with the current code generated by async_trait then I think this can be closed.

I think there is no problem for use to use the old version of async_trait if a new, semver-incompatible one is released. Users can keep using it, too - possibly in parallel with other newer versions of async_trait, this is not a big cost and I don't see why it would be a problem. There is also always an option for the users to manually implement the traits as they are defined in the documentation, they can do it even right now.

[muzarski]

I agree with the above comments - closing the PR since async_trait does not introduce any items to our public API.