Macros one dot one (Do Not Merge) #28

Merged
merged 6 commits into from Jan 19, 2017

Projects

None yet

4 participants

@badboy
Collaborator
badboy commented Oct 11, 2016

So today I read up on the new Macros 1.1 approach and it turns out it is really easy to use (together with the syn and quote crates).

I ported over the code to use Macros 1.1 and it works with significantly less code now.
I had to move the tests into their own crate, compiling a test binary in the right library mode is impossible.

This makes the code require a recent nightly, DO NOT MERGE immediately

This can probably stay around until Macros 1.1 are actually stable (probably not before the end of the year though).

@killercup
Collaborator

Woohoo! But I really want to merge this right now! ๐Ÿ˜…

It might make sense to release this as an alternative version (if it's any good, I haven't read your code yet!). The macro derive code should be semi-stable in nightly -- diesel 0.8 pretty much promises you can use the latest nightly/syntex and it will work.

Hw much work will testing two crates be? I.e., we have macro_rules & macros1.1 and a set of integration tests that work for both.

@badboy
Collaborator
badboy commented Oct 11, 2016

After I pushed this I thought about having both code parts behind feature flags. But then again, this seems less optimal. We could however make it a derive-builder-next package to be released now and merge both back when Macros 1.1 hit stable.

@colin-kiegel
Owner

I would like to talk with you guys about renaming things to derive(setter, getter) and making derive(builder) a meta crate which only wraps things
with the option to depcrecate in the future depending on usage. But I
expect the alternative name much more intuitive.

I had this thought for quite a while and I think this is a good moment to
actually do it. What do you think?

Jan-Erik Rediger notifications@github.com schrieb am Di., 11. Okt. 2016,
19:54:

After I pushed this I thought about having both code parts behind feature
flags. But then again, this seems less optimal. We could however make it a
derive-builder-next package to be released now and merge both back when
Macros 1.1 hit stable.

โ€”
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHVbsp_-YG4Gq4mBFxyParYp00OZpeN1ks5qy81mgaJpZM4KT5Yr
.

@killercup

LGTM!

(I saw you commented on some stuff while I was reviewing this. Will respond in another comment.)

@@ -4,13 +4,17 @@ version = "0.2.1"
authors = ["Colin Kiegel <kiegel@gmx.de>",
"Pascal Hertleif <killercup@gmail.com>"]
@killercup
killercup Oct 11, 2016 Collaborator

If this gets merged you should be listed here, @badboy!

-**This is a work in progress.** Use it at your own risk.
+**This is a work in progress.** Use it at your own risk.
+**This currently requires Rust nightly, due to the usage of Macros 1.1**
@killercup
killercup Oct 11, 2016 edited Collaborator

Please mention which nightly you think works; something like "requires Rust nightly (2016-10-09 or newer)" is good. Also: Period at the end.

@@ -70,7 +69,6 @@ The builder pattern is explained [here][builder-pattern], including its variants
[doc]: https://colin-kiegel.github.io/rust-derive-builder
[rust]: https://www.rust-lang.org/
-[custom_derive]: https://crates.io/crates/custom_derive
@killercup
killercup Oct 11, 2016 edited Collaborator

It was fun while it lasted ๐Ÿ˜ข

+#[test]
+fn annotations() {
+ // this is currently just a compile-test (may switch to token comparisons here)
+ // https://github.com/colin-kiegel/rust-derive-builder/issues/19
@killercup
killercup Oct 11, 2016 edited Collaborator

Still valid? Is there a test story for nicely comparing syn tokens?

//! ```rust
-//! #[macro_use] extern crate custom_derive;
//! #[macro_use] extern crate derive_builder;
@killercup
killercup Oct 11, 2016 Collaborator

Can you also add the #![feature(proc_macro)] here (or does that not compile)?

@badboy
badboy Oct 11, 2016 Collaborator

I need to try.

+
+#[proc_macro_derive(Builder)]
+pub fn derive(input: TokenStream) -> TokenStream {
+ let input: String = input.to_string();
@killercup
killercup Oct 11, 2016 Collaborator

probably unnecessary : String

@badboy
badboy Oct 11, 2016 Collaborator

Indeed. that bit was copy'n'pasted

+ self.#f_name = value.into();
+ self
+ })
+ });
@killercup
killercup Oct 11, 2016 edited Collaborator

Nice & concise! ๐Ÿ‘

IIRC* quote! returns Tokens which has an append method. For future extensions we could add stuff like

let le_tokens = quote!(); // empty Tokens

if cfg!(feature = "consuming") {
    le_tokens.append(quote!(/* what you wrote */))
} else { /* other stuff */}

if cfg!(feature = "getters") { /* getters, maybe filtered by some attr on the field */ }

* by which I mean "read https://docs.rs/quote/0.3.3/quote/struct.Tokens.html correctly after not trusting my memory"

