RunQueryDsl: match same trait constraints as diesel

[eakoli]

This resolves issue #142 which stems from the async RunQueryDsl being implemented for ALL types.

In addition it adds some constraints that were missing from async LoadQuery when compared to diesel LoadQuery

Comparing the original diesel RunQueryDsl to the async one, the original only implements RunQueryDsl for types that implement Table and a few other explicitly tagged (like SqlQuery)

https://github.com/diesel-rs/diesel/blob/v2.2.7/diesel/src/query_dsl/mod.rs#L1785-L1791

the following shows how there is not an issue with diesel proper, only with diesel_async::RunQueryDsl

mod sub {

    pub(crate) struct A {
        pub(crate) items: Vec<String>,
    }
}

mod success_1 {
    fn test(value: super::sub::A) -> String {
        value.items.first().map(|s| s.into()).unwrap() // properly compiles and uses Vec::first
    }
}

mod success_2 {
    use diesel::prelude::*;
    fn test(value: super::sub::A) -> String {
        value.items.first().map(|s| s.into()).unwrap() // properly compiles and uses Vec::first
    }
}

mod failure {
    use diesel::prelude::*;
    use diesel_async::RunQueryDsl;

    fn test(value: super::sub::A) -> String {
        value.items.first().map(|s| s.into()).unwrap() // complains Table is not implemented by Vec<std::string::String>
    }
}

[weiznich]

Thanks for submitting this PR. I fear this approach won't work for the reasons outlined below.

[weiznich]

I fear this assumption is not correct. See for example this impl: https://docs.diesel.rs/master/diesel/prelude/trait.RunQueryDsl.html#impl-RunQueryDsl%3CConn%3E-for-BoxedSqlQuery%3C'_,+%3CConn+as+Connection%3E::Backend,+Query%3E

For the linked impl these trait bounds would imply that Conn implements AsyncConnection and Connection at the same time, which is generally not fulfilled by any type.

Also even if we get it so that diesel doesn't have this kind of constraints internally that wouldn't help with third party crates which totally could write connection specific impls.

[eakoli]

Im not sure I follow what the reason is, are you saying that the where ends up being too restrictive?

For the linked impl these trait bounds would imply that Conn implements AsyncConnection and Connection at the same time, which is generally not fulfilled by any type.

Where does it imply that Conn implements anything? It only makes assertion with respect to T, not to Conn.

Also even if we get it so that diesel doesn't have this kind of constraints internally that wouldn't help with third party crates which totally could write connection specific impls.

What internal constraint are you referring to? And connection specific impl of what?

What about something a little more explicit?

    impl <T, Conn: Connection, AsyncConn> RunQueryDsl<AsyncConn> for T where T: RunQueryDsl<Conn> {}

[weiznich]

I'm not sure if we are talking about the same impl, because the linked impl literally reads as follows:

impl<Conn: Connection, Query> RunQueryDsl<Conn> for BoxedSqlQuery<'_, Conn::Backend, Query>

which obviously does restrict Conn to the Connection trait from diesel.

For this trait implementation the changed impl would imply that Conn needs at the same time implement AsyncConnection and Connection (via the T: diesel::RunQueryDsl<Conn> bound). That's not possible to be fulfilled by any connection type.

What about something a little more explicit?

 impl <T, Conn: Connection, AsyncConn> RunQueryDsl<AsyncConn> for T where T: RunQueryDsl<Conn> {}

I fear it's unfortunally not possible to use different generics for these impl due to how the trait system in rust works. You will get an error about an unconstrained generic parametr (Conn) in that case. Otherwise this would likely be the right way to solve this issue.

[eakoli]

Ok, I see what you mean with the conn :Connection, and unmatched type.

What about T where T: AsQuery, as this comment implies it really should be ?

I pushed up a changes to match on AsQuery, as well as tests to verify all the current types that work for RunQueryDsl also work for diesel_async::RunQueryDsl<conn: AsyncConnection>

[weiznich]

Sorry for taking that long to answer, but I really did not have the time to actually check whether the comment there is still relevant.
I did now have some time and checked with the following example:

async fn test(conn: &mut AsyncPgConnection) {
    users::table
        .filter(posts::id.eq(42))
        .load::<(i32,)>(conn)
        .await;
}

without your patch that generates the following error:

error[E0271]: type mismatch resolving `<table as AppearsInFromClause<table>>::Count == Once`
  --> src/main.rs:11:10
   |
11 |         .load::<(i32,)>(conn)
   |          ^^^^ expected `Once`, found `Never`
   |
note: required for `posts::columns::id` to implement `AppearsOnTable<users::table>`
  --> src/main.rs:6:25
   |
6  | diesel::table! { posts{ id -> Integer}}
   |                         ^^
   = note: associated types for the current `impl` cannot be restricted in `where` clauses
   = note: 2 redundant requirements hidden
   = note: required for `Grouped<Eq<id, Bound<Integer, i32>>>` to implement `AppearsOnTable<users::table>`
   = note: required for `WhereClause<Grouped<Eq<id, Bound<Integer, i32>>>>` to implement `query_builder::where_clause::ValidWhereClause<FromClause<users::table>>`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `Query`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `diesel_async::methods::LoadQuery<'_, _, (i32,)>`
   = note: the full name for the type has been written to '/tmp/diesel_test/target/debug/deps/diesel_test-acb1381933ce240a.long-type-13903671074851835938.txt'
   = note: consider using `--verbose` to print the full type name to the console

error[E0277]: the trait bound `users::table: TableNotEqual<posts::table>` is not satisfied
  --> src/main.rs:11:10
   |
11 |         .load::<(i32,)>(conn)
   |          ^^^^ the trait `TableNotEqual<posts::table>` is not implemented for `users::table`
   |
   = note: double check that `posts::table` and `users::table` appear in the same `allow_tables_to_appear_in_same_query!` 
           call if both are tables
   = help: the following other types implement trait `TableNotEqual<T>`:
             `Only<diesel::pg::metadata_lookup::pg_namespace::table>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_type::table>`
             `Only<diesel::pg::metadata_lookup::pg_type::table>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_namespace::table>`
             `Tablesample<diesel::pg::metadata_lookup::pg_namespace::table, TSM>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_type::table>`
             `Tablesample<diesel::pg::metadata_lookup::pg_type::table, TSM>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_namespace::table>`
             `diesel::pg::metadata_lookup::pg_namespace::table` implements `TableNotEqual<Only<diesel::pg::metadata_lookup::pg_type::table>>`
             `diesel::pg::metadata_lookup::pg_namespace::table` implements `TableNotEqual<Tablesample<diesel::pg::metadata_lookup::pg_type::table, TSM>>`
             `diesel::pg::metadata_lookup::pg_namespace::table` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_type::table>`
             `diesel::pg::metadata_lookup::pg_type::table` implements `TableNotEqual<Only<diesel::pg::metadata_lookup::pg_namespace::table>>`
           and 2 others
   = note: required for `users::table` to implement `AppearsInFromClause<posts::table>`
note: required for `posts::columns::id` to implement `AppearsOnTable<users::table>`
  --> src/main.rs:6:25
   |
6  | diesel::table! { posts{ id -> Integer}}
   |                         ^^
   = note: 2 redundant requirements hidden
   = note: required for `Grouped<Eq<id, Bound<Integer, i32>>>` to implement `AppearsOnTable<users::table>`
   = note: required for `WhereClause<Grouped<Eq<id, Bound<Integer, i32>>>>` to implement `query_builder::where_clause::ValidWhereClause<FromClause<users::table>>`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `Query`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `diesel_async::methods::LoadQuery<'_, _, (i32,)>`
   = note: the full name for the type has been written to '/tmp/diesel_test/target/debug/deps/diesel_test-acb1381933ce240a.long-type-13903671074851835938.txt'
   = note: consider using `--verbose` to print the full type name to the console

with your patch this generates that error:

error[E0599]: the method `load` exists for struct `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>`, but its trait bounds were not satisfied
  --> src/main.rs:11:10
   |
9  | /     users::table
10 | |         .filter(posts::id.eq(42))
11 | |         .load::<(i32,)>(conn)
   | |         -^^^^ method cannot be called due to unsatisfied trait bounds
   | |_________|
   |
   |
  ::: /home/weiznich/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/diesel-2.2.4/src/query_builder/select_statement/mod.rs:48:1
   |
48 | / #[diesel_derives::__diesel_public_if(
49 | |     feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
50 | |     public_fields(
51 | |         select,
...  |
61 | | )]
   | |__- doesn't satisfy `_: AsQuery` or `_: RunQueryDsl<_>`
   |
   = note: the full type name has been written to '/tmp/diesel_test/target/debug/deps/diesel_test-7a841758257f912e.long-type-11988839005461461334.txt'
   = note: consider using `--verbose` to print the full type name to the console
   = note: the following trait bounds were not satisfied:
           `SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: AsQuery`
           which is required by `SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: diesel_async::RunQueryDsl<_>`
           `&SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: AsQuery`
           which is required by `&SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: diesel_async::RunQueryDsl<_>`
           `&mut SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: AsQuery`
           which is required by `&mut SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: diesel_async::RunQueryDsl<_>`
   = help: items from traits can only be used if the trait is in scope

So it seems like the comment over there in the diesel code is still relevant today. I'm sorry to say that, but in my opinion that's a too bad regression of the error messages emitted in such failure cases to be acceptable.

[urkle]

@weiznich is there anyway we can get something in place to reduce this global injection? As this makes it very challenging to use diesel-async without having to always use the alternate trait call syntax. As I have other crates which implement same-named methods (e.g. execute etc.) that start conflicting all over.

diesel_async::RunQueryDsl::first::<Model>(query, &mut db);

[weiznich]

@urkle I'm not aware of any solution here, but I'm open to accept a PR (or even suggestions) with something that doesn't break other usages (see details above). Ultimately that's in my opinion a bug in rustc as the relevant method is not even valid for other types as they don't fulfill the trait bounds.

[eakoli]

@weiznich Ive been experimenting with two alternative measures to remove the global injection.

A) if async was moved into diesel proper (as opposed to being an extra create) then it could properly apply the same exact impl clauses as the sync RunQueryDsl.

