Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parsing attributes with arbitrary tokens. #111

Merged
merged 12 commits into from
Apr 21, 2017
Merged

Support parsing attributes with arbitrary tokens. #111

merged 12 commits into from
Apr 21, 2017

Conversation

Arnavion
Copy link
Contributor

Ref rust-lang/rust#40346

This is a nightly-only feature (gated by proc_macro feature). Maybe you'd prefer it behind a feature instead of enabled by default?

src/attr.rs Outdated
@@ -170,6 +177,25 @@ pub mod parsing {
)
|
do_parse!(
name_and_token_trees: preceded!(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to implement this here instead of in the meta_item rule because I needed to use delimited! to limit how much token_trees parses. Is there a better way?

@dtolnay
Copy link
Owner

dtolnay commented Apr 15, 2017

Thanks! The implementation looks good but I would prefer an approach more like how it's done in rust-lang/rust#40346/src/libsyntax/ast.rs - Attribute stores tokens instead of a MetaItem. Then we can provide a function to try parsing the tokens as a Metaitem. It seems wrong that if someone's macro expects an attribute to be plain tokens, they would need to handle all four MetaItem cases in case the user's tokens accidentally matched one of the MetaItem patterns.

impl Attribute {
    fn meta_item(&self) -> Option<MetaItem>;
}

Also please update the inner_attr parser as well - that one deals with #![...] attributes.

@Arnavion
Copy link
Contributor Author

I did think of that but didn't go that way because it would be a bigger breaking change. I can do that if you're okay with it.

@Arnavion
Copy link
Contributor Author

Done, but I want to add more tests for the nested meta items parser as well.

@Arnavion
Copy link
Contributor Author

Arnavion commented Apr 15, 2017

I don't understand the CI failure. nvm got it.

@Arnavion
Copy link
Contributor Author

Tests done.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks great. I am busy with some Serde things this week but I scheduled time on Thursday to review more closely and get this published.

Would you be able to also help me with #109? I think that is the only other AST change that needs to happen presently.

src/attr.rs Outdated
@@ -6,13 +6,97 @@ use std::iter;
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Attribute {
pub style: AttrStyle,
pub value: MetaItem,
pub name: Ident,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one in rustc supports any Path here - for example they have this unit test:

#[::attr_args::identity
  fn main() { assert_eq!(foo(), "Hello, world!"); }]
struct Dummy;

Any idea how much harder that would be to support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was initially holding off on it to not make a breaking change. But since meta_item() is already going to be one it makes sense to combine them.

@Arnavion
Copy link
Contributor Author

I think I'm done with changes for now. Let me know if you approve so I can squash and fix the first commit's message to mention the other changes since then.

@dtolnay
Copy link
Owner

dtolnay commented Apr 21, 2017

Hmm something is not right. I tried this code:

nothing/src/lib.rs

extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Nothing, attributes(inert))]
pub fn demo(input: TokenStream) -> TokenStream {
    "".parse().unwrap()
}

testing/src/main.rs

#![feature(proc_macro)]

#[macro_use]
extern crate nothing;

extern crate syn;

#[derive(Nothing)]
#[inert <T>]
struct S;

fn main() {
    let a = "#[inert <T>]";
    println!("{:#?}", syn::parse_outer_attr(a).unwrap());
}

Using cargo rustc -- -Z ast-json the attribute on S parses as path(inert) vec![lt ident(T) gt] but using syn it parses as path(inert<T>) vec![]. If I remember correctly libsyntax actually has a few different parsers for Path that get used in different contexts. Will take a closer look tomorrow.

@Arnavion
Copy link
Contributor Author

You're right. https://github.com/rust-lang/rust/blob/535ee6c7f05e29a6e94edba06b228d64f8ba74ec/src/libsyntax/parse/attr.rs#L152

I'll add mod_style_path and mod_style_path_segment rules.

@dtolnay dtolnay merged commit 92cb1a6 into dtolnay:master Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants