Skip to content

Commit

Permalink
Rewrite #[derive(QueryableByName)] in derives2
Browse files Browse the repository at this point in the history
Since `QueryableByName` is one of the more recently written derives, it
should have been a really straightforward port. Unfortunately, the
tests for this derive hit multiple rustc bugs

- rust-lang/rust#47983
- rust-lang/rust#47311

I love what we were able to do with the error message here. We could
even go so far as to have the `help` lines point at the struct itself
for the `table_name` annotation if we want to.

I also much prefer the workaround for
rust-lang/rust#47311 in this PR to the one I
did in #1529. I'll need to update that PR if this is merged first.
  • Loading branch information
sgrif committed Feb 3, 2018
1 parent 14887fc commit 8f48569
Show file tree
Hide file tree
Showing 17 changed files with 245 additions and 128 deletions.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#[macro_use] extern crate diesel;

#[derive(QueryableByName)]
struct Foo {
foo: i32,
bar: String,
}

#[derive(QueryableByName)]
struct Bar(i32, String);

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
error: Cannot determine the SQL type of foo
--> $DIR/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:5:5
|
5 | foo: i32,
| ^^^
|
= help: Your struct must either be annotated with `#[table_name = "foo"]` or have all of its fields annotated with `#[sql_type = "Integer"]`

error: Cannot determine the SQL type of bar
--> $DIR/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:6:5
|
6 | bar: String,
| ^^^
|
= help: Your struct must either be annotated with `#[table_name = "foo"]` or have all of its fields annotated with `#[sql_type = "Integer"]`

error: All fields of tuple structs must be annotated with `#[column_name]`
--> $DIR/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:10:12
|
10 | struct Bar(i32, String);
| ^^^

error: Cannot determine the SQL type of field
--> $DIR/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:10:12
|
10 | struct Bar(i32, String);
| ^^^
|
= help: Your struct must either be annotated with `#[table_name = "foo"]` or have all of its fields annotated with `#[sql_type = "Integer"]`

error: All fields of tuple structs must be annotated with `#[column_name]`
--> $DIR/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:10:17
|
10 | struct Bar(i32, String);
| ^^^^^^

error: Cannot determine the SQL type of field
--> $DIR/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:10:17
|
10 | struct Bar(i32, String);
| ^^^^^^
|
= help: Your struct must either be annotated with `#[table_name = "foo"]` or have all of its fields annotated with `#[sql_type = "Integer"]`

error: aborting due to 6 previous errors

12 changes: 0 additions & 12 deletions diesel_derives/src/attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,6 @@ impl Attr {
)
}

pub fn sql_type(&self) -> Option<&syn::Ty> {
self.sql_type.as_ref()
}

pub fn has_flag<T>(&self, flag: &T) -> bool
where
T: ?Sized,
syn::Ident: PartialEq<T>,
{
self.flags.iter().any(|f| f == flag)
}

fn field_kind(&self) -> &str {
if is_option_ty(&self.ty) {
"option"
Expand Down
6 changes: 0 additions & 6 deletions diesel_derives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ mod insertable;
mod model;
mod query_id;
mod queryable;
mod queryable_by_name;
mod sql_type;
mod util;

Expand All @@ -45,11 +44,6 @@ pub fn derive_queryable(input: TokenStream) -> TokenStream {
expand_derive(input, queryable::derive_queryable)
}

#[proc_macro_derive(QueryableByName, attributes(table_name, column_name, sql_type, diesel))]
pub fn derive_queryable_by_name(input: TokenStream) -> TokenStream {
expand_derive(input, queryable_by_name::derive)
}

#[proc_macro_derive(Insertable, attributes(table_name, column_name))]
pub fn derive_insertable(input: TokenStream) -> TokenStream {
expand_derive(input, insertable::derive_insertable)
Expand Down
76 changes: 0 additions & 76 deletions diesel_derives/src/queryable_by_name.rs

This file was deleted.

1 change: 0 additions & 1 deletion diesel_derives/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,5 @@ extern crate diesel;
extern crate diesel_derives;

mod queryable;
mod queryable_by_name;
mod associations;
mod test_helpers;
2 changes: 1 addition & 1 deletion diesel_derives2/src/as_changeset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ fn field_changeset_expr(
table_name: syn::Ident,
treat_none_as_null: bool,
) -> syn::Expr {
let field_access = &field.name;
let field_access = field.name.access();
let column_name = field.column_name();
if !treat_none_as_null && is_option_ty(&field.ty) {
parse_quote!(self#field_access.as_ref().map(|x| #table_name::#column_name.eq(x)))
Expand Down
56 changes: 44 additions & 12 deletions diesel_derives2/src/field.rs
Original file line number Diff line number Diff line change
@@ -1,34 +1,49 @@
use proc_macro2::Span;
use quote;
use syn;
use syn::spanned::Spanned;
use syn;

use diagnostic_shim::*;
use meta::*;
use util::*;

pub struct Field {
pub ty: syn::Type,
pub name: FieldName,
pub span: Span,
pub sql_type: Option<syn::Type>,
column_name_from_attribute: Option<syn::Ident>,
flags: MetaItem,
}

impl Field {
pub fn from_struct_field(field: &syn::Field, index: usize) -> Self {
let column_name_from_attribute =
MetaItem::with_name(&field.attrs, "column_name").map(|m| m.expect_ident_value());
let name = match field.ident {
Some(x) => FieldName::Named(x),
Some(mut x) => {
// https://github.com/rust-lang/rust/issues/47983#issuecomment-362817105
x.span = fix_span(x.span, Span::call_site());
FieldName::Named(x)
}
None => FieldName::Unnamed(syn::Index {
index: index as u32,
// https://github.com/rust-lang/rust/issues/47312
span: Span::call_site(),
}),
};
let sql_type = MetaItem::with_name(&field.attrs, "sql_type")
.and_then(|m| m.ty_value().map_err(Diagnostic::emit).ok());
let flags = MetaItem::with_name(&field.attrs, "diesel")
.unwrap_or_else(|| MetaItem::empty("diesel"));
let span = field.span();

Self {
ty: field.ty.clone(),
column_name_from_attribute,
name,
sql_type,
flags,
span,
}
}

Expand All @@ -37,8 +52,7 @@ impl Field {
.unwrap_or_else(|| match self.name {
FieldName::Named(x) => x,
_ => {
self.ty
.span()
self.span
.error(
"All fields of tuple structs must be annotated with `#[column_name]`",
)
Expand All @@ -47,22 +61,40 @@ impl Field {
}
})
}

pub fn has_flag(&self, flag: &str) -> bool {
self.flags.has_flag(flag)
}
}

pub enum FieldName {
Named(syn::Ident),
Unnamed(syn::Index),
}

impl FieldName {
pub fn assign(&self, expr: syn::Expr) -> syn::FieldValue {
let span = self.span();
// Parens are to work around https://github.com/rust-lang/rust/issues/47311
let tokens = quote_spanned!(span=> #self: (#expr));
parse_quote!(#tokens)
}

pub fn access(&self) -> quote::Tokens {
let span = self.span();
quote_spanned!(span=> .#self)
}

pub fn span(&self) -> Span {
match *self {
FieldName::Named(x) => x.span,
FieldName::Unnamed(ref x) => x.span,
}
}
}

impl quote::ToTokens for FieldName {
fn to_tokens(&self, tokens: &mut quote::Tokens) {
use proc_macro2::{Spacing, TokenNode, TokenTree};

// https://github.com/rust-lang/rust/issues/47312
tokens.append(TokenTree {
span: Span::call_site(),
kind: TokenNode::Op('.', Spacing::Alone),
});
match *self {
FieldName::Named(x) => x.to_tokens(tokens),
FieldName::Unnamed(ref x) => x.to_tokens(tokens),
Expand Down
2 changes: 1 addition & 1 deletion diesel_derives2/src/identifiable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn derive(item: syn::DeriveInput) -> Result<quote::Tokens, Diagnostic> {
.primary_key_names
.iter()
.filter_map(|&pk| model.find_column(pk).map_err(Diagnostic::emit).ok())
.map(|f| (&f.ty, &f.name))
.map(|f| (&f.ty, f.name.access()))
.unzip();

Ok(wrap_in_dummy_mod(
Expand Down
6 changes: 6 additions & 0 deletions diesel_derives2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ mod util;

mod as_changeset;
mod identifiable;
mod queryable_by_name;

use diagnostic_shim::*;

Expand All @@ -42,6 +43,11 @@ pub fn derive_identifiable(input: TokenStream) -> TokenStream {
expand_derive(input, identifiable::derive)
}

#[proc_macro_derive(QueryableByName, attributes(table_name, column_name, sql_type, diesel))]
pub fn derive_queryable_by_name(input: TokenStream) -> TokenStream {
expand_derive(input, queryable_by_name::derive)
}

fn expand_derive(
input: TokenStream,
f: fn(syn::DeriveInput) -> Result<quote::Tokens, Diagnostic>,
Expand Down
Loading

0 comments on commit 8f48569

Please sign in to comment.