`TokenStream`-based attributes, paths in attribute and derive macro invocations #40346

jseyfried · 2017-03-08T02:55:23Z

This PR

refactors Attribute to use Path and TokenStream instead of MetaItem.
supports macro invocation paths for attribute procedural macros.
- e.g. #[::foo::attr_macro] struct S;, #[cfg_attr(all(), foo::attr_macro)] struct S;
supports macro invocation paths for derive procedural macros.
- e.g. #[derive(foo::Bar, super::Baz)] struct S;
supports arbitrary tokens as arguments to attribute procedural macros.
- e.g. #[foo::attr_macro arbitrary + tokens] struct S;
supports using arbitrary tokens in "inert attributes" with derive procedural macros.
- e.g. #[derive(Foo)] struct S(#[inert arbitrary + tokens] i32);
  where #[proc_macro_derive(Foo, attributes(inert))]

r? @nrc

jseyfried · 2017-03-08T03:14:08Z

cc #38356
cc @keeperofdakeys @abonander @SimonSapin @cgswords @dtolnay @SergioBenitez

dtolnay · 2017-03-08T03:21:15Z

What is the use case that motivates arbitrary tokens in attributes?

The test case that has the following is not a real thing that we expect people to write, correct?

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

jseyfried · 2017-03-08T03:50:09Z

@dtolnay

What is the use case that motivates arbitrary tokens in attributes?

We want to give attribute procedural macro authors the power/flexibility to parse the attributes however they want. Also, we pass the attribute's arguments to the procedural macro as a TokenStream, so it makes sense to support any TokenStream.

The test case that has the following is not a real thing that we expect people to write, correct?

Well, it's a not a useful procedural macro, but people can put whatever they want in macro attributes. Items in macro attributes is technically fine but looks ugly so I wouldn't expect it in the wild. However, it seems reasonable to for people to use other AST nodes (e.g. expressions, patterns, paths, etc.) in attribute macros.

dtolnay · 2017-03-08T04:21:41Z

I guess I would strongly prefer to have a concrete use case in mind to motivate this change. Are there any existing procedural macro crates whose API you think would be improved by using "unconventional" attributes? Are there any procedural macro crates that you wish could exist but require this feature?

I agree that we can come up with hypothetical ways to use this but so far I disagree that this functionality is the best way to address any of those use cases, and consequently my instinct would be to discourage libraries from using this.

I don't find "we pass the attribute's arguments to the procedural macro as a TokenStream" to be a compelling justification. The only thing that means is that it is technically possible to implement this change, not that it is a good idea.

nrc · 2017-03-08T04:42:02Z

r? @nrc

nrc · 2017-03-08T04:51:28Z

@dtolnay an example might be an attribute on an enum that wants to do something specific with one field:

#[foo::macro(Var2::field2)]
enum Bar {
    Var1,
    Var2 { field1: i32, field2: i32 }
}

or maybe specifying a precondition:

#[precondition x < 100]
fn foo(x: i32) { ... }

nrc · 2017-03-08T04:52:17Z

Concretely, my libhoare crate does the precondition thing, but hacks it with a string atm.

dtolnay · 2017-03-08T04:57:40Z

Neat, the libhoare use case is compelling. Thanks!

jseyfried · 2017-03-08T05:02:12Z

@dtolnay
Also, the "unconventional" attributes this PR allows are gated behind #![feature(proc_macro)], so we can always decide to change course and add restrictions later.

bors · 2017-03-11T08:30:38Z

