[WIP] Expand `#[derive(..)]`s in the InvocationCollector #39391

keeperofdakeys · 2017-01-29T11:40:21Z

Expand derive attributes in the InvocationCollector

This allows builtin derives to be registered and resolved, just like other derive types.

keeperofdakeys · 2017-01-29T11:42:06Z

@jseyfried This code could is a bit rough, and it currently has a bug that means it only works with legacy custom derives, and builtin derives. I'm hoping that a second set of eyes will help me spot the error I'm making.

keeperofdakeys · 2017-01-29T11:44:14Z

cc #38356

keeperofdakeys · 2017-01-29T11:56:00Z

Cause of bug:

22:21 < jseyfried> The problem is that no proc macro derives have been imported yet when we are doing invocation collection

jseyfried · 2017-01-29T11:47:42Z

src/libsyntax/ext/derive.rs

+pub fn is_derive_attr(attr: &ast::Attribute) -> bool {
+    let res = attr.name() == Symbol::intern("derive");
+    res
+}


nit: This can just be attr.name() == "derive" (I would inline and remove the function)

jseyfried · 2017-01-29T12:02:55Z

src/libsyntax/ext/derive.rs

+}
+
+pub fn get_derive_attr(cx: &mut ExtCtxt, attrs: &mut Vec<ast::Attribute>,
+                       is_derive_type: fn(&mut ExtCtxt, &NestedMetaItem) -> bool)


I think it would be a little cleaner if this were an enum (three variants) and is_derive_type(...) were a match.

jseyfried · 2017-01-29T12:12:04Z

src/libsyntax/ext/expand.rs

