Add some additional checks to #[derive(Selectable)] to ensure field

[weiznich]

compatibility

We generate an additional function with trait bounds for each fields to check whether a certain rust field type is compatible with the underlying database type. This greatly improves error messages for potential type mismatches, as the user now gets an error message that points to the field causing the problem + which has a pretty clear meaning (stating that FromSql<…> is not implemented).

Unfortunaly that's not a perfect solution as we need to check against concrete backend implementation. This PR solves this by hard-coding all three out of the box supported backends (behind the corresponding feature flags). That works for cases where users want to have compatibility with all enabled backends, but there are certainly use-cases where this is not the case. We might need some opt-in/opt-out system here to prevent this from being a breaking change or making it impossible to support valid use-cases. On the one hand we might want to have this as default as the improved error messages is something which is really helpful for new users, on the other hand I cannot see how that would be possible without breaking changes.

Another down-side of this approach is that it generates basically duplicated error messages when more than one backend is enabled.

This is currently marked as Draft as I see that more as request for comments due to the various problems outlined above. I really would like to see something like that in stable diesel as this would improve the usability quite significantly in my opinion. (We would just need to tell everyone to use Selectable and after that at least many cryptic errors will go away)

[Ten0]

What makes it work with generics? (#2962)

[Ten0]

Isn't the Queryable requirement a bit harsh? (since it's essentially a helper to implement FromSqlRow shouldn't we require that instead?)

[weiznich]

What makes it work with generics? (#2962)

Currently it just skips embedded fields. So it handles generics, but not handling them at all…

Isn't the Queryable requirement a bit harsh? (since it's essentially a helper to implement FromSqlRow shouldn't we require that instead?)

Yes that's a good idea.

[Mingun]

I think, you also should describe downsides of this approach, otherwise it is unclear, why you need to manually enable great compiler messages. Also, it is unclear why the user should specify backend, because the struct is the same for all backends and isn't depend on them.

It would also be useful, if you include the example of the generated code that implements the check, maybe under the implementation note heading.

[weiznich]

For the downsides: There are no other downsides that this is backend specific, which should be obvious from the way you enable this.

For why not enabling that by default:

• It would be a breaking change
• Even if it wouldn't be a breaking change, there is no way to make this work without knowing the concrete backend (which sometimes might come from a third party crate).

For the generated code: That's really uninteresting as it's just another (well placed) trait bound.

I will update the documentation to clarify why the concrete backend is required. That's because deserialization is generic over backends and rustc fails to see through this for the generic case.

[Mingun]

At least example of generated code here in comment could be used if someone would take its time to think about possible solutions of mentioned problems -- just to understand how exactly the solution worked right now.

[weiznich]

The following usage

#[derive(Selectable, Queryable)]
#[diesel(check_for_backend(diesel::pg::Pg))]
struct User {
    id: String,
    name: i32,
}

generates the following rust code

impl<__DB: diesel::backend::Backend> Selectable<__DB> for User {
    type SelectExpression = (users::id, users::name);
    fn construct_selection() -> Self::SelectExpression {
        (users::id, users::name)
    }
}
fn _check_field_compatibility()
where
    String: diesel::deserialize::FromSqlRow<diesel::dsl::SqlTypeOf<users::id>, diesel::pg::Pg>,
    i32: diesel::deserialize::FromSqlRow<diesel::dsl::SqlTypeOf<users::name>, diesel::pg::Pg>,
{
}

Without the usage of #[diesel(check_for_backend()] the function is not generated at all.

[Mingun]

Ok, it seems that the problem with the generic check is also lied in the way, that types in schema module is backend dependent, right? I tried to illustrate the diesel approach of mapping SQL->Rust types here and it seems so.

[weiznich]

No, that's not the case. This playground is closer to what diesel does. Notability the sql types can be shared between different backends (although there are also backend dependent types there). This makes the schema.rs module backend independent, as long as you only use types that are supported by all backends.

[Mingun]

Actually, you show the same thing. I'd just try to show the generic case where backend type markers are different. Of course, in reality many of them are shared.

[weiznich]

While that's technically true, it's an important concept that they are mostly shared in practice.

[Mingun]

Another down-side of this approach is that it generates basically duplicated error messages when more than one backend is enabled.

Probably this could be solved if generate something like that:

// The implementation of `Check` conceptually is this,
// but supports tuples in DBs parameter
type Check<Column, DBs> = FromSqlRow<SqlTypeOf<Column>::SqlType, DBs>;

// Check for `check_for_backend(Pg, Mysql)`
fn check()
where
    String: Check<table::str_column, (Pg, Mysql)>,
    i32: Check<table::int_column, (Pg, Mysql)>,
{}

[weiznich]

Probably this could be solved if generate something like that:

I would guess that this results in the same problem as the current approach without check_for_backend(…): Rustc is not able to decide which of the impls is meant for the tuple + generic impl case, so it will not suggest a concrete impl, resulting it in suggesting an impl far of the actual error. In that case the error message would likely only mention that i32: Check<table::str_column, (Pg, Mysql)> is not satisfied, but not that that's because of i32 is missing the relevant FromSql impls.