B) (possibly less intrusive) introduce a marker trait with no bounds to indicate what should implement RunQueryDsl, similar to how its currently matched on T: Table

  // Marker trait for opting in to RunQueryDsl support
  trait SupportRunQueryDsl {}

  // items that currently implement RunQueryDsl explicitly
  // -- query_dsl/mod.rs
  - impl<T, Conn> RunQueryDsl<Conn> for T: Table {};
  + impl<T> SupportRunQueryDsl for T: Table {};

  // -- query_dsl/positional_order_dsl.rs
  - impl<Source, Expr, Conn> RunQueryDsl<Conn> for PositionalOrderClause<Source, Expr> {}
  + impl<Source, Expr> SupportRunQueryDsl for PositionalOrderClause<Source, Expr> {}

  // -- expression/sql_literal.rs
  - impl<ST, T, Conn> RunQueryDsl<Conn> for SqlLiteral<ST,T> {}
  + impl<ST, T> SupportRunQueryDsl for SqlLiteral<ST,T> {}
  ...
  - impl<Query, Value, Conn> RunQueryDsl<Conn> for UncheckedBind<Query,Value> {} 
  + impl<Query, Value> SupportRunQueryDsl for UncheckedBind<Query,Value> {}

  // -- query_builder/sql_query.rs
  - impl<Inner, Conn> RunQueryDsl<Conn> for diesel::query_builder::SqlQuery<Inner> {}
  + impl<Inner> SupportRunQueryDsl for diesel::query_builder::SqlQuery<Inner> {}
  ...
  - impl<Conn, Query, Value> RunQueryDsl<Conn> for diesel::query_builder::UncheckedBind<Query,Value> {}
  + impl<Query, Value> SupportRunQueryDsl for diesel::query_builder::UncheckedBind<Query,Value> {}
  ...
  - impl<Conn: Connection, Query> RunQueryDsl<Conn> for diesel::query_builder::BoxedSqlQuery<'_, Conn::Backend, Query> {}
  + impl<Conn: Connection, Query> SupportRunQueryDsl for diesel::query_builder::BoxedSqlQuery<'_, Conn::Backend, Query> {}

 // -- query_builder/combination_clause.rs
 - impl<Combinator, Rule, Source, Rhs, Conn> RunQueryDsl<Conn> for CombinationClause<Combinator, Rule, Source, Rhs> {}
 + impl<Combinator, Rule, Source, Rhs> SupportRunQueryDsl for CombinationClause<Combinator, Rule, Source, Rhs> {}

 // -- query_builder/update_statement/mod.rs
 - impl<T: QuerySource, U, V, Ret, Conn> RunQueryDsl<Conn> for UpdateStatement<T, U, V, Ret> {}
 + impl<T: QuerySource, U, V, Ret> SupportRunQueryDsl for UpdateStatement<T, U, V, Ret> {}

 // -- query_builder/select_statement/boxed.rs
 - impl<ST, QS, DB, Conn, GB> RunQueryDsl<Conn> for BoxedSelectStatement<'_, ST, QS, DB, GB> {}
 + impl<ST, QS, DB, GB> SupportRunQueryDsl for BoxedSelectStatement<'_, ST, QS, DB, GB> {}

 // -- query_builder/select_statement/dsl_impls.rs
 - impl<F, S, D, W, O, LOf, G, H, LC, Conn> RunQueryDsl<Conn> for SelectStatement<F, S, D, W, O, LOf, G, H, LC> {}
 + impl<F, S, D, W, O, LOf, G, H, LC> SupportRunQueryDsl for SelectStatement<F, S, D, W, O, LOf, G, H, LC> {}

 // -- query_builder/insert_statement/mod.rs
 - impl<T: QuerySource, U, Op, Ret, Conn> RunQueryDsl<Conn> for SelectStatement<T, U, Op, Ret> {}
 + impl<T: QuerySource, U, Op, Ret> SupportRunQueryDsl for SelectStatement<T, U, Op, Ret> {}

 // -- query_builder/delete_statement/mod.rs
 - impl<T, U, Ret, Conn> RunQueryDsl<Conn> for DeleteStatement<T, U, Ret> where T: QuerySource {}
 + impl<T, U, Ret> SupportRunQueryDsl for DeleteStatement<T, U, Ret> where T: QuerySource {}

 // -- query_source/aliasing/dsl_impls.rs
 -  impl<S: AliasSource, Conn> RunQueryDsl<Conn> for Alias<S> {}
 +  impl<S: AliasSource> SupportRunQueryDsl for Alias<S> {}
 
  ....

  
  // then to implement RunQueryDsl on all things that should be is simply
  impl <T, Conn> RunQueryDsl<Conn> for T where T: SupportRunQueryDsl {}

  // which then lets diesel_async simply do this
  impl <T, Conn> diesel_async::RunQueryDsl<Conn> for T where T: SupportRunQueryDsl {}

