WIP Macros 1.1 port #453

sgrif · 2016-09-30T21:03:17Z

This pull request is incomplete (unfortunately it will need to be all one pull request as I don't want a ton of git churn from crate renaming, and I won't be able to get the build in a passing state otherwise.

However, enough is done that I'd like some feedback on the overall structure before I finish this. /cc @killercup

This makes our API compatible with something that can be represented in Macros 1.1

In the macros 1.1 system, the entire struct body is given as the input and is expected to be in the output. This is to allow the annotations to modify the struct as needed. If the derive uses any custom annotations besides just the derive itself, it is the responsibility of that macro to strip those attributes. However, our story is a bit more complex, as we end up sharing these attributes between multiple traits where more than one can be derived on the same struct. So to work around this, we need to keep a list of all the traits that we might be deriving, check if any are left, and if we're the last one do the clean up. Because we're good citizens, we want to leave any attributes that aren't ours.

The addition of `..` in the stable macro is to make sure that we generate a valid pattern for matching the struct when we filter out the `id` on the proc macro side. The old proc macro was not using the stable macro. The stable macro does not filter out the `id` column, and it was non-trivial to add. This does not work if there is a tuple struct with the column name `id`, but I'm not concerned about this case right now.

sgrif · 2016-09-30T21:04:56Z

Gah the insertable commit got pulled in here and I can't make it go away

killercup

Great to see this coming together nicely! Looks good overall :) I've reviewed this commit by commit so there are a few inline comments on outdated diffs that don't really matter.

Which versions of syntex/nightly does this target? Travis doesn't seem to like it with syntex, and the nightly doesn't seem to like the compiletest version.

killercup · 2016-10-01T11:22:00Z

diesel_codegen_new/src/attr.rs

+        let column_name = ident_value_of_attr_with_name(&field.attrs, "column_name")
+            .map(Clone::clone)
+            .or_else(|| field_name.clone());
+        let ty = field.ty.clone();


Wow, there's a lot of cloning going on in these lines. I don't have any concrete advice to reduce that right now, just seems like a bad smell.

killercup · 2016-10-01T11:31:54Z

diesel_codegen_new/src/attr.rs

+    }
+}
+
+impl quote::ToTokens for Attr {


Memo to self: This impl is used by quote! which calls Tokens::append_separated which calls ToTokens::to_tokens

killercup · 2016-10-01T11:33:46Z

diesel_codegen_new/src/lib.rs

+    let input = input.to_string();
+    let input = input.replace("#[structural_match]", "");
+
+    let item = parse_item(&input);


~~I guess these first three calls are needed for every derive_* extension; would it make sense to put them into a separate fn?~~ Never mind you abstracted this to be a HOF instead

killercup · 2016-10-01T11:35:22Z

diesel_codegen_new/src/lib.rs

+    let model = match Model::from_item(&item) {
+        Ok(m) => m,
+        Err(e) => panic!("#[derive(Queryable)] {}", e),
+    };


I think this match generates the same output as .expect("#[derive(Queryable)]") since e is just a String

No, wait, I guess you were aiming for

#[derive(Queryable)] cannot be used with enums

(maybe add the backticks, though)

killercup · 2016-10-01T11:51:00Z

diesel_codegen_new/src/model.rs

+            result.push(character);
+        }
+    }
+    result


Maybe use the inflections crate instead and reduce this to name[..1].to_string().to_pascal_case()?

