Update dependencies to syn 0.14 and proc-macro2 0.4 #42

hcpl · 2018-05-23T16:02:01Z

I've split the changes into 3 commits:

The main part of this PR:

List of breaking changes from proc-macro2, quote and syn.
Replacing quote::Tokens with proc_macro2::TokenStream + quote::TokenStreamExt was easy.
Updating to the new Ident was far more tricky because both impl AsRef<str> and impl Copy are gone -- now you can't pass around Idents without thinking about ownership and comparing to strings incurs heap allocation overhead (even impl<T: AsRef<str> + ?Sized> PartialEq<T> uses .to_string() underneath).

The build in this PR produces the following error message:

Output from cargo +1.18.0 build

error[E0271]: type mismatch resolving `<std::collections::HashSet<&proc_macro2::Ident, std::hash::BuildHasherDefault<fnv::FnvHasher>> as std::iter::IntoIterator>::Item == proc_macro2::Ident`
  --> core/src/codegen/trait_impl.rs:72:14
   |
72 |         hits.extend(
   |              ^^^^^^ expected reference, found struct `proc_macro2::Ident`
   |
   = note: expected type `&proc_macro2::Ident`
              found type `proc_macro2::Ident`

error: no method named `as_ref` found for type `proc_macro2::Ident` in the current scope
  --> core/src/options/input_field.rs:30:39
   |
30 |                 .unwrap_or(self.ident.as_ref()),
   |                                       ^^^^^^

error: no method named `as_ref` found for type `proc_macro2::Ident` in the current scope
  --> core/src/options/input_variant.rs:24:39
   |
24 |                 .unwrap_or(self.ident.as_ref()),
   |                                       ^^^^^^

error: no method named `as_ref` found for type `&proc_macro2::Ident` in the current scope
  --> core/src/util/ident_list.rs:29:31
   |
29 |         self.iter().map(|i| i.as_ref()).collect()
   |                               ^^^^^^
   |
   = note: the method `as_ref` exists but the following trait bounds were not satisfied: `proc_macro2::Ident : std::convert::AsRef<_>`

error: aborting due to 4 previous errors

error: Could not compile `darling_core`.

I believe that to resolve this on my behalf I would need to dig further and make fundamental changes in the API. I'm not comfortable to do so, for being afraid to mess up the logic, though if you think that would be easy to do, I'll probably try. Also, I might be totally wrong and just overthinking the complexity of this library 😄.

Implement "proc-macro" feature propagation for darling_core so that there was a way to be dependency-free from rustc-related libraries. Since darling_macro directly depends on libproc_macro and darling is just a re-export facade (and so indirectly depends on libproc_macro), they didn't get the same treatment.
Miscellaneous improvements to developer's workflow and stability guarantees regarding Rust compiler version -- now CI tracks Rust 1.18.

With that said, please make a thorough review and suggest ideas/criticize where appropriate!

Closes #38.

NOTE: This commit makes breaking changes only on surface level and builds are not successful yet, so some in-depth changes are needed too. Expect this commit to be squashed with others to produce a working one.

This will make `darling_core` usable in projects that don't want to depend on the dynamic library libproc_macro from rustc toolchain.

TedDriggs

First off, thanks for making another great dependency update. I saw this come out just after I'd finished a round of changes, and I definitely had an "oh noooo" moment.

error[E0271]: type mismatch resolving `<std::collections::HashSet<&proc_macro2::Ident, std::hash::BuildHasherDefault<fnv::FnvHasher>> as std::iter::IntoIterator>::Item == proc_macro2::Ident`
  --> core/src/codegen/trait_impl.rs:72:14
   |
72 |         hits.extend(
   |              ^^^^^^ expected reference, found struct `proc_macro2::Ident`
   |
   = note: expected type `&proc_macro2::Ident`
              found type `proc_macro2::Ident`

I'm pretty sure I understand what's happening here; I was confused why this code worked before, and I think it's because Ident implemented Copy and I didn't know about it. I was thinking this morning about adding a new collect_cloned_type_params method to that trait which would clone the Idents before returning; this seems to be one more reason to do that.

Implement "proc-macro" feature propagation for darling_core so that there was a way to be dependency-free from rustc-related libraries. Since darling_macro directly depends on libproc_macro and darling is just a re-export facade (and so indirectly depends on libproc_macro), they didn't get the same treatment.

I don't understand the impact of this.

What do other users gain by removing this indirect dependency (and therefore on rustc-related libraries)?
Do we lose anything?

Once I know that I'll be able to update the readme. I'm also wondering if it makes sense to put this feature on darling so that people still interact with one crate, and they turn off the proc-macro feature there to avoid getting the proc macros.

TedDriggs · 2018-05-23T17:03:11Z

core/src/options/from_variant.rs

@@ -51,7 +51,7 @@ impl ParseAttribute for FromVariantOptions {

 impl ParseData for FromVariantOptions {
    fn parse_field(&mut self, field: &Field) -> Result<()> {
-        match field.ident.as_ref().map(|i| i.as_ref()) {
+        match field.ident.as_ref().map(|i| i.to_string().as_ref()) {


Should this line be using as_str?

Technically it works fine but for consistency reasons, yeah, you're right.

TedDriggs · 2018-05-23T17:04:18Z

core/src/options/mod.rs

@@ -43,7 +44,7 @@ impl FromMeta for DefaultExpression {
    }

    fn from_string(lit: &str) -> Result<Self> {
-        Ok(DefaultExpression::Explicit(syn::Path::from(lit)))
+        Ok(DefaultExpression::Explicit(syn::Path::from(syn::Ident::new(lit, Span::call_site()))))


This changes the meaning of this line; we allow path::to::function here, but I don't think that's a valid ident.

I didn't quite get it -- does from_string accept ::-separated list of idents by design or by accident?

But just in case, let's see how it worked with syn 0.13:

impl<T> From<T> for Path where T: Into<PathSegment>: https://docs.rs/syn/0.13.11/syn/struct.Path.html#impl-From%3CT%3E

impl<T> From<T> for PathSegment where T: Into<Ident>: https://docs.rs/syn/0.13.11/syn/struct.PathSegment.html#impl-From%3CT%3E

impl<'a> From<&'a str> for Ident: https://docs.rs/syn/0.13.11/syn/struct.Ident.html#impl-From%3C%26%27a%20str%3E

So Path::from(lit) is actually Path::from(PathSegment::from(Ident::from(lit))) which is equivalent to

Path { leading_colon: None, segments: FromIterator::from_iter(iter::once(PathSegment { ident: Ident::new(lit, Span::call_site()), arguments: PathArguments::None, })), }

Which means that even if from_string was intended to accept path::to::function (and parse it correctly), Ident::from(lit) would just panic with such input.

That's concerning. This is supposed to have behaved like serde(default="path::to::foo"); I wonder if it never worked. It's worth adding a test that exercises the path functionality.

These make composition easier at the expense of additional allocations

- Change codegen structs to take an ident list rather than string slices - Do horrible things to transform idents to `&str` during darling's own attribute parsing. These things involve multiple calls to as_ref and map. - Change how tests create idents

TedDriggs · 2018-05-23T21:23:04Z

core/src/codegen/mod.rs

            let mut __fwd_attrs: ::darling::export::Vec<::syn::Attribute> = vec![];

            for __attr in &#input.attrs {
                // Filter attributes based on name
-                match __attr.path.segments.iter().map(|s| s.ident.as_ref()).collect::<Vec<&str>>().join("::").as_str() {
+                match  ::darling::export::ToString::to_string(&__attr.path.clone().into_token_stream()).as_str() {


@dtolnay, do you have an idea on how to efficiently match a path against other paths? Having to import quote, write to a token stream, and convert to a string feels wasteful (or at least roundabout)

PartialEq for Path? https://docs.rs/syn/0.14/syn/struct.Path.html#impl-PartialEq
I think it is gated behind "extra-traits" feature.

This code is generating a match statement over attribute paths that are taken from user strings, and the generated code is going to live in the user's crate.

Is there a good way to write a match statement which does equality checks against paths?

There is no builtin way to compare a Path with a string directly so you will be comparing either strings with strings or paths with paths. If strings with strings, this looks fine. If paths with paths, the attr_names need to be converted to paths with a match-guard to do the == comparison.

I'm conflicted here. Re-parsing the handled and forwarded attribute path values every time we come through feels wasteful, but having to indirectly import quote to tokenize the path feels very hacky.

You don't necessarily need a parser involved to construct Path values: playground.

Ooh, that's a good point here. The user input is technically passed as an IdentList, I believe. Maybe I can do the comparisons of "segment length is 1, and first segment is in the user-specified idents"

hcpl · 2018-05-24T02:00:09Z

Sorry for the late response, I forgot to tune in when notifications arrived.

First off, thanks for making another great dependency update.

Thank you too for supporting contributions and this library!

I saw this come out just after I'd finished a round of changes, and I definitely had an "oh noooo" moment.

Frankly, I began tackling this PR 3 days ago, when the new proc-macro2 0.4 + quote 0.6 + syn 0.14 stack was just released but only finished today. And just when I thought everything was ready to launch a PR, I found new 6 commits in master that appeared in the following 2 days. Which, of course, broke some stuff, but fortunately that was easy to fix 😆.

I'm pretty sure I understand what's happening here; I was confused why this code worked before, and I think it's because Ident implemented Copy and I didn't know about it. I was thinking this morning about adding a new collect_cloned_type_params method to that trait which would clone the Idents before returning; this seems to be one more reason to do that.

I see you've already fixed that in 28959d0.

What do other users gain by removing this indirect dependency (and therefore on rustc-related libraries)?

For example, an ability to use in non-proc-macro contexts -- such a case in presented in the discussion here: dtolnay/quote#62 -- because with the "proc-macro" feature on, the library will pull in a dynamically-linked dependency and that is not desirable for binary distribution.

On the other hand, is darling even used in such situations (or will be)? I'd say that as darling grows its public exposure over time, there will be more and more projects using the library, so the possibility of such case will likely rise. That's why I think future-proofing is a good idea.

Do we lose anything?

Since the released darling facade and child crates don't have public features, it's unlikely that downstream projects use it with default-features = false. Otherwise I don't see anything to lose.

I'm also wondering if it makes sense to put this feature on darling so that people still interact with one crate, and they turn off the proc-macro feature there to avoid getting the proc macros.

That should be doable too, with #[cfg]ing the derive_macro parts.

So I'm glad to see the builds green. Thank you for pitching in!

TedDriggs · 2018-05-30T16:23:15Z

That should be doable too, with #[cfg]ing the derive_macro parts.

Let's do that, using a feature called derive, same as serde (though ours will be true by default). I think it'll be easier for documentation etc. if people are using one entry point.

hcpl added 3 commits May 23, 2018 16:04

Update dependencies

02f966b

NOTE: This commit makes breaking changes only on surface level and builds are not successful yet, so some in-depth changes are needed too. Expect this commit to be squashed with others to produce a working one.

Propagate "proc-macro" feature from dependencies

e678562

This will make `darling_core` usable in projects that don't want to depend on the dynamic library libproc_macro from rustc toolchain.

Update .travis.yml and .gitignore

17f9867

TedDriggs requested changes May 23, 2018

View reviewed changes

TedDriggs changed the title ~~[WIP] Update dependencies~~ [WIP] Update dependencies to syn 0.14 and proc-macro2 0.4 May 23, 2018

TedDriggs added 5 commits May 23, 2018 11:21

Add new 'cloned' methods to usage traits

28959d0

These make composition easier at the expense of additional allocations

Add new IdentString type

38d7e6f

Fix from_meta tests

4b50405

Get all tests to pass

a976f7b

- Change codegen structs to take an ident list rather than string slices - Do horrible things to transform idents to `&str` during darling's own attribute parsing. These things involve multiple calls to as_ref and map. - Change how tests create idents

Fix issue with default="..." not accepting valid paths

8b874bb

TedDriggs reviewed May 23, 2018

View reviewed changes

TedDriggs approved these changes May 23, 2018

View reviewed changes

TedDriggs added the enhancement label May 23, 2018

TedDriggs added this to the 0.7.0 milestone May 23, 2018

TedDriggs added 2 commits June 5, 2018 09:29

Explain current solution for attribute extraction

40eb134

Revert proc-macro change to core/Cargo.toml

a8d697f

TedDriggs merged commit d171944 into TedDriggs:master Jun 5, 2018

TedDriggs changed the title ~~[WIP] Update dependencies to syn 0.14 and proc-macro2 0.4~~ Update dependencies to syn 0.14 and proc-macro2 0.4 Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dependencies to syn 0.14 and proc-macro2 0.4 #42

Update dependencies to syn 0.14 and proc-macro2 0.4 #42

hcpl commented May 23, 2018

TedDriggs left a comment

TedDriggs May 23, 2018

hcpl May 23, 2018

TedDriggs May 23, 2018

hcpl May 23, 2018

TedDriggs May 23, 2018

TedDriggs May 23, 2018

dtolnay May 23, 2018

TedDriggs May 23, 2018

dtolnay May 23, 2018

TedDriggs May 23, 2018

dtolnay May 23, 2018

TedDriggs May 23, 2018

hcpl commented May 24, 2018

TedDriggs commented May 30, 2018

Update dependencies to syn 0.14 and proc-macro2 0.4 #42

Update dependencies to syn 0.14 and proc-macro2 0.4 #42

Conversation

hcpl commented May 23, 2018

TedDriggs 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

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

hcpl commented May 24, 2018

TedDriggs commented May 30, 2018