Update to the proc_macro2 crate and rewritten `syn` #980

alexcrichton · 2017-07-06T22:13:57Z

No description provided.

alexcrichton · 2017-07-06T22:14:48Z

Note that this is a PR against the proc-macro2 branch, not master. It's intended that this updates #978.

alexcrichton · 2017-07-06T22:16:38Z

cc @mystor as well

Some things I've learned as part of this transition:

Overall the transition was pretty smooth, API-wise nothing was too bad.
Delimited kinda sucks. It doesn't have all the Vec APIs so using it can be sort of verbose.
The biggest footgun is having a syn AST that's in an inconsistent state. For example you could have a WhereClause with a bunch of where predicates without a where token. This happened a ton here and I had to fix up a bunch of places.

Curious what y'all think of those points!

mystor · 2017-07-06T23:24:16Z

I haven't had the time to read the actual patch yet, so I'm just commenting based on your comment.

For point 2, I think in many cases we could probably get away with the into_vec and from(vec) methods, as usually rustc won't be producing diagnostics pointing to delimiters.

That being said, we should definitely persue better ergonomics for Delimited. The obvious solution is to implement more of Vec's API, which is an unfortunate maintenance burden. My other idea would be to store the delimiters and the AST nodes in two separate Vec objects internally, so that we can provide a &mut to the AST node Vec. When tokenising the AST nodes, we could pull tokens from the delimiter vec until we run out of tokens, and then we'd start generating them from thin air with dummy spans. That way we provide the ergonomics of ignoring the tokens and using a Vec<AST Node> directly unless you need to worry about delimiter spans.

I filed dtolnay/syn#177 for point 3.

mystor

I added some comments to the changes, mostly with an eye to how we can make syn better. I don't know anything about serde's codebase, so I can't comment on the correctness of the new code, but it looks like a fairly straightforward transformation, so I think it should be good.

mystor · 2017-07-07T02:36:15Z

serde_derive/Cargo.toml

 serde_derive_internals = { version = "=0.15.1", default-features = false, path = "../serde_derive_internals" }
-syn = { version = "0.11", features = ["visit"] }
+syn = { git = 'https://github.com/dtolnay/syn', features = ["visit"] }
+synom = { git = 'https://github.com/dtolnay/syn', default-features = false }


I don't love that we have to import synom explicitly here, when we used to only need to import syn. I don't feel like it's a good idea to re-export proc-macro2, but for most users synom should be an implementation detail.

Can we re-export the important bits from syn? I feel like Span, tokens and Delimited should probably all be re-exported under syn's root.

mystor · 2017-07-07T02:39:38Z

serde_derive/src/bound.rs

                    }
                },
            )
-            .collect(),
+            .collect::<Vec<_>>()


It would be great to have FromIterator implemented on Delimited. While it feels overkill to have every Vec method or trait, that one at least feels useful.

mystor · 2017-07-07T02:40:52Z

serde_derive/src/bound.rs

+    for predicate in predicates {
+        if generics.where_clause.where_token.is_none() {
+            generics.where_clause.where_token = Some(tokens::Where::default());
+        }


This is really gross. Could we make our ToTokens implementation detect that we need a Where token and just make one for us in this situation implicitly?

mystor · 2017-07-07T02:42:18Z

serde_derive/src/bound.rs

@@ -101,15 +113,16 @@ where
    }
    impl visit::Visitor for FindTyParams {
        fn visit_path(&mut self, path: &syn::Path) {
-            if let Some(seg) = path.segments.last() {
+            if path.segments.len() > 0 {
+                let seg = *path.segments.get(path.segments.len() - 1).item();


first[_mut] and last[_mut] also seem worthwhile to add to Delimited.

mystor · 2017-07-07T02:44:49Z

serde_derive/src/bound.rs

+
+    for lifetime_def in generics.lifetimes.iter_mut().map(|m| m.into_item()) {
+        if lifetime_def.colon_token.is_none() {
+            lifetime_def.colon_token = Some(tokens::Colon::default());


We'd also need to make the colon token be automatically added :-/

mystor · 2017-07-07T03:16:23Z

serde_derive/src/de.rs

@@ -163,7 +173,10 @@ fn needs_deserialize_bound(attrs: &attr::Field) -> bool {
 // Fields with a `default` attribute (not `default=...`), and fields with a
 // `skip_deserializing` attribute that do not also have `default=...`.
 fn requires_default(attrs: &attr::Field) -> bool {
-    attrs.default() == &attr::Default::Default
+    match *attrs.default() {
+        attr::Default::Default => true,


The extra-traits change feels unrelated to me, but I suppose you're doing it to try to save on compile time? It feels a bit like it should be done in a separate patch to me.

mystor · 2017-07-07T03:19:33Z

serde_derive_internals/Cargo.toml


 [badges]
 travis-ci = { repository = "serde-rs/serde" }
+
+[features]
+extra-traits = ["syn/extra-traits"]


extra-traits is listed twice, both as a required and optional feature. I think it should only be listed in one of those two places.

dtolnay · 2017-07-07T12:18:43Z

Great, this is enough to convince me that things are still usable. I don't think we need to polish this further right now.

I responded in dtolnay/syn#123 (comment) but I think we can go ahead and publish the current syn/synom/quote under different names and address some of the points here over the next couple weeks.

alexcrichton · 2017-07-08T03:48:07Z

@dtolnay do you have a preference on coordination of merging this? Would you prefer to only merge the update into serde_derive when syn/synom/etc are published? (that seems like a reasonable constraint to me)

Would you otherwise want to merge this into the proc-macro2 branch here for continued development of the proc-macro2 integration?

alexcrichton · 2017-07-08T04:08:34Z

Apart from dtolnay/syn#177 I believe I've addressed the issues mentioned here

dtolnay

Let's keep this on the proc-macro2 branch.

alexcrichton mentioned this pull request Jul 6, 2017

Update to proc-macro2 #978

Closed

mystor mentioned this pull request Jul 6, 2017

Somehow prevent the creation of invalid ASTs dtolnay/syn#177

Closed

mystor approved these changes Jul 7, 2017

View reviewed changes

dtolnay mentioned this pull request Jul 7, 2017

How to parse keeping spans dtolnay/syn#178

Closed

Update to the proc_macro2 crate and rewritten syn

370d4ef

alexcrichton force-pushed the syn branch from 484d177 to 370d4ef Compare July 8, 2017 04:08

dtolnay approved these changes Jul 8, 2017

View reviewed changes

dtolnay merged commit 46a3aaa into serde-rs:proc-macro2 Jul 8, 2017

alexcrichton deleted the syn branch July 8, 2017 18:24

sgrif mentioned this pull request Jul 10, 2017

What is proc-macro2 diesel-rs/diesel#1005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to the proc_macro2 crate and rewritten `syn` #980

Update to the proc_macro2 crate and rewritten `syn` #980

alexcrichton commented Jul 6, 2017

alexcrichton commented Jul 6, 2017

alexcrichton commented Jul 6, 2017

mystor commented Jul 6, 2017

mystor left a comment

mystor Jul 7, 2017

mystor Jul 7, 2017

mystor Jul 7, 2017

mystor Jul 7, 2017

mystor Jul 7, 2017

mystor Jul 7, 2017

mystor Jul 7, 2017

dtolnay commented Jul 7, 2017

alexcrichton commented Jul 8, 2017

alexcrichton commented Jul 8, 2017

dtolnay left a comment

Update to the proc_macro2 crate and rewritten syn #980

Update to the proc_macro2 crate and rewritten syn #980

Conversation

alexcrichton commented Jul 6, 2017

alexcrichton commented Jul 6, 2017

alexcrichton commented Jul 6, 2017

mystor commented Jul 6, 2017

mystor left a comment

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

mystor Jul 7, 2017

Choose a reason for hiding this comment

dtolnay commented Jul 7, 2017

alexcrichton commented Jul 8, 2017

alexcrichton commented Jul 8, 2017

dtolnay left a comment

Choose a reason for hiding this comment

Update to the proc_macro2 crate and rewritten `syn` #980

Update to the proc_macro2 crate and rewritten `syn` #980