Adding extended support for Box, Rc, and Arc

[LucaCappelletti94]

This PR addresses discussion #4609

Still have to:

• Implement also:
  • impl<T, DB, const N: usize> CanInsertInSingleQuery<DB> for Rc<[T; N]>
  • impl<T, DB, const N: usize> CanInsertInSingleQuery<DB> for Arc<[T; N]>
  • impl<DB: Backend> Migration<DB> for Rc<dyn Migration<DB> + '_>
  • impl<DB: Backend> Migration<DB> for Arc<dyn Migration<DB> + '_>
  • impl<T: QueryId + ?Sized> QueryId for Rc<T>
  • impl<T: QueryId + ?Sized> QueryId for Arc<T>
  • impl<T: ?Sized, DB> QueryFragment<DB> for Rc<T>
  • impl<T: ?Sized, DB> QueryFragment<DB> for Arc<T>
  • impl<T: Insertable> Insertable for Rc<T>
  • impl<T: Insertable> Insertable for Arc<T>
  • impl<T: AppearsOnTable> AppearsOnTable for Rc<T>
  • impl<T: AppearsOnTable> AppearsOnTable for Arc<T>
  • impl<T: SelectableExpression> SelectableExpression for Rc<T>
  • impl<T: SelectableExpression> SelectableExpression for Arc<T>
  • impl<T: ValidGrouping> ValidGrouping for Rc<T>
  • impl<T: ValidGrouping> ValidGrouping for Arc<T>
  • impl<T: ?Sized, ST, DB> Queryable<ST, DB> for std::rc::Rc<T>
  • impl<T: ?Sized, ST, DB> Queryable<ST, DB> for std::sync::Arc<T>
  • impl<T: ?Sized, ST, DB> Queryable<ST, DB> for Box<T>
  • impl<impl<T: ?Sized, ST, DB> ToSql<ST, DB> for std::rc::Rc<T>
  • impl<impl<T: ?Sized, ST, DB> ToSql<ST, DB> for std::sync::Arc<T>
  • impl<impl<T: ?Sized, ST, DB> ToSql<ST, DB> for Box<T>
  • impl<T, ST, DB> FromSql<ST, DB> for std::rc::Rc<T>
  • impl<T, ST, DB> FromSql<ST, DB> for std::sync::Arc<T>
  • impl<T, ST, DB> FromSql<ST, DB> for Box<T>
• Extend test suite for Rc
• Extend test suite for Arc

[weiznich]

Thanks for opening this PR. I think we likely want to cover most of these impls as well. (All but that one for Instrumentation)

[LucaCappelletti94]

Thanks for opening this PR. I think we likely want to cover most of these impls as well. (All but that one for Instrumentation)

Certainly - I just started with the PR draft to make sure that these initial minimal changes did not break anything yet.

[LucaCappelletti94]

@weiznich I have extended the list of traits to be implemented, please do let me know whether anything is missing.

[weiznich]

From the search linked above it's missing the following impls:

• Insertable for Arc/Rc
• Migration for Arc/Rc
• AppearsOnTable for Arc/Rc
• SelectableExpression for Arc/Rc
• ValidGrouping for Arc/Rc

Also it lists the instrumentation impl which will not be helpful as the only method there takes a &mut self argument which you won't have for Arc/Rc for obvious reasons.

[LucaCappelletti94]

From the search linked above it's missing the following impls:

* `Insertable for Arc/Rc`

* `Migration for Arc/Rc`

* `AppearsOnTable for Arc/Rc`

* `SelectableExpression for Arc/Rc`

* `ValidGrouping for Arc/Rc`

Also it lists the instrumentation impl which will not be helpful as the only method there takes a &mut self argument which you won't have for Arc/Rc for obvious reasons.

I have updated the PR tasks to include also these traits and removed the Instrumentation

[LucaCappelletti94]

I am afraid I will require some more instructions regarding how to proceed. As you can see from the CI run, trying to use std::rc::Rc or std::sync::Arc in a struct leads to the following errors:

 the trait `diesel::Expression` is not implemented for `std::string::String`

and

the trait `diesel::AppearsOnTable<schema::comments::table>` is not implemented for `std::string::String`

Which make sense as indeed, those traits are needed for the current trait implementations and those traits are not implemented for String. What should be preferable to do?

[weiznich]

That's kind of expected that this fails as the list of traits as written above is not designed to enable that use-case, but only Arc/Rc<dyn BoxableExpression>.
If that other things is the goal you would also need to implement AsExpression, although I'm not sure if it's possible to easily have both without running into conflicting trait implementations issues.

[LucaCappelletti94]

That's kind of expected that this fails as the list of traits as written above is not designed to enable that use-case, but only Arc/Rc<dyn BoxableExpression>. If that other things is the goal you would also need to implement AsExpression, although I'm not sure if it's possible to easily have both without running into conflicting trait implementations issues.

Just to better understand, in discussion #4609 I was asking exactly regarding the use of Rc in such structs, but maybe I had explained myself poorly. Do you believe it should be viable to do, in theory, or are there any blocking reasons?

That being said, there is a blanked implementation adding AsExpression<ST> to all types which already implement Expression<ST> for that ST, and Rc<T> would therefore implement AsExpression if it can implement Expression, which depends on T having implemented Expression - why does not T=String (in this case) implement the Expression trait?

impl<T, ST> AsExpression<ST> for T
where
    T: Expression<SqlType = ST>,
    ST: SqlType + TypedExpressionType,
{
    type Expression = T;

    fn as_expression(self) -> T {
        self
    }
}

[weiznich]

Just to better understand, #4609 I was asking exactly regarding the use of Rc in such structs, but maybe I had explained myself poorly. Do you believe it should be viable to do, in theory, or are there any blocking reasons?

That was mostly me skipping over details assuming I know what you want there.

That written, it's actually good to know that we cannot have both, which means from my point of view this is for now kind of a dead end as I consider both use-cases (using Rc<dyn BoxableExpression> and Rc<String as field in your struct) as something that would be desirable to support. If we cannot easily support both it's kind of hard to say which one should be supported without making it essentially impossible to support the other one as well. So as long as we do not have a solution for this I would hold of implementing anything to not block any future extensions. I personally do currently not have the time to investigate this in depth, given the many other things I need to handle (like answering all your questions).

That being said, there is a blanked implementation adding AsExpression to all types which already implement Expression for that ST, and Rc would therefore implement AsExpression if it can implement Expression, which depends on T having implemented Expression - why does not T=String (in this case) implement the Expression trait?

Note that this only implements AsExpression if the underlying type is an actual Expression String or any other rust value type is never considered a SQL expression by diesel, so that wild card impl is not helpful here to support both variants. What might be viable is to generate specific Rc and Arc variants for the AsExpression impls via the derive, but again that would drastically increase the number of impls generated by diesel, which in turn will certainly have an impact on compile times and error messages. It might be worth to quantify that impact, maybe it's not as bad as assumed but without hard numbers I wouldn't like blindly to go that way.

[LucaCappelletti94]

Thank you for your reply - regarding extending the AsExpression derive, I can try to add the Rc and Arc derive impls and report how much the compile time increase for both diesel and the practical case of my project.

I am not sure I understood whether I should try to add support for impl<ST> AsExpression<ST> for Rc<String> where String: AsExpression<ST> (and other std/core types), or whether such an implementation would preclude potential future impls of Rc<dyn BoxableExpression> and as such should not be added for the time being. I suppose you mentioned extending the AsExpression derive exactly because I could then wrap the String type into a custom type?

Thank you again for the patience in answering all of these questions, hopefully over time I will learn better the internals of diesel and be better able to contribute to it.

[weiznich]

Thank you for your reply - regarding extending the AsExpression derive, I can try to add the Rc and Arc derive impls and report how much the compile time increase for both diesel and the practical case of my project.

That would be very helpful

I am not sure I understood whether I should try to add support for impl AsExpression for Rc where String: AsExpression (and other std/core types), or whether such an implementation would preclude potential future impls of Rc and as such should not be added for the time being. I suppose you mentioned extending the AsExpression derive exactly because I could then wrap the String type into a custom type?

We want to add these impls here:

• impl #impl_generics AsExpression<#sql_type> for std::rc::Rc<#struct_ty> #where_clause
• impl #impl_generics AsExpression<#sql_type> for std::sync::Arc<#struct_ty> #where_clause

Possibly also these are required:

• impl #impl_generics AsExpression<#sql_type> for &'__expr std::rc::Rc<#struct_ty> #where_clause
• impl #impl_generics AsExpression<#sql_type> for &'__expr std::sync::Arc<#struct_ty> #where_clause

We could also consider adding impls for Box as well. Also that would need to update the documentation here + add these impls as well for the manual AsExpression impls in diesel.

By having this specific impls rustc can prove for each impl that there won't be a conflicting one through the impl<T> AsExpression<T::SqlType> for T where T: Expression impl as at knows that all of the specific types (#struct_ty) don't implement Expression.

[LucaCappelletti94]

I have approached the AsExpression implementation by using the same approach used for Cow and it seems to work appropriately also for Rc and Arc. If this is correct, there is no need for blankets.

That being said, it turns out that methods like get_result and first require Send and since Rc does not implement Send, I get the error `Rc<str>` cannot be sent between threads safely. At least, these are required when using diesel-async, I am not sure whether the same would apply also for diesel in its sync version.

Do you think such Send trait requirements could be removed in any way?

[weiznich]

Thanks for updating this PR.

As these impls do not seem to conflict with anything it should be fine to go that way.

There are still a few things I noticed about the changes.

Additionally I would like to see the following things added to the PR:

• A changelog entry stating that Rc+Arc are now supported types for serialization + deserialization + BoxableExpression
• A testcase that actually verifies inserting + loading data works
• A test case that verifies that Arc<dyn BoxableExpression> (and the Rc equivalent works)

That being said, it turns out that methods like get_result and first require Send and since Rc does not implement Send, I get the error Rc<str> cannot be sent between threads safely. At least, these are required when using diesel-async, I am not sure whether the same would apply also for diesel in its sync version.

That's diesel-async specific. It should just work with normal diesel, so it's reasonable to keep it here.

Do you think such Send trait requirements could be removed in any way?

Not without regressing use-cases that require the Send bound to be there as there is currently no way to abstract over the fact whether or not a type implements Send.

[weiznich]

I would like to add Box<T> variants for these impls as well as Box is essentially just another smart pointer.

[LucaCappelletti94]

For all traits described in the first post of the PR, or only for specifically ToSql?

[weiznich]

That's for ToSql, FromSql and AsExpression. For the other traits it's already implemented.

[LucaCappelletti94]

I have added ToSql and FromSql, but AsExpression seems to be already covered by the blanked. I must admit I am not sure why that blanked has a collision with Box and not with Rc or Arc. I will write tests for all three cases, and we will see whether they do actually work or not.

[LucaCappelletti94]

Yeah as expected, I can use Rc and Arc in the structs but the same does not work for Box. Again, it is not clear to me why the blanked covers the Box case but does not cover the Arc and Rc, I believe it has something to do with Rust's auto-dereferencing.

[LucaCappelletti94]

I will go through the comments and update the PR accordingly - regarding the Send/Rc issue, should I open an issue in diesel-async on the topic and, after this PR is fully resolved, try to explore ways with which such problem may be overcome, or do you reckon it is a hard blocking problem?

In my project I am currently using diesel-async for the backend, and this Rc PR is primarily to avoid creating near duplicates of the table structs I use in the frontend with yew (since structs composed of RCs and copy types can be easily shared across web components).

If it turns out that diesel-async cannot be used with Rcs, I would likely have to switch back to plain diesel in the backend to avoid excessive code duplication - I must admit I am still unfamiliar with what are the benefits of using diesel async vs diesel, I was just going by the recommendation of other developers. I would appreciate if you had any comments or benchmarks regarding this comparison.

Thank you for your time and patience,
Luca

[weiznich]

I will go through the comments and update the PR accordingly - regarding the Send/Rc issue, should I open an issue in diesel-async on the topic and, after this PR is fully resolved, try to explore ways with which such problem may be overcome, or do you reckon it is a hard blocking problem?