☔ The latest upstream changes (presumably #40432) made this pull request unmergeable. Please resolve the merge conflicts.

nrc

r=me with the naming and refactoring addressed

nrc · 2017-03-13T03:51:04Z

src/librustc/middle/stability.rs

@@ -402,7 +402,8 @@ impl<'a, 'tcx> Index<'tcx> {

        let mut is_staged_api = false;
        for attr in &krate.attrs {
-            if attr.name() == "stable" || attr.name() == "unstable" {
+            let name = match attr.name() { Some(name) => name, _ => continue };


worth factoring this idiom out into a macro? Maybe doesn't get used enough, but I'm not really a fan of match one-liners

nrc · 2017-03-13T03:56:41Z

src/librustc_resolve/macros.rs

@@ -194,9 +197,11 @@ impl<'a> base::Resolver for Resolver<'a> {

        // Check for legacy derives
        for i in 0..attrs.len() {
-            if attrs[i].name() == "derive" {
+            let name = match attrs[i].name() { Some(name) => name, None => continue };


Ha, 4 uses, definitely needs a macro :-)

nrc · 2017-03-13T04:00:12Z

src/libsyntax/attr.rs

    }

-    pub fn is_word(&self) -> bool { self.meta().is_word() }
+    pub fn is_word(&self) -> bool {
+        self.tokens.is_empty()


This seems a bit odd to me - this basically means that the attribute is just a path with no other tokens? But that means is_word is a bad name - either there should be a check that the path has length 1 or the name should change.

Good point -- I added a check that the path has length 1.

…Item`.

bors · 2017-03-19T10:56:13Z

⌛ Testing commit 85e02bd with merge 9c15de4...

@nrc

`TokenStream`-based attributes, paths in attribute and derive macro invocations This PR - refactors `Attribute` to use `Path` and `TokenStream` instead of `MetaItem`. - supports macro invocation paths for attribute procedural macros. - e.g. `#[::foo::attr_macro] struct S;`, `#[cfg_attr(all(), foo::attr_macro)] struct S;` - supports macro invocation paths for derive procedural macros. - e.g. `#[derive(foo::Bar, super::Baz)] struct S;` - supports arbitrary tokens as arguments to attribute procedural macros. - e.g. `#[foo::attr_macro arbitrary + tokens] struct S;` - supports using arbitrary tokens in "inert attributes" with derive procedural macros. - e.g. `#[derive(Foo)] struct S(#[inert arbitrary + tokens] i32);` where `#[proc_macro_derive(Foo, attributes(inert))]` r? @nrc

bors · 2017-03-19T13:57:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 9c15de4 to master...

Since rust-lang/rust#40346 has now been merged, Attribute no longer has a .value field. Instead, we must follow the token stream and modify the tokens directly. For Docstring attributes, there should only be one token, the docstring value.

@nrc

…rochenkov macros: fix bug parsing `#[derive]` invocations Fixes rust-lang#40962 (introduced in rust-lang#40346). r? @nrc

@nrc

…rochenkov macros: fix bug parsing `#[derive]` invocations Fixes rust-lang#40962 (introduced in rust-lang#40346). r? @nrc

matklad · 2018-01-11T06:49:54Z

Hm, should arbitrary token trees in attributes require an RFC? At least, there was an RFC for the current meta_item stuff and arbitrary literals.

I would feel slightly more confident if attributes were restricted to, say expressions. The problem that I see is that currently, #[foo(bar, baz quuz spam)] is a syntax error, which is easily detected during parsing. With token trees, this is a totally valid syntax though, and you'd need semantic knowledge just to parse this thing, which hurts IDEs.

I don't think I necessary disagree with allowing arbitrary token trees, but it does make me feel uneasy :)

matklad · 2018-01-20T22:54:19Z

Found one more interesting problem with arbitrary token trees in attributes: they make good error recovery very challenging.

Consider, for example, this code:

#[cfg(foo,
fn foo() {
}

mod m {
}

With the previous restricted grammar, the parser, after consuming foo, and waiting for the next meta item, would see a fn token, which can not start a meta item, but belongs to the FIRST(item). So the natural and easy thing to do would be to report an error, stop parsing the attributes, and then proceed with parsing the following function and module.

Now, when arbitrary token trees are allowed in attributes, the parser has to parse all of the following code as a part of an attribute, and so it would not recognize than a function and a module are, in fact, a function and a module.

Granted, the similar effect is also present with Rust macros, but it is expected that macros are annoying to work with :-)

rust-highfive assigned nrc Mar 8, 2017

jseyfried force-pushed the path_and_tokenstream_attr branch from ec443c5 to 64cdec0 Compare March 8, 2017 02:55

jseyfried mentioned this pull request Mar 8, 2017

Tracking issue for RFC 1566: Procedural macros #38356

Closed

31 tasks

nrc removed their assignment Mar 8, 2017

rust-highfive assigned nrc Mar 8, 2017

jseyfried force-pushed the path_and_tokenstream_attr branch 9 times, most recently from e05b383 to f7b2fca Compare March 8, 2017 23:13

jseyfried force-pushed the path_and_tokenstream_attr branch 2 times, most recently from 062c4a7 to e672b5e Compare March 12, 2017 08:46

nrc approved these changes Mar 13, 2017

View reviewed changes

jseyfried added 2 commits March 14, 2017 03:35

Cleanup.

460bf55

Refactor Attribute to use Path and TokenStream instead of `Meta…

68c1cc6

…Item`.

jseyfried force-pushed the path_and_tokenstream_attr branch from e672b5e to 81d8269 Compare March 14, 2017 04:08

bors merged commit 85e02bd into rust-lang:master Mar 19, 2017

This was referenced Mar 19, 2017

Allow use macro imports to shadow global macros #40501

Merged

macros: improve Span's expansion information #40597

Merged

Improve InternedString usability #40606

Merged

Replace hardcoded forward slash with path::MAIN_SEPARATOR #40620

Merged

jseyfried deleted the path_and_tokenstream_attr branch March 19, 2017 17:25

SergioBenitez mentioned this pull request Mar 20, 2017

ICE during derive expansion on 2017-03-19 nightly #40663

Closed

jonhoo mentioned this pull request Mar 20, 2017

Fix following removal of Attribute.value google/tarpc#136

Merged

dwrensha mentioned this pull request Mar 21, 2017

partially update for latest rustc nightly rust-lang/miri#154

Merged

jseyfried mentioned this pull request Apr 3, 2017

macros: fix bug parsing #[derive] invocations #41050

Merged

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 6, 2017

Rollup merge of rust-lang#41050 - jseyfried:fix_derive_parsing, r=pet…

d8ddb94

…rochenkov macros: fix bug parsing `#[derive]` invocations Fixes rust-lang#40962 (introduced in rust-lang#40346). r? @nrc

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 6, 2017

Rollup merge of rust-lang#41050 - jseyfried:fix_derive_parsing, r=pet…

89b364d

…rochenkov macros: fix bug parsing `#[derive]` invocations Fixes rust-lang#40962 (introduced in rust-lang#40346). r? @nrc

Arnavion mentioned this pull request Apr 15, 2017

Support parsing attributes with arbitrary tokens. dtolnay/syn#111

Merged

jseyfried mentioned this pull request Apr 24, 2017

warning: Quasi-quoting might make incremental compilation very inefficient: NtIdent(..) #40946

Closed

dwrensha mentioned this pull request May 16, 2017

update for latest nightly rustc serde-deprecated/aster#134

Closed

mystor mentioned this pull request Jan 11, 2018

What's up with attributes? rust-lang/rust-analyzer#8

Closed

topecongiro mentioned this pull request Feb 12, 2018

Implement tool_attributes feature (RFC 2103) #47773

Closed

SergioBenitez mentioned this pull request Aug 5, 2018

Stabilize 'attr_literals' feature. #53044

Merged

petrochenkov mentioned this pull request Jan 6, 2019

Stabilize unrestricted_attribute_tokens #57367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`TokenStream`-based attributes, paths in attribute and derive macro invocations #40346

`TokenStream`-based attributes, paths in attribute and derive macro invocations #40346

jseyfried commented Mar 8, 2017 •

edited

Loading

jseyfried commented Mar 8, 2017 •

edited

Loading

dtolnay commented Mar 8, 2017

jseyfried commented Mar 8, 2017

dtolnay commented Mar 8, 2017

nrc commented Mar 8, 2017

nrc commented Mar 8, 2017

nrc commented Mar 8, 2017

dtolnay commented Mar 8, 2017

jseyfried commented Mar 8, 2017 •

edited

Loading

bors commented Mar 11, 2017

nrc left a comment

nrc Mar 13, 2017

nrc Mar 13, 2017

jseyfried Mar 13, 2017 •

edited

Loading

nrc Mar 13, 2017

jseyfried Mar 13, 2017 •

edited

Loading

bors commented Mar 19, 2017

bors commented Mar 19, 2017

matklad commented Jan 11, 2018

matklad commented Jan 20, 2018 •

edited

Loading

TokenStream-based attributes, paths in attribute and derive macro invocations #40346

TokenStream-based attributes, paths in attribute and derive macro invocations #40346

Conversation

jseyfried commented Mar 8, 2017 • edited Loading

jseyfried commented Mar 8, 2017 • edited Loading

dtolnay commented Mar 8, 2017

jseyfried commented Mar 8, 2017

dtolnay commented Mar 8, 2017

nrc commented Mar 8, 2017

nrc commented Mar 8, 2017

nrc commented Mar 8, 2017

dtolnay commented Mar 8, 2017

jseyfried commented Mar 8, 2017 • edited Loading

bors commented Mar 11, 2017

nrc left a comment

Choose a reason for hiding this comment

nrc Mar 13, 2017

Choose a reason for hiding this comment

nrc Mar 13, 2017

Choose a reason for hiding this comment

jseyfried Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

nrc Mar 13, 2017

Choose a reason for hiding this comment

jseyfried Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 19, 2017

bors commented Mar 19, 2017

matklad commented Jan 11, 2018

matklad commented Jan 20, 2018 • edited Loading

`TokenStream`-based attributes, paths in attribute and derive macro invocations #40346

`TokenStream`-based attributes, paths in attribute and derive macro invocations #40346

jseyfried commented Mar 8, 2017 •

edited

Loading

jseyfried commented Mar 8, 2017 •

edited

Loading

jseyfried commented Mar 8, 2017 •

edited

Loading

jseyfried Mar 13, 2017 •

edited

Loading

jseyfried Mar 13, 2017 •

edited

Loading

matklad commented Jan 20, 2018 •

edited

Loading