+ });
+
+ quote! {
+ impl #impl_generics #name #ty_generics #where_clause {
@killercup
killercup Oct 11, 2016 Collaborator

Will this render as impl <T> Stuff <T> where T: Magic -- with the spaces? If I was just one tiny bit more crazy I'd ask you to get rid of the spaces before the generics lists ๐Ÿ˜†

@badboy
badboy Oct 11, 2016 Collaborator

I have no idea, I just copy'n'pasted that bit and it worked.

@dtolnay
dtolnay Oct 13, 2016

Technical details which in no way affect my preference about where to put spaces:

  • From the quote! macro's perspective there is no way for it to tell where you put spaces. It sees a Vec<TokenTree> which looks like ["impl", "#", "impl_generics", "#", "name", "#", "ty_generics", "#", "where_clause", Braces(["#", Parens(["funcs"]), "*"])].

  • The string constructed by the quote! macro will have a single space in between every token (at least as of quote 0.3.3; I think in this version there might be more than one in certain cases). This is the simplest heuristic that is always correct with respect to rustc being able to parse the result as intended. So if impl_generics is <T> then the string will start with `"impl < T >".

  • The builtin stringify! has the same behavior for the same reason:

    fn main() {
        // impl # impl_generics # name { # ( funcs ) * }
        println!("{}", stringify!(impl #impl_generics #name { #(funcs)* }));
    }
  • When you take the string from quote! and parse it to a TokenStream for #[derive(Builder)] to return, the spaces are throw out again. The TokenStream looks like ["impl", "<", "T", ">", ...].

  • One of the first things Rust does upon getting back the TokenStream is parse it. So if you cargo expand a crate containing #[derive(Builder)] the spaces will be where Rust's pretty-printer puts them, not where quote! puts them and not where you put them.

Nontechnical details which fully determine my preference:

  • I find impl#impl_generics #name#ty_generics less readable.
  • I prefer the style @badboy used here.
  • He probably got it from my examples so it is no coincidence ๐Ÿ˜›.

Bottom line:

  • Put spaces wherever you want.
  • There are so many steps between where you put the spaces and where the spaces end up that even if most of them mattered, it still wouldn't matter.
@colin-kiegel
Owner

PS: if we go down this road we should add options to support different setter styles

@killercup
Collaborator
killercup commented Oct 11, 2016 edited

@badboy instead of a new crate with a new name we could also do a "derive-builder v0.3.0.alpha-1" release or something like that (if @colin-kiegel wants to do that).

@colin-kiegel Interesting! I'd use the plural names though: #[derive(Getters, Setters)]. (And getters should require explicit attrs to be generated.)

It might also be valuable to introduce a whole new level of 'indirect builder': By defining a new struct FooBuilder with the same fields as Foo but only the builder methods (incl. finalize/build/watshouldwecallit) we can introduce a bit more type safety by making it very explicit when the building step is complete.

@colin-kiegel
Owner
colin-kiegel commented Oct 11, 2016 edited

@killercup Good points. :-)
cc @badboy

If you both like the idea of #derive(Getters, Setters) AND publishing as new crates derive_setters and derive_getters or just setters and getters we should act fast.. :-)

You may feel free to secure one set of crate names you like by publishing dummy crates. Because I'd guess it may be a popular exercise to implement this kind of stuff right now.

PS: Or if you want me to do it, I could just switch to my dev partition and do it.

@dtolnay

This is a massive improvement, nicely done.

+ });
+
+ quote! {
+ impl #impl_generics #name #ty_generics #where_clause {
@dtolnay
dtolnay Oct 13, 2016

Technical details which in no way affect my preference about where to put spaces:

  • From the quote! macro's perspective there is no way for it to tell where you put spaces. It sees a Vec<TokenTree> which looks like ["impl", "#", "impl_generics", "#", "name", "#", "ty_generics", "#", "where_clause", Braces(["#", Parens(["funcs"]), "*"])].

  • The string constructed by the quote! macro will have a single space in between every token (at least as of quote 0.3.3; I think in this version there might be more than one in certain cases). This is the simplest heuristic that is always correct with respect to rustc being able to parse the result as intended. So if impl_generics is <T> then the string will start with `"impl < T >".

  • The builtin stringify! has the same behavior for the same reason:

    fn main() {
        // impl # impl_generics # name { # ( funcs ) * }
        println!("{}", stringify!(impl #impl_generics #name { #(funcs)* }));
    }
  • When you take the string from quote! and parse it to a TokenStream for #[derive(Builder)] to return, the spaces are throw out again. The TokenStream looks like ["impl", "<", "T", ">", ...].

  • One of the first things Rust does upon getting back the TokenStream is parse it. So if you cargo expand a crate containing #[derive(Builder)] the spaces will be where Rust's pretty-printer puts them, not where quote! puts them and not where you put them.

Nontechnical details which fully determine my preference:

  • I find impl#impl_generics #name#ty_generics less readable.
  • I prefer the style @badboy used here.
  • He probably got it from my examples so it is no coincidence ๐Ÿ˜›.

Bottom line:

  • Put spaces wherever you want.
  • There are so many steps between where you put the spaces and where the spaces end up that even if most of them mattered, it still wouldn't matter.
@colin-kiegel
Owner

There seems to be a way to use the syn crate in a build script for custom derives:

I'm not sure if it's worth investigating, since macros 1.1 should not be too far away now.

@killercup
Collaborator

Any plan on finishing/merging this? 1.15 with stable macros 1.1 will be released on February 3rd, and I'd be delighted to ship a new, shiny derive-builder version on day one :)

@colin-kiegel
Owner

Good point. I'll try to look into updating this PR this weekend.

@colin-kiegel colin-kiegel merged commit d616575 into colin-kiegel:master Jan 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment