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

Implementation of sqlx::Acquire is not general enough for MySqlConnection #1015

Closed
apognu opened this issue Jan 25, 2021 · 9 comments
Closed
Labels
bug db:mysql Related to MySQL fix-HRTB-pls Issues relating to Higher Ranked Trait Bounds in Rust

Comments

@apognu
Copy link

apognu commented Jan 25, 2021

This is, for me, a weird one, that I can only reproduce in a very specific situation, which may or may not be on sqlx's side.

My setup includes version 0.4.2 of sqlx, with Rocket from master (more on that below).

I am using sqlx as a State in Rocket, so I can recover a Pool<MySql> in every action function. The global idea of what I am trying to do is create database-layer functions that could either directly take &Pool<MySql>s or transactions into the pool (depending on where it is used, it may or may not be part of a transaction).

When used this way,, an error saying that the implementation of sqlx::Acquire is not general enough.

Here is a full minimal reproduction of the issue:

#[macro_use]
extern crate rocket;

use rocket::State;
use sqlx::{Acquire, MySql, Pool};

#[tokio::main]
async fn main() {}

#[get("/pool")]
async fn pool(pool: State<'_, Pool<MySql>>) {
    run_query(&*pool).await;
}

#[get("/transaction")]
async fn txn(pool: State<'_, Pool<MySql>>) {
    let mut txn = pool.begin().await.unwrap();

    run_query(&mut *txn).await;

    txn.commit().await.unwrap();
}

async fn run_query<'c, E>(pool: E)
where
    E: Acquire<'c, Database = MySql>,
{
    let mut conn = pool.acquire().await.unwrap();

    sqlx::query("SELECT 1").fetch_one(&mut *conn).await.unwrap();
}

The reported error is the following:

   Compiling rocket-sqlx-repro v0.1.0 (/.../rocket-sqlx-repro)
error: implementation of `sqlx::Acquire` is not general enough
  --> src/main.rs:10:1
   |
10 |   #[get("/pool")]
   |   ^^^^^^^^^^^^^^^ implementation of `sqlx::Acquire` is not general enough
   |
  ::: /.../src/gh.neting.cc-1ecc6299db9ec823/sqlx-core-0.4.2/src/acquire.rs:8:1
   |
8  | / pub trait Acquire<'c> {
9  | |     type Database: Database;
10 | |
11 | |     type Connection: Deref<Target = <Self::Database as Database>::Connection> + DerefMut;
...  |
15 | |     fn begin(self) -> BoxFuture<'c, Result<Transaction<'c, Self::Database>, Error>>;
16 | | }
   | |_- trait `sqlx::Acquire` defined here
   |
   = note: `&Pool<MySql>` must implement `sqlx::Acquire<'0>`, for any lifetime `'0`...
   = note: ...but `&Pool<MySql>` actually implements `sqlx::Acquire<'1>`, for some specific lifetime `'1`
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: implementation of `sqlx::Acquire` is not general enough
  --> src/main.rs:16:1
   |
16 |   #[get("/transaction")]
   |   ^^^^^^^^^^^^^^^^^^^^^^ implementation of `sqlx::Acquire` is not general enough
   |
  ::: /.../src/gh.neting.cc-1ecc6299db9ec823/sqlx-core-0.4.2/src/acquire.rs:8:1
   |
8  | / pub trait Acquire<'c> {
9  | |     type Database: Database;
10 | |
11 | |     type Connection: Deref<Target = <Self::Database as Database>::Connection> + DerefMut;
...  |
15 | |     fn begin(self) -> BoxFuture<'c, Result<Transaction<'c, Self::Database>, Error>>;
16 | | }
   | |_- trait `sqlx::Acquire` defined here
   |
   = note: `sqlx::Acquire<'1>` would have to be implemented for the type `&'0 mut MySqlConnection`, for any two lifetimes `'0` and `'1`...
   = note: ...but `sqlx::Acquire<'2>` is actually implemented for the type `&'2 mut MySqlConnection`, for some specific lifetime `'2`
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

error: could not compile `rocket-sqlx-repro`

To learn more, run the command again with --verbose.

The exact same function works properly when used without the Rocket layer, but fails when wrapped in Rocket's macros. I reported the issue here first because the error mentioned sqlx::Acquire. But I really have no idea what is going on.

Feel free to close if you think this is a Rocket issue (and sorry if it is).

Thank you.

@abonander abonander changed the title Implementation of sqlx::Acquire is not general enough Implementation of sqlx::Acquire is not general enough for MySqlConnection Jan 26, 2021
@abonander
Copy link
Collaborator

abonander commented Jan 26, 2021

Related to #954 but this seems to concern a different trait impl, or at least one of the errors does.

@abonander abonander added db:mysql Related to MySQL fix-HRTB-pls Issues relating to Higher Ranked Trait Bounds in Rust bug labels Jan 26, 2021
@apognu
Copy link
Author

apognu commented Jan 26, 2021

Thank you for classifying this. I see you added db:mysql: if it is any help at all, the same error appears if I replace MySql with Postgres, so it might not be a MySQL-specific issue.

@jplatte
Copy link
Contributor

jplatte commented Jan 26, 2021

I don't think this is related to #954, and probably a bug in user code. Can you provide a minimal verifiable example?

@jplatte
Copy link
Contributor

jplatte commented Jan 26, 2021

Also, what version of Rust are you using?

@apognu
Copy link
Author

