macros: fix inert attributes from `proc_macro_derives` with `#![feature(proc_macro)]` #39572

jseyfried · 2017-02-05T23:29:41Z

This PR refactors collection of proc_macro_derive invocations to fix #39347.

After this PR, the input to a #[proc_macro_derive] function no longer sees #[derive]s on the underlying item. For example, consider:

extern crate my_derives;
use my_derives::{Trait, Trait2};

#[derive(Copy, Clone)]
#[derive(Trait)]
#[derive(Trait2)]
struct S;

Today, the input to the Trait derive is #[derive(Copy, Clone, Trait2)] struct S;, and the input to the Trait2 derive is #[derive(Copy, Clone)] struct S;. More generally, a proc_macro_derive sees all builtin derives, as well as all proc_macro_derives listed after the one being invoked.

After this PR, both Trait and Trait2 will see struct S;.
This is a [breaking-change], but I believe it is highly unlikely to cause breakage in practice.

r? @nrc

jseyfried · 2017-02-05T23:32:45Z

An alternative would be for proc_macro_derives to see all builtin derives and proc_macro_derives applied to the item, e.g. both Trait and Trait2 would see
#[derive(Copy, Clone)] #[derive(Trait)] #[derive(Trait2)] struct S;.

jseyfried · 2017-02-05T23:33:13Z

cc #38356
cc @keeper @abonander

keeperofdakeys · 2017-02-06T09:16:15Z

src/librustc_metadata/creader.rs

-                );
+                let attrs = attributes.iter().cloned().map(Symbol::intern).collect::<Vec<_>>();
+                let derive = ProcMacroDerive::new(expand, attrs.clone());
+                let derive = SyntaxExtension::ProcMacroDerive(Box::new(derive), attrs);


Any reason you can't just store the ProcMacroDerive struct without boxing? Seems a waste to have two separate Vecs for the same thing.

I could, but it would require moving more code from libsyntax_ext into libsyntax. Still might be worth it, but probably best for another PR (perf. impact is negligible).

keeperofdakeys · 2017-02-06T09:21:13Z

src/librustc_resolve/macros.rs

@@ -172,7 +176,8 @@ impl<'a> base::Resolver for Resolver<'a> {
        ImportResolver { resolver: self }.resolve_imports()
    }

-    fn find_attr_invoc(&mut self, attrs: &mut Vec<ast::Attribute>) -> Option<ast::Attribute> {
+    fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<ast::Attribute>)


What makes this legacy, will it only resolve non-path attributes?

