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

Remove async-trait in favor of impl trait and native async in impls #196

Open
svix-jplatte opened this issue Mar 27, 2024 · 6 comments
Open

Comments

@svix-jplatte
Copy link

Hi! I would like to propose to redefine the ManageConnection and CustomizeConnection traits from

#[async_trait]
pub trait ManageConnection: Sized + Send + Sync + 'static {
    // other items...

    async fn connect(&self) -> Result<Self::Connection, Self::Error>;
    async fn is_valid(&self, conn: &mut Self::Connection) -> Result<(), Self::Error>;
}

#[async_trait]
pub trait CustomizeConnection<C: Send + 'static, E: 'static>:
    fmt::Debug + Send + Sync + 'static
{
    async fn on_acquire(&self, _connection: &mut C) -> Result<(), E> {
        Ok(())
    }
}

to

pub trait ManageConnection: Sized + Send + Sync + 'static {
    // other items...

    fn connect(&self) -> impl Future<Output = Result<Self::Connection, Self::Error>> + Send;
    fn is_valid(&self, conn: &mut Self::Connection) -> impl Future<Output = Result<(), Self::Error>> + Send;
}

pub trait CustomizeConnection<C: Send + 'static, E: 'static>:
    fmt::Debug + Send + Sync + 'static
{
    fn on_acquire(&self, _connection: &mut C) -> impl Future<Output = Result<(), E>> + Send {
        async move { Ok(()) }
    }
}

for bb8 0.9.0.

This will allow bb8 to be used without pulling in any proc-macros AFAICT, people will still be able to write async fn in impls (just without #[async_trait]), and it will improve the rendered documentation (see svix/omniqueue-rs#34 for the diff on another crate).

I can send a PR if you agree that this is a good idea. In that case, do you want to keep main for the 0.8.x release line and create a new branch for 0.9.x, or have the PR against main?

@djc
Copy link
Owner

djc commented Mar 27, 2024

Would this need an MSRV bump, and if so, to what version? This is just RPITIT, right? Why not use AFIT here?

I do think it would be nice to adopt a more direct API eventually and am open to a PR, but unsure about the timeline here. It feels a little silly to adopt RPITIT now only to move over to AFIT later?

@svix-jplatte
Copy link
Author

Would this need an MSRV bump, and if so, to what version?

Yes, this would bump the MSRV to 1.75.

This is just RPITIT, right? Why not use AFIT here?

This is both, because they're compatible. RPITIT for the trait declarations to require Send futures, otherwise code that's generic over these traits has to assume !Send futures until RTN (return-type notation) comes around, which would be a PITA; impls can be written using AFIT.

I do think it would be nice to adopt a more direct API eventually and am open to a PR, but unsure about the timeline here. It feels a little silly to adopt RPITIT now only to move over to AFIT later?

Yeah, it would be a breaking change to move the declarations to AFIT later, but a very minor one because implementations wouldn't change at all, just code that's generic over these traits and assumes the returned futures are Send would have to use RTN or some equivalent once the Send requirement is removed from the trait decl (with or without switching from RPITIT to AFIT).

@djc
Copy link
Owner

djc commented Mar 27, 2024

Okay, it will be a while before I want to accept a bump to 1.75, probably when we hit 1.81 or 1.83 or some such?

@svix-jplatte
Copy link
Author

svix-jplatte commented Mar 27, 2024

Okay, so... In half a year or a bit more. That's seems like a lot to me but I'm not here to start a debate over MSRV policies :)

Thanks for the fast responses!

@djc
Copy link
Owner

djc commented Mar 27, 2024

Yes, sorry -- I know I am on the conservative side here -- and given that this is definitely a lower-priority project for me I really don't want to maintain two branches in parallel.

@svix-jplatte
Copy link
Author

No worries, it's not like async-trait is a seriously problematic dependency to get rid of.

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

No branches or pull requests

2 participants