Inflector is an alternative crate that has to_singular, but no pascal case yet (cf. whatisinternet/Inflector#27)

This function is just a straight copy paste from the old crate. I'd like to avoid having an inflector beyond the absolute simplest cases. It's been a major source of pain in Rails, as it's brittle, will never get everything right, and is impossible to change without breaking people's apps.

Also admit it you just want to have a function called to_pascal_case

Interesting. I didn't know Rails had problems with that, but I can totally see that happening. Makes sense to keep this really simple; if it doesn't work you can just set the table name explicitly anyway.

Also admit it you just want to have a function called to_pascal_case

Haha, you got me 😉 (I usually call it UpperCamelCase because it just sounds weird to me otherwise… "Huh, is anyone talking to me? Aww, no, they're still discussion string casing…")

dtolnay

Let me know if you have any suggestions for syn or quote that would have made this more pleasant.

dtolnay · 2016-10-02T16:01:02Z

diesel_codegen_new/src/lib.rs

+fn expand_derive(input: TokenStream, f: fn(syn::Item) -> quote::Tokens) -> TokenStream {
+    let input = input.to_string();
+    // FIXME: https://github.com/rust-lang/rust/issues/35900#issuecomment-245971366
+    let input = input.replace("#[structural_match]", "");


This was fixed upstream in rust-lang/rust#36782 and the workaround is no longer necessary.

dtolnay · 2016-10-02T16:13:46Z

diesel_codegen_new/src/as_changeset.rs

+            }
+            match values[0] {
+                syn::MetaItem::NameValue(ref name, ref value)
+                    if name.as_ref() == "treat_none_as_null" => value == "true",


I added an impl<T: ?Sized> PartialEq<T> for Ident where T: AsRef<str> in syn 0.5.0 so the explicit as_ref() will no longer be necessary.

dtolnay · 2016-10-02T16:14:57Z

diesel_codegen_new/src/as_changeset.rs

+    let usage_err = || panic!(r#"`#[changeset_options]` must be in the form \
+        `#[changeset_options(treat_none_as_null = "true")]`"#);
+
+    match options_attr.value {


I filed dtolnay/syn#25 to make attribute parsing less annoying.

dtolnay · 2016-10-02T16:19:17Z

diesel_codegen_new/src/as_changeset.rs

+        .collect::<Vec<_>>();
+
+    if lifetimes.is_empty() {
+        lifetimes.push(syn::LifetimeDef {


I added syn::LifetimeDef::new("'a") in syn 0.8.5 to simplify this a bit.

dtolnay · 2016-10-02T16:26:01Z

diesel_codegen_new/src/ast_builder.rs

@@ -0,0 +1,16 @@
+use syn::{Ident, Ty, Path, PathSegment};
+
+pub fn ty_ident(ident: Ident) -> Ty {


Do you think it makes sense to add these upstream as constructors? syn::Ty::ident(i), syn::Ty::path(p), syn::Path::ident(i) or something like that. And should the ident ones take Ident or Into<Ident> so they can be called with strings?

I think it makes sense for syn to only provide the AST definitions and parsing, and having a separate crate for the AST builder. I definitely think that abstraction needs to exist. I'm not confident enough in the usage patterns that have emerged to really find the right abstractions yet though.

dtolnay · 2016-10-02T16:41:39Z

diesel_codegen_new/src/lib.rs

+    // FIXME: https://github.com/rust-lang/rust/issues/35900#issuecomment-245971366
+    let input = input.replace("#[structural_match]", "");
+
+    let mut item = parse_item(&input);


I changed this to parse_macro_input in syn 0.6.0 which handles only structs and enums, as opposed to the entire array of items (functions, modules, typedefs, statics, extern crates, traits, impls, etc). The parse_item is behind a feature flag because it compiles a bit slower and most people don't need it.

dtolnay · 2016-10-02T16:44:48Z

diesel_codegen_new/src/lib.rs

+    }
+    let input = quote!(#item);
+
+    TokenStream::from_str(&format!("{} {}", input, output)).unwrap()


Or equivalently, quote!(#item #output).to_string().parse().unwrap().

dtolnay · 2016-10-02T16:51:18Z

diesel_codegen_new/src/util.rs

+}
+
+pub fn attr_name(attr: &Attribute) -> &Ident {
+    match attr.value {


I added attr.value.name() -> &str in syn 0.7.1 and attr.name() -> &str in 0.8.5.

This was extracted from #453. Going forward Macros 1.1 is the intended path of stabilization for procedural macros, so `diesel_codegen` will need to be rewritten to use it. Much of the helper code around this is a direct port of the libsyntax version of our code, rewritten to use `syn` instead.

…g Macros 1.1 This was extracted from #453. Going forward Macros 1.1 is the intended path of stabilization for procedural macros, so `diesel_codegen` will need to be rewritten to use it. Much of the helper code around this is a direct port of the libsyntax version of our code, rewritten to use `syn` instead. Close #453.

sgrif added 8 commits September 30, 2016 11:52

Change #[insertable_into] to #[derive(Insertable)]

ef79f64

This makes our API compatible with something that can be represented in Macros 1.1

Port queryable over

a76262f

Slightly better abstraction

177c2f6

Port over Identifiable

1fd8262

Port over Insertable

15cf2ac

Update some code from the rebase to the latest nightly

f56a29c

killercup reviewed Oct 1, 2016

View reviewed changes

dtolnay reviewed Oct 2, 2016

View reviewed changes

sgrif added 2 commits October 3, 2016 09:50

remove git churn

3900f9e

Update to the latest version of syn

0111cf1

sgrif mentioned this pull request Oct 3, 2016

Re-implement #[derive(Queryable)] using Macros 1.1 #458

Merged

sgrif mentioned this pull request Oct 3, 2016

Re-implement #[derive(Identifiable)] using Macros 1.1 #459

Merged

sgrif mentioned this pull request Oct 3, 2016

Re-implement #[derive(Identifiable)] using Macros 1.1 #460

Merged

sgrif closed this in #460 Oct 3, 2016

sgrif deleted the sg-macros-1-1 branch October 3, 2016 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Macros 1.1 port #453

WIP Macros 1.1 port #453

sgrif commented Sep 30, 2016

sgrif commented Sep 30, 2016

killercup left a comment

killercup Oct 1, 2016

killercup Oct 1, 2016

killercup Oct 1, 2016 •

edited

Loading

killercup Oct 1, 2016

killercup Oct 1, 2016

killercup Oct 1, 2016 •

edited

Loading

sgrif Oct 1, 2016

sgrif Oct 1, 2016

killercup Oct 1, 2016

dtolnay left a comment

dtolnay Oct 2, 2016

dtolnay Oct 2, 2016

dtolnay Oct 2, 2016

dtolnay Oct 2, 2016

dtolnay Oct 2, 2016

sgrif Oct 3, 2016

dtolnay Oct 2, 2016

dtolnay Oct 2, 2016

dtolnay Oct 2, 2016 •

edited

Loading

		@@ -0,0 +1,16 @@
		use syn::{Ident, Ty, Path, PathSegment};

		pub fn ty_ident(ident: Ident) -> Ty {

WIP Macros 1.1 port #453

WIP Macros 1.1 port #453

Conversation

sgrif commented Sep 30, 2016

sgrif commented Sep 30, 2016

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtolnay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtolnay Oct 2, 2016 • edited Loading

Choose a reason for hiding this comment

killercup Oct 1, 2016 •

edited

Loading

killercup Oct 1, 2016 •

edited

Loading

dtolnay Oct 2, 2016 •

edited

Loading