launchbadge / sqlx

🧰 The Rust SQL Toolkit. An async, pure Rust SQL crate featuring compile-time checked queries without a DSL. Supports PostgreSQL, MySQL, and SQLite.
Apache License 2.0
13.42k stars 1.28k forks source link

async_fn_in_trait (AFIT) usage #3016

Open tgross35 opened 9 months ago

tgross35 commented 9 months ago

I haven't seen it discussed anywhere, but could sqlx benefit from AFIT as-is?

This was recently released with 1.75 (2023-12-28). As I understand it, it would mean that some of the traits that look pretty messy:

pub trait Executor<'c>: Send + Debug + Sized {
    type Database: Database;

    fn fetch_optional<'e, 'q, E>(
        self,
        query: E
    ) -> Pin<Box<dyn Future<Output = Result<Option<<Self::Database as Database>::Row>, Error>> + Send + 'e>>
       where 'q: 'e,
             'c: 'e,
             E: 'q + Execute<'q, Self::Database>;
    // ...
}

Can now be written with a more understandable syntax, which also drops the Box<dyn ...> indirection:

pub trait Executor<'c>: Send + Debug + Sized {
    type Database: Database;

    async fn fetch_optional<'e, 'q, E>(
        self,
        query: E
    ) -> Result<Option<<Self::Database as Database>::Row>, Error>
       where 'q: 'e,
             'c: 'e,
             E: 'q + Execute<'q, Self::Database>;
    // ...
}

The blog post here has more information https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html. It seems like the limitations could make things less usable, but perhaps still beneficial?

Obviously putting this to use would be a MSRV break so it won't be usable anytime soon, but I am just curious if this has been looked at at all.

Thanks!

FSMaxB commented 9 months ago

From what I understand, this would need to be:

pub trait Executor<'c>: Send + Debug + Sized {
    type Database: Database;

    fn fetch_optional<'e, 'q, E>(
        self,
        query: E
    ) -> impl Future<Output = Result<Option<<Self::Database as Database>::Row>, Error>> + Send + 'e
       where 'q: 'e,
             'c: 'e,
             E: 'q + Execute<'q, Self::Database>;
    // ...
}

This would be required to get the Send bound in there, but when implementing the trait, async fn can be used.

FSMaxB commented 9 months ago

In other words: The limitation mentioned in the release blog post doesn't apply, because sqlx already requires the futures to be Send anyways.

dave42w commented 9 months ago

This would be a fantastic improvement, I've found the current signatures very intimidating.

abonander commented 9 months ago

This is a refactor we've been planning to do for a long time, we were just waiting for the feature to land in stable. And of course, to find the time to actually do it.

abonander commented 8 months ago

Re. MSRV, I'll remind readers of our official policy: https://github.com/launchbadge/sqlx/blob/main/FAQ.md#what-versions-of-rust-does-sqlx-support-what-is-sqlxs-msrv

This refactor is a breaking change regardless, as it would change the return types of many public methods. They'd go from nameable boxed trait objects to unnameable impl Future types.