It only resolves legacy attribute macro invocations and legacy derive invocations (i.e. attributes and derives from #![plugin(...)]). I'll add a comment.

keeperofdakeys · 2017-02-06T09:46:58Z

src/librustc_resolve/macros.rs

+                    } else {
+                        attrs[i].value = ast::MetaItem {
+                            name: attrs[i].name(),
+                            span: span,


Should this be the span for the old derive attribute?

Yeah, good point -- will do.

keeperofdakeys · 2017-02-06T09:53:31Z

src/librustc_resolve/macros.rs

+                    let span = traits.remove(j).span;
+                    self.gate_legacy_custom_derive(legacy_name, span);
+                    if traits.is_empty() {
+                        attrs.remove(i);


Should this attribute be marked as used, or does that happen elsewhere?

It gets marked as used during expansion (here).

keeperofdakeys · 2017-02-06T10:02:50Z

src/libsyntax/ext/expand.rs

-            let expansion = match ext {
-                Some(ext) => self.expand_invoc(invoc, ext),
-                None => invoc.expansion_kind.dummy(invoc.span()),
+            let (expansion, new_invocations) = if let Some(ext) = ext {


This seems a bit complex, should there be a separate expand_derives function? (Two Some(ext)'s?)

Probably -- I was thinking maybe a separate collect_derives. I was thinking once we implement inert attribute vs macro attribute ambiguity errors (i.e. scope (b) from #39347 (comment)) and expanding items before derives, we'll have a better idea of how best to abstract / refactor out.

Ah, makes sense. I think it still deserves a comment for now at least.

keeperofdakeys · 2017-02-06T10:19:13Z

src/libsyntax/ext/expand.rs

+                return attrs;
+            }
+
+            traits = collect_derives(&mut self.cx, &attrs);


If there are multiple inert attributes, will collect_derives be called multiple times? If so, multiple derive warnings may occur.

No -- we don't call classify_item again after we collect a potential proc_macro_attribute invocation.
If the potential proc_macro_attribute invocation ends up being an inert attribute, we just mutate the invocation and re-resolve it (separate from this code-path).

I don't think I quite understand. What's the final for loop needed for?

keeperofdakeys · 2017-02-06T10:29:30Z

src/libsyntax/ext/expand.rs

+            _ => {}
+        }
+
+        for &(name, span) in traits {


I don't quite understand what's happening here. For each InvocKind::Attr, we have a possible instance of an inert attribute, then we loop through the traits seeing if it belongs to any invocation? And if it does, we simply add it to a dummy item, and mark it as used?

Yeah -- if it is an inert attribute, we mark it as known (so that it won't get collected as a proc_macro_attribute invocation again), add it back to underlying item (the dummy item is just a workaround for lack of easy mutable access to an Expansion's attributes), and put the invocation back on the work list.

nrc · 2017-02-09T02:18:12Z

cc @alexcrichton I feel like this discussion has come up before in the macros 1.1 process.

jseyfried · 2017-02-09T02:27:39Z

Yeah, c.f. #35900 (comment) and #37067.

nrc

r=me with the added comment, but I'd like to give Alex a chance to see this before merging

nrc · 2017-02-09T02:31:11Z

src/libsyntax/ext/base.rs

@@ -514,7 +514,7 @@ pub enum SyntaxExtension {
    /// The input is the annotated item.
    /// Allows generating code to implement a Trait for a given struct
    /// or enum item.
-    ProcMacroDerive(Box<MultiItemModifier>),
+    ProcMacroDerive(Box<MultiItemModifier>, Vec<Symbol>),


Could you comment on the additional arg please?

jseyfried · 2017-02-09T02:35:11Z

IIRC, #37067 was written when proc_macro_derives could modify the underlying item, so neither the current solution in this PR nor the solution from #39572 (comment) was possible.

alexcrichton · 2017-02-09T17:56:40Z

Yeah I think that the current behavior made more sense when custom derive could alter the item itself, but nowadays where it can't then this looks good to me!

(I can't think of any problems at least)

jseyfried · 2017-02-12T06:00:10Z

@bors r=nrc

bors · 2017-02-12T06:00:11Z

📌 Commit 1029ee0 has been approved by nrc

keeperofdakeys · 2017-02-12T06:09:47Z

Did you fix the span issue in find_legacy_attr_invoc?

bors · 2017-02-12T06:21:11Z

⌛ Testing commit 1029ee0 with merge c110a97...

…re(proc_macro)]`.

jseyfried · 2017-02-12T07:22:08Z

@keeperofdakeys Yeah.
@bors r=nrc

bors · 2017-02-12T07:22:09Z

📌 Commit 2cc61ee has been approved by nrc

jseyfried · 2017-02-12T23:21:11Z

@bors p=1

bors · 2017-02-12T23:21:18Z

⌛ Testing commit 2cc61ee with merge 956e2bc...

@nrc

macros: fix inert attributes from `proc_macro_derives` with `#![feature(proc_macro)]` This PR refactors collection of `proc_macro_derive` invocations to fix #39347. After this PR, the input to a `#[proc_macro_derive]` function no longer sees `#[derive]`s on the underlying item. For example, consider: ```rust extern crate my_derives; use my_derives::{Trait, Trait2}; #[derive(Copy, Clone)] #[derive(Trait)] #[derive(Trait2)] struct S; ``` Today, the input to the `Trait` derive is `#[derive(Copy, Clone, Trait2)] struct S;`, and the input to the `Trait2` derive is `#[derive(Copy, Clone)] struct S;`. More generally, a `proc_macro_derive` sees all builtin derives, as well as all `proc_macro_derive`s listed *after* the one being invoked. After this PR, both `Trait` and `Trait2` will see `struct S;`. This is a [breaking-change], but I believe it is highly unlikely to cause breakage in practice. r? @nrc

bors · 2017-02-13T01:54:31Z

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

nikomatsakis · 2017-04-05T17:49:47Z

I'm tagging this as relnotes since the there is certainly a change in behavior here.

…opy`" This reverts commit 4117fff. As of rust 1.17, proc-macros don't get attributes other than their own in the token stream they're fed. See rust-lang/rust#39572

rust-highfive assigned nrc Feb 5, 2017

jseyfried mentioned this pull request Feb 5, 2017

feature(proc_macro) breaks attributes on custom derive #39347

Closed

jseyfried force-pushed the fix_inert_attributes branch from e0e1eb0 to 1068ff0 Compare February 5, 2017 23:38

jseyfried changed the title ~~macros: Fix inert attributes from proc_macro_derives behind #![feature(proc_macro)]~~ macros: fix inert attributes from proc_macro_derives with #![feature(proc_macro)] Feb 6, 2017

jseyfried force-pushed the fix_inert_attributes branch 5 times, most recently from 1094f9f to b260146 Compare February 6, 2017 05:34

jseyfried mentioned this pull request Feb 6, 2017

Tracking issue for RFC 1566: Procedural macros #38356

Closed

31 tasks

keeperofdakeys reviewed Feb 6, 2017

View reviewed changes

jseyfried force-pushed the fix_inert_attributes branch from b260146 to 51e7f21 Compare February 6, 2017 12:43

nrc approved these changes Feb 9, 2017

View reviewed changes

Move legacy custom derives collection into resolver.find_attr_invoc().

4b413bc

jseyfried force-pushed the fix_inert_attributes branch from 51e7f21 to 1029ee0 Compare February 12, 2017 04:08

Allow using inert attributes from proc_macro_derives with `#![featu…

2cc61ee

…re(proc_macro)]`.

jseyfried force-pushed the fix_inert_attributes branch from 1029ee0 to 2cc61ee Compare February 12, 2017 07:20

bors merged commit 2cc61ee into rust-lang:master Feb 13, 2017

This was referenced Feb 13, 2017

Refactor macro resolution errors + add derive macro suggestions #39752

Merged

Simplify TokenTree and fix macro_rules! bugs #39419

Merged

jseyfried deleted the fix_inert_attributes branch February 18, 2017 04:42

dtolnay mentioned this pull request Mar 16, 2017

No attributes are available dtolnay/syn#108

Closed

iddm mentioned this pull request Mar 16, 2017

Unable to see derive attributes anymore #40574

Closed

This was referenced Apr 5, 2017

Regresion in rustls-0.5.7, Rust 1.17 #40963

Closed

Regression in fxsm, Rust 1.17 #40966

Closed

nikomatsakis added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 5, 2017

hcpl mentioned this pull request Apr 4, 2018

Update dependencies mcarton/rust-derivative#16

Closed

MihirLuthra mentioned this pull request May 31, 2021

Input to a #[proc_macro_derive] sees underlying derives on the item #85854

Closed

macros: fix inert attributes from proc_macro_derives with #![feature(proc_macro)] #39572

macros: fix inert attributes from proc_macro_derives with #![feature(proc_macro)] #39572

Conversation

jseyfried commented Feb 5, 2017

jseyfried commented Feb 5, 2017 • edited Loading

jseyfried commented Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried Feb 6, 2017 • 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

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 Feb 6, 2017 • edited by jseyfried Loading

Choose a reason for hiding this comment

jseyfried Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

nrc commented Feb 9, 2017

jseyfried commented Feb 9, 2017

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried commented Feb 9, 2017 • edited Loading

alexcrichton commented Feb 9, 2017

jseyfried commented Feb 12, 2017

bors commented Feb 12, 2017

keeperofdakeys commented Feb 12, 2017

bors commented Feb 12, 2017

jseyfried commented Feb 12, 2017

bors commented Feb 12, 2017

jseyfried commented Feb 12, 2017

bors commented Feb 12, 2017

bors commented Feb 13, 2017

nikomatsakis commented Apr 5, 2017

macros: fix inert attributes from `proc_macro_derives` with `#![feature(proc_macro)]` #39572

macros: fix inert attributes from `proc_macro_derives` with `#![feature(proc_macro)]` #39572

jseyfried commented Feb 5, 2017 •

edited

Loading

jseyfried commented Feb 5, 2017 •

edited

Loading

jseyfried Feb 6, 2017 •

edited

Loading

keeperofdakeys Feb 6, 2017 •

edited by jseyfried

Loading

jseyfried Feb 6, 2017 •

edited

Loading

jseyfried commented Feb 9, 2017 •

edited

Loading