The diesel-async Send "problem" does not block this PR from being merged. As for opening an issue to get this fixed: I doubt that there is anything I can do there without language level support for such abstraction. If that's there we can talk about how to fix it in diesel-async, but until then it won't help.
(Well technically it might be possible to fix it "today" by never using any future generated by async blocks + never return a opaque return type, etc but only specific types that depend on the input parameters, etc and then let rustc figure out whether that type implements Send or not, but that's for rather obvious reason neither maintainable nor implementable)

I must admit I am still unfamiliar with what are the benefits of using diesel async vs diesel, I was just going by the recommendation of other developers. I would appreciate if you had any comments or benchmarks regarding this comparison.

There is no one fits all answer there. It really depends on your requirements, your setup, the amount of traffic you expect, etc. There are some benchmark results here. Please note that these are specific results, you are likely better of with just measuring the performance impact in your service instead to get something more meaningful for your use-case. What I can confidently say is that crates.io itself run with diesel (not async) until ~ a year ago. They were doing fine performance wise at that point, but switched to diesel-async then because it fitter better in their code base. A while after the switch they started using postgres query pipelining (diesel-async only feature), which helped them to improve the performance of a few endpoints by ~30%. But again pipelining has quite specific requirements to even work, so that might not be translatable to other code bases.

[LucaCappelletti94]

I am trying to test and fix the case for Rc<dyn BoxableExpression>, but unfortunately this case does not seem to implement Expression: Rc<dyn BoxableExpression<table, Pg, SqlType = _>>. Same thing happens for Arc.

#[diesel_test_helper::test]
fn can_rc_query_with_boxable_expression() {
    let connection = &mut connection_with_sean_and_tess_in_users_table();

    let expr: Rc<dyn BoxableExpression<users::table, _, SqlType = _>> =
        Rc::new(users::name.eq("Sean")) as _;

    let data = users::table.into_boxed().filter(expr).load(connection);
    let expected = vec![find_user_by_name("Sean", connection)];
    assert_eq!(Ok(expected), data);

    let expr: Rc<dyn BoxableExpression<users::table, _, SqlType = _>> =
        Rc::new(users::name.eq("Sean")) as _;

    let data = users::table.filter(expr).into_boxed().load(connection);
    let expected = vec![find_user_by_name("Sean", connection)];
    assert_eq!(Ok(expected), data);
}

with the full error being:

error[E0277]: the trait bound `Rc<dyn BoxableExpression<table, Pg, SqlType = _>>: AppearsOnTable<...>` is not satisfied
   --> diesel_tests/tests/boxed_queries.rs:201:42
    |
201 |     let data = users::table.into_boxed().filter(expr).load(connection);
    |                                          ^^^^^^ the trait `diesel::Expression` is not implemented for `Rc<dyn diesel::BoxableExpression<schema::users::table, Pg, SqlType = _>>`
    |
    = help: the following other types implement trait `diesel::Expression`:
              &T
              (T0, T1)
              (T0, T1, T2)
              (T0, T1, T2, T3)
              (T0, T1, T2, T3, T4)
              (T0, T1, T2, T3, T4, T5)
              (T0, T1, T2, T3, T4, T5, T6)
              (T0, T1, T2, T3, T4, T5, T6, T7)
            and 452 others
    = note: required for `Rc<dyn diesel::BoxableExpression<schema::users::table, Pg, SqlType = _>>` to implement `diesel::AppearsOnTable<schema::users::table>`
    = note: required for `BoxedSelectStatement<'_, (Integer, Text, Nullable<Text>), ..., ...>` to implement `FilterDsl<Rc<dyn diesel::BoxableExpression<schema::users::table, Pg, SqlType = _>>>`
    = note: the full name for the type has been written to '/home/luca/github/diesel/target/debug/deps/integration_tests-14ee3c308f18e762.long-type-688333457155762760.txt'
    = note: consider using `--verbose` to print the full type name to the console

[weiznich]

So I had a look at the relevant code and it seems like that this PR is missing that impl for Rc and Arc:

diesel/diesel/src/expression/mod.rs

Lines 166 to 168 in 15216dc

	impl<T: Expression + ?Sized> Expression for Box<T> {
	type SqlType = T::SqlType;
	}

This impl enables the Box<dyn BoxableExpression> pattern, but prevents using the wild card impl. That's that what I wrote initially with it's required to put specific impls in the derive generated output to prevent needing to decide wheter we want to support one (loading/storing values wrapped in a Arc/Rc) or the other feature (using Arc<dyn BoxableExpression> for dynamic clonable query fragments.

The summary is that you need to:

• Add the equivalents of the linked impl for Arc/Rc
• Remove the two blanked impls
• Add back the generated specific impls for Arc/Rc (and ideally Box) in the AsExpression macro + measure the compile time impact of the additional impls