Implement `Executor` for T when &T implements `Executor`

[jyn514]

In docs.rs I have a lot of code that looks like this:

        query!(
            "INSERT INTO queue (name, version, priority) VALUES ($1, $2, $3);",
            name,
            version,
            priority,
        )
        .fetch_all(self.db.get()?)

where db.get() returns Result<PoolConnection, Error>. This gives an error that PoolConnection doesn't implement Executor:

error[E0277]: the trait bound `sqlx::pool::PoolConnection<sqlx::Postgres>: sqlx::Executor<'_>` is not satisfied
  --> src/build_queue.rs:51:20
   |
51 |         .fetch_all(self.db.get()?)
   |                    ^^^^^^^^^^^^^^ the trait `sqlx::Executor<'_>` is not implemented for `sqlx::pool::PoolConnection<sqlx::Postgres>`
   |
   = help: the following implementations were found:
             <&'c mut sqlx::pool::PoolConnection<sqlx::Postgres> as sqlx::Executor<'c>>

The fix is to use .fetch_all(&mut self.db.get()?) instead. However this is a lot of boilerplate that seems like it shouldn't be necessary. Could sqlx add an impl Executor for PoolConnection directly?

I tried and failed to implement this (https://github.com/rust-lang/rust/issues/77158) so probably won't be making a PR in the near future.

[jyn514]

This breaks deref coercion. Here's an example for https://github.com/launchbadge/sqlx/blob/e575501a399be47864fcd5120ddd0df00ed1d9bc/sqlx-core/src/postgres/listener.rs#L117-L130

error[E0382]: use of moved value: `connection`
   --> sqlx-core/src/postgres/listener.rs:126:36
    |
119 |             let mut connection = self.pool.acquire().await?;
    |                 -------------- move occurs because `connection` has type `pool::connection::PoolConnection<postgres::database::Postgres>`, which does not implement the `Copy` trait
...
122 |             connection
    |             ---------- value moved here
...
126 |             self.connection = Some(connection);
    |                                    ^^^^^^^^^^ value used here after move

I'm not sure what to do from here ... it definitely doesn't seem ideal to move these unless you use (&mut conn).execute but I'd also like to have this feature :/

[juancampa]

I was also bitten by this.

I think the problem is that Executor is implemented for &mut Stuff and execute takes an Executor.

But a more idiomatic approach is to implement Executor for Stuff and have execute take a &mut Executor.

My particular case is that I have a function that takes an E: Executor which can only be used once because it's moved into execute, instead of borrowed.

[abonander]

But a more idiomatic approach is to implement Executor for Stuff and have execute take a &mut Executor.

We did have that in earlier versions. The problem is implementing Executor for Pool. Because we want to be able to use &Pool references as Executor, it required passing &mut &pool, which was confusing.

You might say "just have Pool work through mutable references like everything else," and perhaps that is a design decision we could revisit, but the reason we didn't go that way is Actix Web. Their Data extractor only provides access via & reference. That's otherwise the most convenient way to get a Pool into your request handler without writing a custom extractor.

[conradludgate]

I just ran into this. We needed a 'static Stream, so we made a simple type wrapper around Pool that implemented Executor without any reference and it worked fine, it just required that we clone the pool to pass into fetch_many()

Maybe I'm missing something. Can we for now have both impls only for pool?

impl Executor for Pool<DB> {}
impl Executor for &Pool<DB> {}

I see no blanket impl that forbids this.

[danielhenrymantilla]

• (FWIW, for the OP case, another option would be to make .fetch_all() take a T where &mut T : Executor)