apognu commented Jan 26, 2021

My first post contains all the code required to reproduce the issue. Bug was present with both Rust 1.49.0 and 1.51.0-nightly. It only occurs when used in a method decorated with Rocket's macro. Here's the minimal dependencies used in my example:

[dependencies]
rocket = { git = "https://github.com/SergioBenitez/Rocket" }
sqlx = { version = "0.4.2", features = ["mysql", "postgres", "runtime-tokio-rustls"] }
tokio = { version = "0.2.9" }

Do you any need any more information?

@jplatte
Copy link
Contributor

jplatte commented Jan 26, 2021

Interesting, it works if you remove just the attributes. Also, I've been able to reduce this using cargo expand to just this:

use rocket::{data::Data, handler::HandlerFuture, request::Request, State};
use sqlx::{Acquire, MySql, Pool};

#[allow(unreachable_code)]
fn monomorphized_function<'_b>(__req: &'_b Request, __data: Data) -> HandlerFuture<'_b> {
    Box::pin(async move {
        let __rocket_param_pool: State<'_, Pool<MySql>> = todo!();
        let ___responder = pool(__rocket_param_pool).await;
        rocket::handler::Outcome::from(__req, ___responder)
    })
}

async fn pool(pool: State<'_, Pool<MySql>>) {
    run_query(&*pool).await;
}

async fn run_query<'c, E>(pool: E)
where
    E: Acquire<'c, Database = MySql>,
{
    let mut conn = pool.acquire().await.unwrap();

    sqlx::query("SELECT 1").fetch_one(&mut *conn).await.unwrap();
}

The reason I find this very weird is that rustc complains about the bound in run_query() at the usage of pool(). This smells like a bug in rustc.

@jplatte
Copy link
Contributor

jplatte commented Jan 26, 2021

This seems to come down to lifetime parameters spilling into impl Future types that async fn are lowered to. I'm pretty sure I've got a fix about ready.

@jplatte
Copy link
Contributor

jplatte commented Jan 26, 2021

Okay, I was wrong. This is related to Send. Conceptually this can be fixed by defining run_query as

fn run_query<'a, 'c, E>(pool: E) -> impl Future<Output = ()> + Send + 'a
where
    E: Acquire<'c, Database = MySql> + Send + 'a,
    E::Connection: Send,
{
    async move {
        let mut conn = pool.acquire().await.unwrap();
        sqlx::query("SELECT 1").fetch_one(&mut *conn).await.unwrap();
    }
}

However, that produces a rather unhelpful error message:

error[E0308]: mismatched types
  --> src/lib.rs:23:37
   |
23 | fn run_query<'a, 'c, E>(pool: E) -> impl Future<Output = ()> + Send + 'a
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
   |
   = note: expected type `std::marker::Send`
              found type `std::marker::Send`

I will try to minimize that error and create an issue at rust-lang/rust.

I've also found a fix to this! For some reason that I couldn't figure out it only works against the current git master of sqlx, not against 0.4.2: You have to add the Send requirement to the associated type Acquire::Connection (#1020):

diff --git a/sqlx-core/src/acquire.rs b/sqlx-core/src/acquire.rs
index 308e80da..dd8670ae 100644
--- a/sqlx-core/src/acquire.rs
+++ b/sqlx-core/src/acquire.rs
@@ -8,7 +8,7 @@ use std::ops::{Deref, DerefMut};
 pub trait Acquire<'c> {
     type Database: Database;
 
-    type Connection: Deref<Target = <Self::Database as Database>::Connection> + DerefMut;
+    type Connection: Deref<Target = <Self::Database as Database>::Connection> + DerefMut + Send;
 
     fn acquire(self) -> BoxFuture<'c, Result<Self::Connection, Error>>;

and then define run_query as

fn run_query<'a, 'c, E>(pool: E) -> impl Future<Output = ()> + Send + 'a
where
    E: Acquire<'c, Database = MySql> + Send + 'a,
{
    async move {
        let mut conn = pool.acquire().await.unwrap();
        sqlx::query("SELECT 1").fetch_one(&mut *conn).await.unwrap();
    }
}

(same as above, but without the bound on E::Connection).

Realistically though, I'd recommend just not using the Acquire trait, instead using a &mut MySqlConnection parameter and calling pool.acquire().await? before run_query:

#[macro_use]
extern crate rocket;

use rocket::State;
use sqlx::{MySql, MySqlConnection, Pool};

#[get("/pool")]
async fn pool(pool: State<'_, Pool<MySql>>) {
    let mut conn = pool.acquire().await.unwrap();
    run_query(&mut conn).await;
}

#[get("/transaction")]
async fn txn(pool: State<'_, Pool<MySql>>) {
    let mut txn = pool.begin().await.unwrap();

    run_query(&mut *txn).await;

    txn.commit().await.unwrap();
}

async fn run_query(conn: &mut MySqlConnection) {
    sqlx::query("SELECT 1").fetch_one(conn).await.unwrap();
}

@apognu
Copy link
Author

apognu commented Jan 30, 2021

Using &mut MySqlConnection does fix my issue, indeed. Thank you for the pointer.

I don't know if you wish to keep this open or not (if there is actually an issue, regardless of my particular problem). I'll leave the decision to you.

Have a nice day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug db:mysql Related to MySQL fix-HRTB-pls Issues relating to Higher Ranked Trait Bounds in Rust
Projects
None yet
Development

No branches or pull requests

4 participants