+                    Annotatable::TraitItem(P(fully_configure!(self, item, noop_fold_trait_item)));
+                return self.collect_attr(attr, item, ExpansionKind::TraitItems).make_trait_items()
+            } else {
+                self.cx.span_err(attr.span, "`derive` can't be only be applied to items");


Could we move this logic into collect_attr?
e.g.

if attr.name() == "derive" && (kind == ExpansionKind::TraitItems || ExpansionKind::ImplItems) { ... }

What would you return fromcollect_attr for the error case? The callers assume that collect_attr will always return a valid Expansion.

It could just return the original item (via kind.expect_from_annotatables(iter::once(item))).

jseyfried · 2017-01-29T12:14:02Z

src/libsyntax_ext/deriving/custom.rs

@@ -54,7 +54,7 @@ impl MultiItemModifier for CustomDerive {
            Annotatable::Item(item) => item,
            Annotatable::ImplItem(_) |
            Annotatable::TraitItem(_) => {
-                ecx.span_err(span, "custom derive attributes may only be \
+                ecx.span_err(span, "proc_macro_derive attributes may only be \


"proc_macro_derive attribute" makes me think of #[proc_macro_derive] itself rather than #[derive(Trait)].
Maybe "proc-macro derives may only be ..."?

jseyfried · 2017-01-29T12:17:57Z

src/libsyntax/ext/derive.rs

+}
+
+pub fn verify_derive_attrs(cx: &mut ExtCtxt, attrs: &mut Vec<ast::Attribute>) {
+    for i in 0..attrs.len() {


nit: for attr in attrs { ... } is more idiomatic

jseyfried · 2017-01-29T12:27:33Z

src/libsyntax/ext/expand.rs

+                    get_proc_macro_derive(self.cx, &mut attrs)
+                }).or_else(|| {
+                    add_structural_marker(self.cx, &mut attrs);
+                    add_copy_clone_marker(self.cx, &mut attrs);


I think we could merge these two function into a single function add_derived_markers that computes a Vec<Name> (or Vec<MetaItem>) of all the derived trait names and then just uses e.g. vec.contains(&Symbol::intern("Copy")) || vec.contains(&Symbol::intern("Clone"))

One annoying thing is that currently we need some span to add for the new attribute. If we make a Vec<Name>, we'll either need to grab the span of the first attribute, or if possible make a dummy span.

I think either would be fine (provided the span is allow_internal_unstable).

keeperofdakeys · 2017-01-30T13:40:04Z

We're kind of breaking the semantics of the InvocationCollector with this change, which causes serious breakage. We're trying to resolve derives during collection, when the InvocationCollector usually resolves separately itself. With Foo as a proc_macro_derive, this works: #[derive(Copy, Foo), this doesn't: #[derive(Foo)] (it's never expanded at all) - as you suggested, this is probably because the first gives #[macro_use] time to be expanded.

The fix for this seems to be moving the resolution of types of derives into the invocation expansion loop. However I'm not sure how to do the ordering of derives if this is done. Maybe it's enough to resolve custom_derives in collect_attr (which become InvocKind::Attr anyway), and leave proc_macro_derive vs builtin to the resolution in the invocation loop. Though I'm not sure how detection of undefined derives works here.

This means that proc_macro_derives that haven't been imported yet don't get ignored. This is safe because legacy_derives and builtin derives are instantly resolvable.

jseyfried · 2017-01-31T07:04:38Z

src/libsyntax/ext/derive.rs

+        return None;
+    }
+
+    let traits = attr.meta_item_list().unwrap();


This looks like it might panic on #[derive].

jseyfried · 2017-01-31T07:05:27Z

src/libsyntax/ext/derive.rs

+    return traits.get(0);
+}
+
+pub fn verify_derive_attrs(cx: &mut ExtCtxt, attrs: &mut Vec<ast::Attribute>) {


nit: I think attrs: &[ast::Attribute] is sufficient here

jseyfried · 2017-01-31T09:41:18Z

src/libsyntax/ext/derive.rs

+}
+
+impl DeriveType {
+    pub fn is_derive_type(&self, cx: &mut ExtCtxt, titem: &NestedMetaItem) -> bool {


Could we instead have a method DeriveType::classify(name, cx) here? The function would check legacy first (returning DeriveType::Legacy if we find derive_* in builtin_macros), then check builtins (returning DeriveType::Builtin), then finally return DeriveType::ProcMacro.

e.g. is_legacy_derive(cx, titem) would become DeriveType::classify(titem.name(), cx) == DeriveType::Legacy.

Even better, we could move this logic into cx.resolver.find_attr_invoc() or a new method of ext::base::Resolver.

jseyfried · 2017-01-31T09:48:02Z

src/libsyntax/ext/derive.rs

+
+    if !attrs.iter().any(|a| a.name() == "structural_match") &&
+       titems.iter().any(|t| *t == "PartialEq") && titems.iter().any(|t| *t == "Eq") {
+       let structural_match = Symbol::intern("structural_match");


nit: indent the if body one more space

jseyfried · 2017-01-31T09:48:12Z

src/libsyntax/ext/derive.rs

+
+    if !attrs.iter().any(|a| a.name() == "rustc_copy_clone_marker") &&
+       titems.iter().any(|t| *t == "Copy") && titems.iter().any(|t| *t == "Clone") {
+       let structural_match = Symbol::intern("structural_match");


(same here)

jseyfried · 2017-01-31T10:04:44Z

src/libsyntax/ext/derive.rs

+    let tname = titem.name().unwrap();
+    let name = Symbol::intern(&format!("derive_{}", tname));
+    let path = ast::Path::from_ident(titem.span, ast::Ident::with_empty_ctxt(name));
+        cx.resolver.resolve_macro(cx.current_expansion.mark, &path, false).is_ok()


Ideally we would just resolve in resolver.builtin_macros, either by moving this logic into resolve or adding a method to check builtin macros directly to ext::base::Resolver.

jseyfried · 2017-01-31T10:05:11Z

src/libsyntax/ext/derive.rs

+pub fn is_builtin_derive(cx: &mut ExtCtxt, titem: &NestedMetaItem) -> bool {
+    let tname = titem.name().unwrap();
+    let derive_mode = ast::Path::from_ident(titem.span, ast::Ident::with_empty_ctxt(tname));
+    cx.resolver.resolve_macro(cx.current_expansion.mark, &derive_mode, false).map(|ext| {


(same here, re resolver.builtin_macros)

jseyfried · 2017-01-31T10:06:33Z

src/libsyntax/ext/expand.rs

@@ -856,7 +957,8 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {

        let (item, attr) = self.classify_item(item);
        if let Some(attr) = attr {
-            let item = Annotatable::ImplItem(P(fully_configure!(self, item, noop_fold_impl_item)));
+            let item =
+                Annotatable::ImplItem(P(fully_configure!(self, item, noop_fold_impl_item)));


nit: stray change

keeperofdakeys · 2017-01-31T10:16:15Z

I implemented the change that you suggested, and all the serious test failures are now gone. It's down to trivial test failures, namely:

Using derive as a macro like this derive!() now gives the error macro undefined: 'derive!', when before it gave 'derive' can only be used in attributes. This is a natural consequence of removing derive as a resolvable macro. Should there be a dummy macro now, since you can't use derive as an attribute macro anyway?

An invalid derive attribute (like #[derive(Zero)]) now gives the error macro undefined: 'Zero!', where as before it gave '#[derive]' for custom traits is not stable enough for use. It is deprecated and will be removed in v1.15 (see issue #29644). Obviously an improvement, but could this be improved even more?

jseyfried · 2017-01-31T10:37:32Z

@keeperofdakeys

Should there be a dummy macro now, since you can't use derive as an attribute macro anyway?

I don't think it matters that much -- macro undefined: 'derive!' is fine.

but could [invalid derive attribute diagnostics] be improved even more?

Yeah, by changing from macro undefined: 'Zero!' to macro undefined: 'Zero' (removing !). Even better -- something like undefined custom derive `Zero` .

keeperofdakeys · 2017-01-31T21:10:48Z

I've pushed some fixes for the latest test failures, there will probably be some more though.

Arnavion · 2017-01-31T21:47:56Z

What is the new error message for the situation described in #39326 ?

keeperofdakeys · 2017-01-31T21:59:16Z

I was thinking something like `Derive macro 'Foo' does not exist`. It isn't in the PR yet as things are still being refactored. In a future PR, I was thinking of doing suggestions for (at least) builtin derives.

jseyfried

r=me with tests passing (all comments optional)

jseyfried · 2017-02-01T06:25:50Z

src/libsyntax/ext/derive.rs

+                tname
+            } else {
+                continue;
+            };


nit: using a match is more idiomatic here

jseyfried · 2017-02-01T06:27:43Z

src/libsyntax/ext/derive.rs

+    let span = attrs[0].span;
+
+    if !attrs.iter().any(|a| a.name() == "structural_match") &&
+        titems.iter().any(|t| *t == "PartialEq") && titems.iter().any(|t| *t == "Eq") {


nit: this line should be un-indented one space.

jseyfried · 2017-02-01T06:27:48Z

src/libsyntax/ext/derive.rs

+    }
+
+    if !attrs.iter().any(|a| a.name() == "rustc_copy_clone_marker") &&
+        titems.iter().any(|t| *t == "Copy") && titems.iter().any(|t| *t == "Clone") {


(same here)

jseyfried · 2017-02-01T06:28:04Z

src/libsyntax/ext/derive.rs

+                        -> Option<ast::Attribute> {
+    verify_derive_attrs(cx, attrs);
+    get_derive_attr(cx, attrs, DeriveType::Legacy).and_then(|a| {
+            let titem = derive_attr_trait(cx, &a);


nit: too much indent

jseyfried · 2017-02-01T06:28:47Z

src/libsyntax/ext/derive.rs

+                    return Some(cx.attribute(mitem.span, mitem));
+                }
+            }
+            None


Using an else to avoid the above return Some is more idiomatic

jseyfried · 2017-02-01T06:29:08Z

src/libsyntax/ext/derive.rs

+                         "custom_derive",
+                         titem.span,
+                         feature_gate::GateIssue::Language,
+                         feature_gate::EXPLAIN_CUSTOM_DERIVE


the above four lines should be un-indented a space

jseyfried · 2017-02-01T06:29:16Z

src/libsyntax/ext/derive.rs

+                    let name = Symbol::intern(&format!("derive_{}", tname));
+                    if !cx.resolver.is_whitelisted_legacy_custom_derive(name) {
+                        cx.span_warn(titem.span,
+                                          feature_gate::EXPLAIN_DEPR_CUSTOM_DERIVE);


(wrong indent)

keeperofdakeys · 2017-02-01T10:30:22Z

Travis seems unhappy with this PR, but happy with others. So try creating a new PR for all this.

keeperofdakeys · 2017-02-01T10:39:07Z

Moved to #39442

keeperofdakeys added 3 commits January 26, 2017 22:35

Make builtin derives a SyntaxExtension

6690cfe

This allows builtin derives to be registered and resolved, just like other derive types.

Rename CustomDerive to ProcMacroDerive for macros 1.1

bcf859c

Move derive expansion into the InvocationCollector

4331790

rust-highfive assigned jseyfried Jan 29, 2017

keeperofdakeys closed this Jan 29, 2017

keeperofdakeys reopened this Jan 29, 2017

jseyfried reviewed Jan 29, 2017

View reviewed changes

jseyfried mentioned this pull request Jan 29, 2017

Tracking issue for RFC 1566: Procedural macros #38356

Closed

31 tasks

keeperofdakeys added 5 commits January 30, 2017 08:06

Update proc-macro derive error messages

8764828

Fix various expand_derive issues

f439179

Use trait spans instead of attribute spans

e70d4ef

Move derive item check to collect_attr

dafc42d

Combine derive markers into one function

2dfa944

keeperofdakeys mentioned this pull request Jan 31, 2017

Proc macro gives bad error about custom derives if regular macro with the same name is imported first #39326

Closed

keeperofdakeys added 2 commits January 31, 2017 19:50

Let get_proc_macro_derive collect unknown derives

dae6850

This means that proc_macro_derives that haven't been imported yet don't get ignored. This is safe because legacy_derives and builtin derives are instantly resolvable.

Fix meta_item_list in add_derived_markers

dd05fc0

jseyfried reviewed Jan 31, 2017

View reviewed changes

Split derives in test because one triggers before the other

2dd9dc2

keeperofdakeys added 4 commits January 31, 2017 23:31

macro resolving refactor, get_derive_attr code refactor, and nits

a914a23

Move files around + line ending fixes

6c22abe

Mark empty derive attributes as used (no traits is a warning)

242f3f3

Fix rustc_copy_clone_marker typo

7559574

jseyfried reviewed Feb 1, 2017

View reviewed changes

keeperofdakeys added 3 commits February 1, 2017 20:12

Add explicit derive macro resolver

8b27f76

Various fixes

2601076

Don't force when resolving a derive macro

65858e0

keeperofdakeys closed this Feb 1, 2017

[WIP] Expand #[derive(..)]s in the InvocationCollector #39391

[WIP] Expand #[derive(..)]s in the InvocationCollector #39391

Conversation

keeperofdakeys commented Jan 29, 2017

keeperofdakeys commented Jan 29, 2017

keeperofdakeys commented Jan 29, 2017

keeperofdakeys commented Jan 29, 2017

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 commented Jan 30, 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

keeperofdakeys commented Jan 31, 2017

jseyfried commented Jan 31, 2017 • edited Loading

keeperofdakeys commented Jan 31, 2017 • edited Loading

Arnavion commented Jan 31, 2017

keeperofdakeys commented Jan 31, 2017 via email • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeperofdakeys commented Feb 1, 2017

keeperofdakeys commented Feb 1, 2017

[WIP] Expand `#[derive(..)]`s in the InvocationCollector #39391

[WIP] Expand `#[derive(..)]`s in the InvocationCollector #39391

keeperofdakeys commented Jan 30, 2017 •

edited

Loading

jseyfried commented Jan 31, 2017 •

edited

Loading

keeperofdakeys commented Jan 31, 2017 •

edited

Loading

keeperofdakeys commented Jan 31, 2017 via email •

edited

Loading