C) Another option (although possibly a more breaking API change) would be to remove the Conn bounds from the RunQueryDsl trait (I don't think its really needed anyway, as all the methods take a conn and apply the actual bounds to it)

that way this solution would work, as there are no unconstrained parameters

 Note: Match the same types that diesel::RunQueryDsl applies to this
// seems safe currently, as the trait imposes no restrictions based on Conn, only on T.
impl<T> diesel_async::RunQueryDsl for T where T: diesel::RunQueryDsl {}

If any of these seem feasible, I can start working up the changes.

[weiznich]

First of all I would like to repeat that I see that this is a problem. It's unfortunately something (as already expressed) without easy solution. I also appropriate that you try to make progress here

That out of the way let's go through your different proposals.

A) if async was moved into diesel proper (as opposed to being an extra create) then it could properly apply the same exact impl clauses as the sync RunQueryDsl.

Yes and no. Yes we could write the same impls as for the other types in diesel, but at the same time we would force anyone that implements RunQueryDsl outside of diesel to now implement the sync and async variant.
(And that completely skips over questions like the fact that diesel (the main crate) is already too large and shouldn't become that much larger + that there are different levels of API stability between what's offered by diesel and diesel-async)

B) (possibly less intrusive) introduce a marker trait with no bounds to indicate what should implement RunQueryDsl, similar to how its currently matched on T: Table

That would again work for types in diesel, but not for external types. The current approach also works for external query types. Now these external types could opt in into the new trait at a later point in time, so that might be feasible from that point of view. It's still a breaking change in diesel-async, but that might be acceptable. The larger question there is: Will it regress error messages? If that's not the case this might be an OK solution. You likely can check this by looking at the differences for the compile tests in diesel_compile_tests in the diesel repo. Possibly #[diagnostic::do_not_recommend] could help there to get back the old messages.

C) Another option (although possibly a more breaking API change) would be to remove the Conn bounds from the RunQueryDsl trait (I don't think its really needed anyway, as all the methods take a conn and apply the actual bounds to it)

As already noted by you, that's a breaking change for diesel. We don't plan any major diesel release anytime soon, so that's not possible to have.

[eakoli]

@weiznich RE: OptionB

I guess the marker would not need to replace the existing, but could be done in-addition to. That way nothing really changes in the public API, other than there being an additional trait that can be used to match on for diesel_async, it could even be made a sealed namable trait. Thus no one could apply it to new types, but it could still be used by async to match everything that should have it implemented.

external re-implmenetations of RunQueryDsl would be unaffected, for both diesel and diesel_async, as they would still need to implement RunQueryDsl for any new types.

It's still a breaking change in diesel-async,

Can you expand on this? The only thing I could possibly see it being a breaking change would be if someone introduced a new Query type and relied on the blanket for T to have it implement RunQueryDsl. That is already a deviation from diesel proper, where the external would need to explicitly implement it for their new query type, and this would technically bring it more inline with how stock diesel works.

---

Also I do see that this is ultimately a rustc 'bug', but thats how it currently works, and I don't see any signs of it getting fixed any time soon.

Additionally, thanks for taking the time to brainstorm on this.

[weiznich]

I spend a bit more time thinking about this. The variant B might work, if it doesn't regress error messages.

I guess the marker would not need to replace the existing, but could be done in-addition to. That way nothing really changes in the public API, other than there being an additional trait that can be used to match on for diesel_async, it could even be made a sealed namable trait. Thus no one could apply it to new types, but it could still be used by async to match everything that should have it implemented.

If we do this change I personally would prefer to use it in diesel internally and also expose this trait publicly + document that this is now the preferred way to get a RunQueryDsl impl as this gets diesel and diesel-async closer together. It's explicitly wanted that people can apply it to their own query types.

Can you expand on this? The only thing I could possibly see it being a breaking change would be if someone introduced a new Query type and relied on the blanket for T to have it implement RunQueryDsl. That is already a deviation from diesel proper, where the external would need to explicitly implement it for their new query type, and this would technically bring it more inline with how stock diesel works.

Yes that's exactly what would break. Given that diesel-async is not at the 1.0 state yet that's acceptable, it's just something to note.