From 396d3113b970ae3b67959705cfdd3e34e46f3d43 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Sun, 2 Oct 2022 18:53:28 +0200 Subject: [PATCH 1/5] Add some additional checks to `#[derive(Selectable)]` to ensure field compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../fail/selectable_with_typemisamatch.rs | 26 ++++ .../fail/selectable_with_typemisamatch.stderr | 119 ++++++++++++++++++ diesel_derives/src/selectable.rs | 43 ++++++- 3 files changed, 185 insertions(+), 3 deletions(-) create mode 100644 diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs create mode 100644 diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr diff --git a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs new file mode 100644 index 000000000000..b745bcd79fe6 --- /dev/null +++ b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs @@ -0,0 +1,26 @@ +extern crate diesel; + +use diesel::prelude::*; + +table! { + users { + id -> Integer, + name -> Text, + } +} + +#[derive(Selectable, Queryable)] +#[diesel(table_name = users)] +struct User { + id: String, + name: i32, +} + + +fn main() { + let mut conn = PgConnection::establish("...").unwrap(); + + users::table.select(User::as_select()).load(&mut conn).unwrap(); + + +} diff --git a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr new file mode 100644 index 000000000000..8d65851babbb --- /dev/null +++ b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr @@ -0,0 +1,119 @@ +error[E0277]: the trait bound `i32: FromSql` is not satisfied + --> tests/fail/selectable_with_typemisamatch.rs:16:11 + | +16 | name: i32, + | ^^^ the trait `FromSql` is not implemented for `i32` + | + = help: the following other types implement trait `FromSql`: + > + > + > + > + > + > + > + > + and 13 others + = note: required because of the requirements on the impl of `diesel::Queryable` for `i32` + = help: see issue #48214 + = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable + +error[E0277]: the trait bound `i32: FromSql` is not satisfied + --> tests/fail/selectable_with_typemisamatch.rs:16:11 + | +16 | name: i32, + | ^^^ the trait `FromSql` is not implemented for `i32` + | + = help: the following other types implement trait `FromSql`: + > + > + > + > + > + > + > + > + and 13 others + = note: required because of the requirements on the impl of `diesel::Queryable` for `i32` + = help: see issue #48214 + = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable + +error[E0277]: the trait bound `i32: FromSql` is not satisfied + --> tests/fail/selectable_with_typemisamatch.rs:16:11 + | +16 | name: i32, + | ^^^ the trait `FromSql` is not implemented for `i32` + | + = help: the following other types implement trait `FromSql`: + > + > + > + > + > + > + > + > + and 13 others + = note: required because of the requirements on the impl of `diesel::Queryable` for `i32` + = help: see issue #48214 + = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable + +error[E0277]: the trait bound `*const str: FromSql` is not satisfied + --> tests/fail/selectable_with_typemisamatch.rs:15:9 + | +15 | id: String, + | ^^^^^^ the trait `FromSql` is not implemented for `*const str` + | + = help: the following other types implement trait `FromSql`: + <*const str as FromSql> + <*const str as FromSql> + <*const str as FromSql> + > + > + > + > + > + = note: required because of the requirements on the impl of `FromSql` for `std::string::String` + = note: required because of the requirements on the impl of `diesel::Queryable` for `std::string::String` + = help: see issue #48214 + = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable + +error[E0277]: the trait bound `*const str: FromSql` is not satisfied + --> tests/fail/selectable_with_typemisamatch.rs:15:9 + | +15 | id: String, + | ^^^^^^ the trait `FromSql` is not implemented for `*const str` + | + = help: the following other types implement trait `FromSql`: + <*const str as FromSql> + <*const str as FromSql> + <*const str as FromSql> + > + > + > + > + > + = note: required because of the requirements on the impl of `FromSql` for `std::string::String` + = note: required because of the requirements on the impl of `diesel::Queryable` for `std::string::String` + = help: see issue #48214 + = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable + +error[E0277]: the trait bound `*const str: FromSql` is not satisfied + --> tests/fail/selectable_with_typemisamatch.rs:15:9 + | +15 | id: String, + | ^^^^^^ the trait `FromSql` is not implemented for `*const str` + | + = help: the following other types implement trait `FromSql`: + <*const str as FromSql> + <*const str as FromSql> + <*const str as FromSql> + > + > + > + > + > + = note: required because of the requirements on the impl of `FromSql` for `std::string::String` + = note: required because of the requirements on the impl of `diesel::Queryable` for `std::string::String` + = help: see issue #48214 + = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable diff --git a/diesel_derives/src/selectable.rs b/diesel_derives/src/selectable.rs index 52d1ffe15a80..47281a406ce9 100644 --- a/diesel_derives/src/selectable.rs +++ b/diesel_derives/src/selectable.rs @@ -1,4 +1,5 @@ use proc_macro2::TokenStream; +use syn::spanned::Spanned; use syn::DeriveInput; use field::Field; @@ -28,9 +29,40 @@ pub fn derive(item: DeriveInput) -> TokenStream { let struct_name = &item.ident; - let field_columns_ty = model.fields().iter().map(|f| field_column_ty(f, &model)); + let field_columns_ty = model + .fields() + .iter() + .map(|f| field_column_ty(f, &model)) + .collect::>(); let field_columns_inst = model.fields().iter().map(|f| field_column_inst(f, &model)); + let field_check_bound = model + .fields() + .iter() + .zip(&field_columns_ty) + .flat_map(|(f, ty)| { + let field_ty = &f.ty; + let span = field_ty.span(); + let mut out = Vec::new(); + + if cfg!(feature = "postgres") { + out.push(quote::quote_spanned! { span => + #field_ty: diesel::prelude::Queryable, diesel::pg::Pg> + }); + } + if cfg!(feature = "sqlite") { + out.push(quote::quote_spanned! { span => + #field_ty: diesel::prelude::Queryable, diesel::sqlite::Sqlite> + }); + } + if cfg!(feature = "mysql") { + out.push(quote::quote_spanned! { span => + #field_ty: diesel::prelude::Queryable, diesel::mysql::Mysql> + }); + } + out + }); + wrap_in_dummy_mod(quote! { use diesel::expression::Selectable; @@ -44,6 +76,13 @@ pub fn derive(item: DeriveInput) -> TokenStream { (#(#field_columns_inst,)*) } } + + fn _check_field_compatibility() + where + #(#field_check_bound,)* + { + + } }) } @@ -62,8 +101,6 @@ fn field_column_ty(field: &Field, model: &Model) -> TokenStream { } fn field_column_inst(field: &Field, model: &Model) -> TokenStream { - use syn::spanned::Spanned; - if let Some(ref select_expression) = field.select_expression { let expr = &select_expression.item; let span = expr.span(); From d6d9260e386d2472719c9b7c9babe5dd9644a8bf Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Thu, 6 Oct 2022 12:37:32 +0200 Subject: [PATCH 2/5] Improve which bounds are generated for the `Selectable` check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * generate the corresponding lifetimes for `Cow<'…>` using higher ranked lifetimes * Filter out any embedded fields as they are generic over the database (they are checked on their own anyway so that should be fine) --- diesel_derives/src/selectable.rs | 83 ++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/diesel_derives/src/selectable.rs b/diesel_derives/src/selectable.rs index 47281a406ce9..c1218ba5f328 100644 --- a/diesel_derives/src/selectable.rs +++ b/diesel_derives/src/selectable.rs @@ -40,25 +40,28 @@ pub fn derive(item: DeriveInput) -> TokenStream { .fields() .iter() .zip(&field_columns_ty) + .filter(|(f, _)| !f.embed()) .flat_map(|(f, ty)| { - let field_ty = &f.ty; + let field_ty = to_field_ty_bound(&f.ty); let span = field_ty.span(); let mut out = Vec::new(); - - if cfg!(feature = "postgres") { - out.push(quote::quote_spanned! { span => - #field_ty: diesel::prelude::Queryable, diesel::pg::Pg> - }); - } - if cfg!(feature = "sqlite") { - out.push(quote::quote_spanned! { span => - #field_ty: diesel::prelude::Queryable, diesel::sqlite::Sqlite> - }); - } - if cfg!(feature = "mysql") { - out.push(quote::quote_spanned! { span => - #field_ty: diesel::prelude::Queryable, diesel::mysql::Mysql> - }); + if let Some(field_ty) = field_ty { + + if cfg!(feature = "postgres") { + out.push(quote::quote_spanned! {span=> + #field_ty: diesel::prelude::Queryable, diesel::pg::Pg> + }); + } + if cfg!(feature = "sqlite") { + out.push(quote::quote_spanned! {span=> + #field_ty: diesel::prelude::Queryable, diesel::sqlite::Sqlite> + }); + } + if cfg!(feature = "mysql") { + out.push(quote::quote_spanned! {span=> + #field_ty: diesel::prelude::Queryable, diesel::mysql::Mysql> + }); + } } out }); @@ -86,6 +89,54 @@ pub fn derive(item: DeriveInput) -> TokenStream { }) } +fn to_field_ty_bound(field_ty: &syn::Type) -> Option { + match field_ty { + syn::Type::Path(p) => { + if let syn::PathArguments::AngleBracketed(ref args) = + p.path.segments.last().unwrap().arguments + { + let lt = args + .args + .iter() + .filter_map(|f| { + if let syn::GenericArgument::Lifetime(lt) = f { + Some(lt) + } else { + None + } + }) + .collect::>(); + if lt.is_empty() { + Some(quote::quote! { + #field_ty + }) + } else if lt.len() == args.args.len() { + Some(quote::quote! { + for<#(#lt,)*> #field_ty + }) + } else { + // type parameters are not supported for checking + // for now + None + } + } else { + Some(quote::quote! { + #field_ty + }) + } + } + syn::Type::Reference(_r) => { + // references are not supported for checking for now + // + // (How ever you can even have references in a `Queryable` struct anyway) + None + } + field_ty => Some(quote::quote! { + #field_ty + }), + } +} + fn field_column_ty(field: &Field, model: &Model) -> TokenStream { if let Some(ref select_expression_type) = field.select_expression_type { let ty = &select_expression_type.item; From 136902bff0c3fff09ccf687d3b9deff98a64c396 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 24 Feb 2023 09:08:00 +0100 Subject: [PATCH 3/5] Improve the implementation * Explicitly accept for which backend the relevant code is generated, which makes this an opt-in feature and allows the use with third party backends as WELL * Add documentation --- diesel/src/deserialize.rs | 11 ++ diesel/src/query_dsl/mod.rs | 13 ++ .../tests/fail/derive/bad_primary_key.stderr | 2 + .../fail/derive/unknown_attribute.stderr | 6 +- .../fail/selectable_with_typemisamatch.rs | 19 ++- .../fail/selectable_with_typemisamatch.stderr | 113 ++---------------- diesel_derives/src/attrs.rs | 20 ++-- diesel_derives/src/lib.rs | 16 ++- diesel_derives/src/model.rs | 6 + diesel_derives/src/selectable.rs | 35 ++---- diesel_derives/src/util.rs | 18 +++ 11 files changed, 121 insertions(+), 138 deletions(-) diff --git a/diesel/src/deserialize.rs b/diesel/src/deserialize.rs index 847a1ddd6ff0..6f1a04dcf2f0 100644 --- a/diesel/src/deserialize.rs +++ b/diesel/src/deserialize.rs @@ -24,6 +24,17 @@ pub type Result = result::Result>; /// /// This trait can be [derived](derive@Queryable) /// +/// ## How to resolve compiler errors while loading data from the database +/// +/// In case you getting uncomprehensable compiler errors while loading data +/// from the database you might want to consider using +/// [`#[derive(Selectable)]`](derive@crate::prelude::Selectable) + +/// `#[diesel(check_for_backend(YourBackendType))]` to check for mismatching fields at compile +/// time. This drastically improves the quality of the generated error messages by pointing +/// to concrete mismatches at field level. You need to specify the concrete database backend +/// this specific struct is indented to be used with, as otherwise rustc cannot correctly +/// identify the required deserialization implementation. +/// /// ## Interaction with `NULL`/`Option` /// [`Nullable`][crate::sql_types::Nullable] types can be queried into `Option`. /// This is valid for single fields, tuples, and structures with `Queryable`. diff --git a/diesel/src/query_dsl/mod.rs b/diesel/src/query_dsl/mod.rs index 09dabd851dd4..2d8f011a0de7 100644 --- a/diesel/src/query_dsl/mod.rs +++ b/diesel/src/query_dsl/mod.rs @@ -1450,6 +1450,19 @@ pub trait RunQueryDsl: Sized { /// [`execute`]: crate::query_dsl::RunQueryDsl::execute() /// [`sql_query`]: crate::sql_query() /// + /// ## How to resolve compiler errors while loading data from the database + /// + /// In case you getting uncomprehensable compiler errors while loading data + /// from the database into a type using [`#[derive(Queryable)]`](derive@crate::prelude::Queryable) + /// you might want to consider + /// using [`#[derive(Selectable)]`](derive@crate::prelude::Selectable) + + /// `#[diesel(check_for_backend(YourBackendType))]` + /// to check for mismatching fields at compile time. This drastically improves + /// the quality of the generated error messages by pointing to concrete type mismatches at + /// field level.You need to specify the concrete database backend + /// this specific struct is indented to be used with, as otherwise rustc cannot correctly + /// identify the required deserialization implementation. + /// /// # Examples /// /// ## Returning a single field diff --git a/diesel_compile_tests/tests/fail/derive/bad_primary_key.stderr b/diesel_compile_tests/tests/fail/derive/bad_primary_key.stderr index 165c191c9360..47086bf33846 100644 --- a/diesel_compile_tests/tests/fail/derive/bad_primary_key.stderr +++ b/diesel_compile_tests/tests/fail/derive/bad_primary_key.stderr @@ -15,6 +15,8 @@ error: unexpected end of input, expected parentheses | 26 | #[diesel(primary_key)] | ^ + | + = help: The correct format looks like `#[diesel(key1, key2)]` error: expected parentheses --> tests/fail/derive/bad_primary_key.rs:33:22 diff --git a/diesel_compile_tests/tests/fail/derive/unknown_attribute.stderr b/diesel_compile_tests/tests/fail/derive/unknown_attribute.stderr index 7dd0305d20f6..00c25a523190 100644 --- a/diesel_compile_tests/tests/fail/derive/unknown_attribute.stderr +++ b/diesel_compile_tests/tests/fail/derive/unknown_attribute.stderr @@ -1,11 +1,11 @@ -error: unknown attribute, expected one of `aggregate`, `not_sized`, `foreign_derive`, `table_name`, `sql_type`, `treat_none_as_default_value`, `treat_none_as_null`, `belongs_to`, `mysql_type`, `sqlite_type`, `postgres_type`, `primary_key` - --> $DIR/unknown_attribute.rs:5:10 +error: unknown attribute, expected one of `aggregate`, `not_sized`, `foreign_derive`, `table_name`, `sql_type`, `treat_none_as_default_value`, `treat_none_as_null`, `belongs_to`, `mysql_type`, `sqlite_type`, `postgres_type`, `primary_key`, `check_for_backend` + --> tests/fail/derive/unknown_attribute.rs:5:10 | 5 | #[diesel(what = true)] | ^^^^ error: unknown attribute, expected one of `embed`, `column_name`, `sql_type`, `serialize_as`, `deserialize_as`, `select_expression`, `select_expression_type` - --> $DIR/unknown_attribute.rs:12:14 + --> tests/fail/derive/unknown_attribute.rs:12:14 | 12 | #[diesel(what = true)] | ^^^^ diff --git a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs index b745bcd79fe6..d57365a5e0da 100644 --- a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs +++ b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs @@ -11,16 +11,29 @@ table! { #[derive(Selectable, Queryable)] #[diesel(table_name = users)] +#[diesel(check_for_backend(diesel::pg::Pg))] struct User { id: String, name: i32, } +#[derive(Selectable, Queryable)] +#[diesel(table_name = users)] +#[diesel(check_for_backend(diesel::pg::Pg))] +struct UserCorrect { + id: i32, + name: String, +} fn main() { let mut conn = PgConnection::establish("...").unwrap(); - users::table.select(User::as_select()).load(&mut conn).unwrap(); - - + users::table + .select(User::as_select()) + .load(&mut conn) + .unwrap(); + users::table + .select(UserCorrect::as_select()) + .load(&mut conn) + .unwrap(); } diff --git a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr index 8d65851babbb..69b64b2dff54 100644 --- a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr +++ b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr @@ -1,119 +1,30 @@ -error[E0277]: the trait bound `i32: FromSql` is not satisfied - --> tests/fail/selectable_with_typemisamatch.rs:16:11 - | -16 | name: i32, - | ^^^ the trait `FromSql` is not implemented for `i32` - | - = help: the following other types implement trait `FromSql`: - > - > - > - > - > - > - > - > - and 13 others - = note: required because of the requirements on the impl of `diesel::Queryable` for `i32` - = help: see issue #48214 - = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable - -error[E0277]: the trait bound `i32: FromSql` is not satisfied - --> tests/fail/selectable_with_typemisamatch.rs:16:11 - | -16 | name: i32, - | ^^^ the trait `FromSql` is not implemented for `i32` - | - = help: the following other types implement trait `FromSql`: - > - > - > - > - > - > - > - > - and 13 others - = note: required because of the requirements on the impl of `diesel::Queryable` for `i32` - = help: see issue #48214 - = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable - error[E0277]: the trait bound `i32: FromSql` is not satisfied - --> tests/fail/selectable_with_typemisamatch.rs:16:11 + --> tests/fail/selectable_with_typemisamatch.rs:17:11 | -16 | name: i32, +17 | name: i32, | ^^^ the trait `FromSql` is not implemented for `i32` | = help: the following other types implement trait `FromSql`: - > - > - > - > - > - > - > - > - and 13 others - = note: required because of the requirements on the impl of `diesel::Queryable` for `i32` - = help: see issue #48214 - = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable - -error[E0277]: the trait bound `*const str: FromSql` is not satisfied - --> tests/fail/selectable_with_typemisamatch.rs:15:9 - | -15 | id: String, - | ^^^^^^ the trait `FromSql` is not implemented for `*const str` - | - = help: the following other types implement trait `FromSql`: - <*const str as FromSql> - <*const str as FromSql> - <*const str as FromSql> - > - > - > - > - > - = note: required because of the requirements on the impl of `FromSql` for `std::string::String` - = note: required because of the requirements on the impl of `diesel::Queryable` for `std::string::String` - = help: see issue #48214 - = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable - -error[E0277]: the trait bound `*const str: FromSql` is not satisfied - --> tests/fail/selectable_with_typemisamatch.rs:15:9 - | -15 | id: String, - | ^^^^^^ the trait `FromSql` is not implemented for `*const str` - | - = help: the following other types implement trait `FromSql`: - <*const str as FromSql> - <*const str as FromSql> - <*const str as FromSql> - > - > - > - > - > - = note: required because of the requirements on the impl of `FromSql` for `std::string::String` - = note: required because of the requirements on the impl of `diesel::Queryable` for `std::string::String` + > + > + > + = note: required for `i32` to implement `diesel::Queryable` + = note: required for `i32` to implement `FromSqlRow` = help: see issue #48214 = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable error[E0277]: the trait bound `*const str: FromSql` is not satisfied - --> tests/fail/selectable_with_typemisamatch.rs:15:9 + --> tests/fail/selectable_with_typemisamatch.rs:16:9 | -15 | id: String, +16 | id: String, | ^^^^^^ the trait `FromSql` is not implemented for `*const str` | = help: the following other types implement trait `FromSql`: <*const str as FromSql> <*const str as FromSql> <*const str as FromSql> - > - > - > - > - > - = note: required because of the requirements on the impl of `FromSql` for `std::string::String` - = note: required because of the requirements on the impl of `diesel::Queryable` for `std::string::String` + = note: required for `std::string::String` to implement `FromSql` + = note: required for `std::string::String` to implement `diesel::Queryable` + = note: required for `std::string::String` to implement `FromSqlRow` = help: see issue #48214 = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable diff --git a/diesel_derives/src/attrs.rs b/diesel_derives/src/attrs.rs index 9da55c748dea..414785f1cf35 100644 --- a/diesel_derives/src/attrs.rs +++ b/diesel_derives/src/attrs.rs @@ -8,7 +8,7 @@ use syn::parse::discouraged::Speculative; use syn::parse::{Parse, ParseStream, Parser, Result}; use syn::punctuated::Punctuated; use syn::token::Comma; -use syn::{parenthesized, Attribute, Ident, LitBool, LitStr, Path, Type, TypePath}; +use syn::{Attribute, Ident, LitBool, LitStr, Path, Type, TypePath}; use deprecated::ParseDeprecated; use parsers::{BelongsTo, MysqlType, PostgresType, SqliteType}; @@ -20,6 +20,7 @@ use util::{ }; use crate::field::SelectExpr; +use crate::util::{parse_paren_list, CHECK_FOR_BACKEND_NOTE}; pub struct AttributeSpanWrapper { pub item: T, @@ -123,7 +124,6 @@ impl Parse for FieldAttr { name, parse_eq(input, SELECT_EXPRESSION_TYPE_NOTE)?, )), - _ => unknown_attribute( &name, &[ @@ -170,6 +170,7 @@ pub enum StructAttr { SqliteType(Ident, SqliteType), PostgresType(Ident, PostgresType), PrimaryKey(Ident, Punctuated), + CheckForBackend(Ident, syn::punctuated::Punctuated), } impl Parse for StructAttr { @@ -212,11 +213,14 @@ impl Parse for StructAttr { name, parse_paren(input, POSTGRES_TYPE_NOTE)?, )), - "primary_key" => Ok(StructAttr::PrimaryKey(name, { - let content; - parenthesized!(content in input); - content.parse_terminated(Ident::parse)? - })), + "primary_key" => Ok(StructAttr::PrimaryKey( + name, + parse_paren_list(input, "key1, key2")?, + )), + "check_for_backend" => Ok(StructAttr::CheckForBackend( + name, + parse_paren_list(input, CHECK_FOR_BACKEND_NOTE)?, + )), _ => unknown_attribute( &name, @@ -233,6 +237,7 @@ impl Parse for StructAttr { "sqlite_type", "postgres_type", "primary_key", + "check_for_backend", ], ), } @@ -253,6 +258,7 @@ impl Spanned for StructAttr { | StructAttr::MysqlType(ident, _) | StructAttr::SqliteType(ident, _) | StructAttr::PostgresType(ident, _) + | StructAttr::CheckForBackend(ident, _) | StructAttr::PrimaryKey(ident, _) => ident.span(), } } diff --git a/diesel_derives/src/lib.rs b/diesel_derives/src/lib.rs index 9a84137666b3..bea281e47f72 100644 --- a/diesel_derives/src/lib.rs +++ b/diesel_derives/src/lib.rs @@ -1,4 +1,3 @@ -#![recursion_limit = "1024"] // Clippy lints #![allow( clippy::needless_doctest_main, @@ -455,7 +454,10 @@ pub fn derive_query_id(input: TokenStream) -> TokenStream { /// **Note**: When this trait is derived, it will assume that __all fields on /// your struct__ matches __all fields in the query__, including the order and /// count. This means that field order is significant if you are using -/// `#[derive(Queryable)]`. __Field name has no effect__. +/// `#[derive(Queryable)]`. __Field name has no effect__. If you see errors while +/// loading data into a struct that derives `Queryable`: Consider using [`#[derive(Selectable)]`] +/// + `#[diesel(check_for_backend(YourBackendType))]` to check for mismatching fields at compile +/// time. /// /// To provide custom deserialization behavior for a field, you can use /// `#[diesel(deserialize_as = SomeType)]`. If this attribute is present, Diesel @@ -844,6 +846,16 @@ pub fn derive_queryable_by_name(input: TokenStream) -> TokenStream { /// If this attribute is not used, the type name converted to /// `snake_case` with an added `s` is used as table name. /// +/// ## Optional Type attributes +/// +/// * `#[diesel(check_for_backend(diesel::pg::Pg, diesel::mysql::Mysql))]`, instructs +/// the derive to generate additional code to identify potential type mismatches. +/// It accepts a list of backend types to check the types against. Using this option +/// will result in much better error messages in cases where some types in your `Queryable` +/// struct do not match. You need to specify the concrete database backend +/// this specific struct is indented to be used with, as otherwise rustc cannot correctly +/// identify the required deserialization implementation. +/// /// ## Field attributes /// /// * `#[diesel(column_name = some_column)]`, overrides the column name for diff --git a/diesel_derives/src/model.rs b/diesel_derives/src/model.rs index 0b10247e1951..847fb80049af 100644 --- a/diesel_derives/src/model.rs +++ b/diesel_derives/src/model.rs @@ -25,6 +25,7 @@ pub struct Model { pub mysql_type: Option, pub sqlite_type: Option, pub postgres_type: Option, + pub check_for_backend: Option>, fields: Vec, } @@ -61,6 +62,7 @@ impl Model { let mut mysql_type = None; let mut sqlite_type = None; let mut postgres_type = None; + let mut check_for_backend = None; for attr in parse_attributes(attrs) { match attr.item { @@ -80,6 +82,9 @@ impl Model { StructAttr::MysqlType(_, val) => mysql_type = Some(val), StructAttr::SqliteType(_, val) => sqlite_type = Some(val), StructAttr::PostgresType(_, val) => postgres_type = Some(val), + StructAttr::CheckForBackend(_, b) => { + check_for_backend = Some(b); + } } } @@ -100,6 +105,7 @@ impl Model { sqlite_type, postgres_type, fields: fields_from_item_data(fields), + check_for_backend, } } diff --git a/diesel_derives/src/selectable.rs b/diesel_derives/src/selectable.rs index c1218ba5f328..ff54b70c0243 100644 --- a/diesel_derives/src/selectable.rs +++ b/diesel_derives/src/selectable.rs @@ -42,28 +42,19 @@ pub fn derive(item: DeriveInput) -> TokenStream { .zip(&field_columns_ty) .filter(|(f, _)| !f.embed()) .flat_map(|(f, ty)| { - let field_ty = to_field_ty_bound(&f.ty); - let span = field_ty.span(); - let mut out = Vec::new(); - if let Some(field_ty) = field_ty { - - if cfg!(feature = "postgres") { - out.push(quote::quote_spanned! {span=> - #field_ty: diesel::prelude::Queryable, diesel::pg::Pg> - }); - } - if cfg!(feature = "sqlite") { - out.push(quote::quote_spanned! {span=> - #field_ty: diesel::prelude::Queryable, diesel::sqlite::Sqlite> - }); - } - if cfg!(feature = "mysql") { - out.push(quote::quote_spanned! {span=> - #field_ty: diesel::prelude::Queryable, diesel::mysql::Mysql> - }); - } - } - out + model + .check_for_backend + .as_ref() + .into_iter() + .flat_map(move |d| { + let field_ty = to_field_ty_bound(&f.ty); + let span = field_ty.span(); + d.iter().map(move |b| { + quote::quote_spanned! {span => + #field_ty: diesel::deserialize::FromSqlRow, #b> + } + }) + }) }); wrap_in_dummy_mod(quote! { diff --git a/diesel_derives/src/util.rs b/diesel_derives/src/util.rs index 3ac60510cca1..42eeddb4c029 100644 --- a/diesel_derives/src/util.rs +++ b/diesel_derives/src/util.rs @@ -21,6 +21,7 @@ pub const SELECT_EXPRESSION_NOTE: &str = "select_expression = schema::table_name::column_name.is_not_null()"; pub const SELECT_EXPRESSION_TYPE_NOTE: &str = "select_expression_type = dsl::IsNotNull"; +pub const CHECK_FOR_BACKEND_NOTE: &str = "diesel::pg::Pg"; pub fn unknown_attribute(name: &Ident, valid: &[&str]) -> ! { let prefix = if valid.len() == 1 { "" } else { " one of" }; @@ -60,6 +61,23 @@ pub fn parse_paren(input: ParseStream, help: &str) -> Result { content.parse() } +pub fn parse_paren_list( + input: ParseStream, + help: &str, +) -> Result> { + if input.is_empty() { + abort!( + input.span(), + "unexpected end of input, expected parentheses"; + help = "The correct format looks like `#[diesel({})]`", help + ); + } + + let content; + parenthesized!(content in input); + content.parse_terminated(T::parse) +} + pub fn wrap_in_dummy_mod(item: TokenStream) -> TokenStream { quote! { #[allow(unused_imports)] From e179d711c5d3296bb9096e356b1ba6e3f2706171 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 24 Feb 2023 14:10:36 +0100 Subject: [PATCH 4/5] Do not generate the check function if not required --- diesel_derives/src/selectable.rs | 47 ++++++++++++++++---------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/diesel_derives/src/selectable.rs b/diesel_derives/src/selectable.rs index ff54b70c0243..d799d711ae82 100644 --- a/diesel_derives/src/selectable.rs +++ b/diesel_derives/src/selectable.rs @@ -36,26 +36,32 @@ pub fn derive(item: DeriveInput) -> TokenStream { .collect::>(); let field_columns_inst = model.fields().iter().map(|f| field_column_inst(f, &model)); - let field_check_bound = model - .fields() - .iter() - .zip(&field_columns_ty) - .filter(|(f, _)| !f.embed()) - .flat_map(|(f, ty)| { - model - .check_for_backend - .as_ref() - .into_iter() - .flat_map(move |d| { + let check_function = if let Some(ref backends) = model.check_for_backend { + let field_check_bound = model + .fields() + .iter() + .zip(&field_columns_ty) + .filter(|(f, _)| !f.embed()) + .flat_map(|(f, ty)| { + backends.iter().map(move |b| { let field_ty = to_field_ty_bound(&f.ty); let span = field_ty.span(); - d.iter().map(move |b| { - quote::quote_spanned! {span => - #field_ty: diesel::deserialize::FromSqlRow, #b> - } - }) + quote::quote_spanned! {span => + #field_ty: diesel::deserialize::FromSqlRow, #b> + } }) - }); + }); + Some(quote::quote! { + fn _check_field_compatibility() + where + #(#field_check_bound,)* + { + + } + }) + } else { + None + }; wrap_in_dummy_mod(quote! { use diesel::expression::Selectable; @@ -71,12 +77,7 @@ pub fn derive(item: DeriveInput) -> TokenStream { } } - fn _check_field_compatibility() - where - #(#field_check_bound,)* - { - - } + #check_function }) } From 5b0e7893eccb5ae3816268cd958aae6098b31dda Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Wed, 1 Mar 2023 09:13:47 +0100 Subject: [PATCH 5/5] Another error message improvement --- diesel/src/deserialize.rs | 7 +++++++ .../fail/derive_deprecated/deprecated_sql_type.stderr | 3 ++- .../fail/select_carries_correct_result_type_info.stderr | 6 ++++-- .../tests/fail/select_sql_still_ensures_result_type.stderr | 3 ++- .../tests/fail/selectable_with_typemisamatch.stderr | 6 ++++-- 5 files changed, 19 insertions(+), 6 deletions(-) diff --git a/diesel/src/deserialize.rs b/diesel/src/deserialize.rs index 6f1a04dcf2f0..f5ad09a1a5b7 100644 --- a/diesel/src/deserialize.rs +++ b/diesel/src/deserialize.rs @@ -420,6 +420,13 @@ pub use diesel_derives::QueryableByName; /// } /// } /// ``` +#[cfg_attr( + feature = "nightly-error-messages", + rustc_on_unimplemented( + message = "Cannot deserialize a value of the type `{A}` as `{Self}`", + note = "Double check your type mappings via the documentation of `{A}`" + ) +)] pub trait FromSql: Sized { /// See the trait documentation. fn from_sql(bytes: DB::RawValue<'_>) -> Result; diff --git a/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr b/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr index 4e2698e42dc5..f29a779f4d04 100644 --- a/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr +++ b/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr @@ -73,12 +73,13 @@ error[E0412]: cannot find type `foo` in this scope 27 | #[sql_type = "foo"] | ^^^^^ not found in this scope -error[E0277]: the trait bound `i32: FromSql<_, __DB>` is not satisfied +error[E0277]: Cannot deserialize a value of the type `_` as `i32` --> tests/fail/derive_deprecated/deprecated_sql_type.rs:25:10 | 25 | #[derive(QueryableByName)] | ^^^^^^^^^^^^^^^ the trait `FromSql<_, __DB>` is not implemented for `i32` | + = note: Double check your type mappings via the documentation of `_` = help: the following other types implement trait `FromSql`: > > diff --git a/diesel_compile_tests/tests/fail/select_carries_correct_result_type_info.stderr b/diesel_compile_tests/tests/fail/select_carries_correct_result_type_info.stderr index d686145a3691..504cf86a0419 100644 --- a/diesel_compile_tests/tests/fail/select_carries_correct_result_type_info.stderr +++ b/diesel_compile_tests/tests/fail/select_carries_correct_result_type_info.stderr @@ -1,4 +1,4 @@ -error[E0277]: the trait bound `i32: FromSql` is not satisfied +error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Text` as `i32` --> tests/fail/select_carries_correct_result_type_info.rs:19:39 | 19 | let ids = select_name.load::(&mut connection); @@ -6,6 +6,7 @@ error[E0277]: the trait bound `i32: FromSql` is not | | | required by a bound introduced by this call | + = note: Double check your type mappings via the documentation of `diesel::sql_types::Text` = help: the following other types implement trait `FromSql`: > > @@ -20,7 +21,7 @@ note: required by a bound in `diesel::RunQueryDsl::load` | Self: LoadQuery<'query, Conn, U>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `RunQueryDsl::load` -error[E0277]: the trait bound `*const str: FromSql` is not satisfied +error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Integer` as `*const str` --> tests/fail/select_carries_correct_result_type_info.rs:20:42 | 20 | let names = select_id.load::(&mut connection); @@ -28,6 +29,7 @@ error[E0277]: the trait bound `*const str: FromSql`: <*const str as FromSql> <*const str as FromSql> diff --git a/diesel_compile_tests/tests/fail/select_sql_still_ensures_result_type.stderr b/diesel_compile_tests/tests/fail/select_sql_still_ensures_result_type.stderr index 9fd7499686f4..0c0802d0820c 100644 --- a/diesel_compile_tests/tests/fail/select_sql_still_ensures_result_type.stderr +++ b/diesel_compile_tests/tests/fail/select_sql_still_ensures_result_type.stderr @@ -1,4 +1,4 @@ -error[E0277]: the trait bound `*const str: FromSql` is not satisfied +error[E0277]: Cannot deserialize a value of the type `BigInt` as `*const str` --> tests/fail/select_sql_still_ensures_result_type.rs:16:51 | 16 | let count = select_count.get_result::(&mut connection).unwrap(); @@ -6,6 +6,7 @@ error[E0277]: the trait bound `*const str: FromSql` is not satisfied | | | required by a bound introduced by this call | + = note: Double check your type mappings via the documentation of `BigInt` = help: the following other types implement trait `FromSql`: <*const str as FromSql> <*const str as FromSql> diff --git a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr index 69b64b2dff54..e167f6f38fd6 100644 --- a/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr +++ b/diesel_compile_tests/tests/fail/selectable_with_typemisamatch.stderr @@ -1,9 +1,10 @@ -error[E0277]: the trait bound `i32: FromSql` is not satisfied +error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Text` as `i32` --> tests/fail/selectable_with_typemisamatch.rs:17:11 | 17 | name: i32, | ^^^ the trait `FromSql` is not implemented for `i32` | + = note: Double check your type mappings via the documentation of `diesel::sql_types::Text` = help: the following other types implement trait `FromSql`: > > @@ -13,12 +14,13 @@ error[E0277]: the trait bound `i32: FromSql` is not = help: see issue #48214 = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable -error[E0277]: the trait bound `*const str: FromSql` is not satisfied +error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Integer` as `*const str` --> tests/fail/selectable_with_typemisamatch.rs:16:9 | 16 | id: String, | ^^^^^^ the trait `FromSql` is not implemented for `*const str` | + = note: Double check your type mappings via the documentation of `diesel::sql_types::Integer` = help: the following other types implement trait `FromSql`: <*const str as FromSql> <*const str as FromSql>