macros 1.1: Allow proc_macro functions to declare attributes to be mark as used #37614

keeperofdakeys · 2016-11-06T12:01:49Z

This PR allows proc macro functions to declare attribute names that should be marked as used when attached to the deriving item. There are a few questions for this PR.

Currently this uses a separate attribute named #[proc_macro_attributes(..)], is this the best choice?
In order to make this work, the check_attribute function had to be modified to not error on attributes marked as used. This is a pretty large change in semantics, is there a better way to do this?
I've got a few clones where I don't know if I need them (like turning item into a TokenStream), can these be avoided?
Is switching to MultiItemDecorator the right thing here?

Also fixes #37563.

rust-highfive · 2016-11-06T12:01:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton

Awesome, thanks! Could you also be sure to add a number of tests for this functionality as well? It'd also be good to see some feature-gate related tests to ensure this attribute is gated appropriately as well.

I believe you'll also need to update the existing tests as the semantics of whether the input item is expected in the output has changed.

alexcrichton · 2016-11-06T16:51:04Z

src/libsyntax/feature_gate.rs

@@ -820,7 +826,7 @@ impl<'a> Context<'a> {
            // feature gate checking. Macro gating runs
            // before the plugin attributes are registered
            // so we skip this then
-            if !is_macro {
+            if !is_macro && !attr::is_used(attr) {


How come this part was modified as well? Presumably this means that we're not gating unused attributes now?

Using mark_used appears to only disable the unused_attribute lint, not the custom_attributes error. So without this change the compiler will still give the custom attribute error for used attributes. The only alternative I can see is registering the attribute name in plugin_attributes.

@alexcrichton It means we're not custom_attribute-checking used attributes, which should be fine because unknown/custom attributes should never be mark_used.

"because unknown/custom attributes should never be mark_used" - where is this enforced?

alexcrichton · 2016-11-06T16:52:44Z

src/libsyntax_ext/proc_macro_registrar.rs

+        let proc_attrs: Vec<_> = item.attrs.iter()
+            .filter(|a| a.check_name("proc_macro_attributes"))
+            .filter_map(|a|
+              a.meta_item_list().or_else(|| {


This spacing/indentation here is a bit difficult to read, could this be updated to follow the surrounding code?

keeperofdakeys · 2016-11-06T21:32:21Z

Thanks for the feedback, I'll work on the tests and get back to you.

jseyfried · 2016-11-06T21:56:14Z

src/librustc_metadata/creader.rs

-                let derive = SyntaxExtension::CustomDerive(Box::new(CustomDerive::new(expand)));
+                                      expand: fn(TokenStream) -> TokenStream,
+                                      attributes: &[&'static str]) {
+                let attrs: Vec<_> = attributes.iter().map(|s| InternedString::new(s)).collect();


nit: .map(|s| InternedString::new(s)) -> .map(InternedString::new)

jseyfried · 2016-11-06T21:56:36Z

src/librustc_metadata/creader.rs

+                                      attributes: &[&'static str]) {
+                let attrs: Vec<_> = attributes.iter().map(|s| InternedString::new(s)).collect();
+                let derive = SyntaxExtension::CustomDerive(
+                    Box::new(CustomDerive::new(expand, &attrs))


This should pass attrs directly instead of by reference so that CustomDerive::new doesn't need to clone.

jseyfried · 2016-11-06T21:57:21Z

src/libsyntax/ext/base.rs

@@ -509,7 +509,7 @@ pub enum SyntaxExtension {
    ///
    IdentTT(Box<IdentMacroExpander>, Option<Span>, bool),

-    CustomDerive(Box<MultiItemModifier>),
+    CustomDerive(Box<MultiItemDecorator>),


This should stay MultiItemModifier.

Why? I'd have thought Decorator is more appropriate?

jseyfried · 2016-11-06T21:58:01Z

src/libsyntax/feature_gate.rs

+                                            "proc_macro",
+                                            "the `#[proc_macro_attributes]` attribute \
+                                             is an experimental feature",
+                                            cfg_fn!(proc_macro))),


We don't need to add a new kind of attribute -- extending proc_macro_derive is fine.

jseyfried · 2016-11-06T22:00:16Z

src/libsyntax_ext/deriving/custom.rs

-use syntax::print::pprust;
+use syntax::parse::token::InternedString;
+
+fn mark_attrs_used<I: Iterator<Item=Attribute>>(attrs: I, whitelist: &[InternedString]) {


This should be a Visitor.

e.g. something like

struct MarkAttrsUsed<'a>(&'a [InternedString]); impl<'a> Visitor for MarkAttrsUsed<'a> { fn visit_attribute(&mut self, attr: &Attribute) { if self.0.contains(attr.name()) { attr::mark_used(attr); } } }

jseyfried · 2016-11-06T22:00:39Z

src/libsyntax_ext/deriving/custom.rs

-    pub fn new(inner: fn(TokenStream) -> TokenStream) -> CustomDerive {
-        CustomDerive { inner: inner }
+    pub fn new(inner: fn(TokenStream) -> TokenStream,
+               attrs: &[InternedString])


This should take a Vec<InternedString>.

jseyfried · 2016-11-06T22:01:11Z

src/libsyntax_ext/deriving/custom.rs

+                                &self.attrs),
+            ItemKind::Enum(ref def, ..) =>
+                mark_attrs_used(def.variants.iter().flat_map(|f| f.node.attrs.iter()).cloned(),
+                                &self.attrs),


If we use a visitor, these calls should be able to be replaced with MarkAttrsUsed(&self.attrs).visit_item(item_ref).

jseyfried · 2016-11-06T22:02:03Z

src/libsyntax_ext/deriving/custom.rs

-        // macros, everything is just parsed as a string. Reassign all spans to
-        // the input `item` for better errors here.
-        item.into_iter().flat_map(|item| {
-            ChangeSpan { span: input_span }.fold_item(item)


We still want to ChangeSpan the new impls generated by custom derives.

nrc · 2016-11-07T02:34:47Z

Currently this uses a separate attribute named #[proc_macro_attributes(..)], is this the best choice?

What is the motivation for that? I'd prefer a single attribute as proposed by @eddyb.

In order to make this work, the check_attribute function had to be modified to not error on attributes marked as used. This is a pretty large change in semantics, is there a better way to do this?

I would prefer not to do this. I think we should keep separate the concepts of used and allowed attributes. White-listing an attribute should make it allowed (scoped to the derive uses, so I guess the white-list has to be per-derive) and mark any uses as used. Other attributes could be marked used by a proc macro, but should still trigger the custom attribute lint.

Is switching to MultiItemDecorator the right thing here?

Yes.

keeperofdakeys · 2016-11-07T14:37:40Z

So a few things I've done:

I've renamed KNOWN_ATTRIBUTES to BUILTIN_ATTRIBUTES, and made KNOWN_ATTRIBUTES similar in scope to USED_ATTRIBUTES. CustomDerive then uses attr::mark_known so that the attribute won't trigger a custom attributes error.
After discussing this on IRC, I've changed back to MultiItemModifier. It has a better call-signature, and CustomDerive may end up having its own copy as a separate trait.
I've updated the tests a bit, but I still need to write some failing tests for custom attributes errors.

nrc · 2016-11-07T18:41:32Z

r? @jseyfried

jseyfried

Looks great! r=me modulo nits.
Could you squash some of the update commits and force-push?

jseyfried · 2016-11-07T20:15:21Z

src/libsyntax/attr.rs

 }

+


nit: extra newline

jseyfried · 2016-11-07T20:17:15Z

src/libsyntax_ext/proc_macro_registrar.rs

@@ -133,7 +134,8 @@ impl<'a> Visitor for CollectCustomDerives<'a> {
        }

        // Once we've located the `#[proc_macro_derive]` attribute, verify
-        // that it's of the form `#[proc_macro_derive(Foo)]`
+        // that it's of the form `#[proc_macro_derive(Foo)]` or
+        // `#[proc_macro_derive(Foo attributes(A, ..))]`


nit: missing comma after Foo.

jseyfried · 2016-11-07T20:22:36Z

src/libsyntax_ext/proc_macro_registrar.rs

-            self.handler.span_err(attr.span(),
-                                  "attribute must only have one argument");
-            return
+        let mut attributes_attr = None;


I think it would be more idiomatic to change the condition of the original if to list.len() != 1 && list.len() != 2 and just add let attributes_attr = list.get(1); instead of the match.

jseyfried · 2016-11-07T20:24:35Z

src/libsyntax_ext/proc_macro_registrar.rs

+                self.handler.span_err(attr.span(),
+                                      "attribute must be of form: \
+                                       `attributes(foo, bar)`");
+                None


nit: None -> Vec::new() and or_else -> unwrap_or_else would avoid the flat_map below.

jseyfried · 2016-11-07T20:25:50Z

src/libsyntax_ext/proc_macro_registrar.rs

+            2 => attributes_attr = Some(&list[1]),
+            _ => {
+                self.handler.span_err(attr.span(),
+                                      "attribute must only have between one and two arguments");


I think "attribute must have either one or two arguments" would be clearer.

KNOWN_ATTRIBUTES should really be named BUILT_ATTRIBUTES, while KNOWN_ATTRIBUTES should be used to mark attributes as known, similar to USED_ATTRIBUTES.

keeperofdakeys · 2016-11-08T10:48:27Z

I've added some basic tests for the proc_macro_derive attribute form with an attributes list, and for custom attribute errors in deriving items. So unless there are more changes that you'd like me to make, this should be ready to merge.

keeperofdakeys · 2016-11-08T11:06:17Z

What changes would need to be made to fix #37614? Is it as simple as reverting the changes made in 3784067? I've checked the example item errors, and they are successfully reported on the correct item span thanks to this PR.

jseyfried · 2016-11-08T11:02:00Z

src/libsyntax_ext/proc_macro_registrar.rs

+                                       `attributes(foo, bar)`");
+                &[]
+            }).into_iter()
+            .filter_map(|attr| {


optional formatting nit:
It would be more idiomatic to either give }) its own line or have a single line }).into_iter().filter_map(|attr| {.

jseyfried · 2016-11-08T11:20:14Z

What changes would need to be made to fix #37614?

Reverting 3784067, adding a regression test, and editing this PR's initial comment to include "fixes #37614".

By using a second attribute `attributes(Bar)` on proc_macro_derive, whitelist any attributes with the name `Bar` in the deriving item. This allows a proc_macro function to use custom attribtues without a custom attribute error or unused attribute lint.

This reverts commit 3784067. Any errors in the derived output now point at the derive attribute instead of the item.

keeperofdakeys · 2016-11-08T22:49:06Z

I've submitted the required changes to fix #37563. This should be ready to merge now.

jseyfried · 2016-11-08T22:58:24Z

Excellent, thanks @keeperofdakeys!
@bors r+

bors · 2016-11-08T22:58:35Z

📌 Commit 134ef4f has been approved by jseyfried

macros 1.1: Allow proc_macro functions to declare attributes to be mark as used This PR allows proc macro functions to declare attribute names that should be marked as used when attached to the deriving item. There are a few questions for this PR. - Currently this uses a separate attribute named `#[proc_macro_attributes(..)]`, is this the best choice? - In order to make this work, the `check_attribute` function had to be modified to not error on attributes marked as used. This is a pretty large change in semantics, is there a better way to do this? - I've got a few clones where I don't know if I need them (like turning `item` into a `TokenStream`), can these be avoided? - Is switching to `MultiItemDecorator` the right thing here? Also fixes rust-lang#37563.

Rollup of 15 pull requests - Successful merges: #36868, #37134, #37229, #37250, #37370, #37428, #37432, #37472, #37524, #37614, #37622, #37627, #37636, #37644, #37654 - Failed merges: #37463, #37542, #37645

nikomatsakis · 2016-11-17T22:58:55Z

Just saw this; thanks for jumping on this!

rust-highfive assigned arielb1 Nov 6, 2016

keeperofdakeys force-pushed the proc_macro branch from 955688e to 3030be2 Compare November 6, 2016 12:03

keeperofdakeys mentioned this pull request Nov 6, 2016

Tracking issue for "Macros 1.1" (RFC #1681) #35900

Closed

53 tasks

alexcrichton reviewed Nov 6, 2016

View reviewed changes

jseyfried reviewed Nov 6, 2016

View reviewed changes

rust-highfive assigned jseyfried and unassigned arielb1 Nov 7, 2016

jseyfried mentioned this pull request Nov 7, 2016

Using $crate with a proc macro #37637

Closed

jseyfried reviewed Nov 7, 2016

View reviewed changes

Rename KNOWN_ATTRS to BUILT_ATTRS, and create KNOWN_ATTRS

d377cf5

KNOWN_ATTRIBUTES should really be named BUILT_ATTRIBUTES, while KNOWN_ATTRIBUTES should be used to mark attributes as known, similar to USED_ATTRIBUTES.

keeperofdakeys force-pushed the proc_macro branch 3 times, most recently from 784aa0d to 96eca8b Compare November 8, 2016 10:48

jseyfried approved these changes Nov 8, 2016

View reviewed changes

keeperofdakeys added 2 commits November 8, 2016 23:03

Revert "Point macros 1.1 errors to the input item"

134ef4f

This reverts commit 3784067. Any errors in the derived output now point at the derive attribute instead of the item.

keeperofdakeys force-pushed the proc_macro branch from 96eca8b to 134ef4f Compare November 8, 2016 12:37

eddyb mentioned this pull request Nov 9, 2016

Rollup of 15 pull requests #37670

Merged

bors merged commit 134ef4f into rust-lang:master Nov 9, 2016

dtolnay mentioned this pull request Nov 9, 2016

Update to use proc_macro_derive attributes serde-rs/serde#614

Closed

est31 mentioned this pull request Nov 16, 2016

Failure when using num_derive on latest nightly rust-num/num#240

Closed

dtolnay mentioned this pull request Nov 28, 2016

[Servo release build] attribute must only have on argument serde-rs/serde#630

Closed

dtolnay mentioned this pull request Dec 30, 2016

proc_macro does not support type macros #38706

Closed

dtolnay mentioned this pull request Jan 7, 2017

Factor attribute-stripping into a fold, similar to libsyntax serde-rs/serde#561

Closed

keeperofdakeys deleted the proc_macro branch February 2, 2017 21:50

dtolnay mentioned this pull request Jan 20, 2018

Poor error message when user forgets derive that has attributes #47608

Open

alercah mentioned this pull request Jul 10, 2018

rustc: Stabilize the proc_macro feature #52081

Merged

alexcrichton mentioned this pull request Jul 10, 2018

Ambiguity between custom derive attributes and proc macro attributes #52226

Closed

macros 1.1: Allow proc_macro functions to declare attributes to be mark as used #37614

macros 1.1: Allow proc_macro functions to declare attributes to be mark as used #37614

Conversation

keeperofdakeys commented Nov 6, 2016 • edited Loading

rust-highfive commented Nov 6, 2016

alexcrichton 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

keeperofdakeys commented Nov 6, 2016

jseyfried Nov 6, 2016 • edited Loading

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

jseyfried Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

jseyfried Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

jseyfried Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Nov 7, 2016

keeperofdakeys commented Nov 7, 2016

nrc commented Nov 7, 2016

jseyfried 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

keeperofdakeys commented Nov 8, 2016

keeperofdakeys commented Nov 8, 2016

Choose a reason for hiding this comment

jseyfried commented Nov 8, 2016

keeperofdakeys commented Nov 8, 2016

jseyfried commented Nov 8, 2016 • edited Loading

bors commented Nov 8, 2016

nikomatsakis commented Nov 17, 2016

keeperofdakeys commented Nov 6, 2016 •

edited

Loading

jseyfried Nov 6, 2016 •

edited

Loading

jseyfried Nov 7, 2016 •

edited

Loading

jseyfried Nov 6, 2016 •

edited

Loading

jseyfried Nov 6, 2016 •

edited

Loading

jseyfried commented Nov 8, 2016 •

edited

Loading