Expand derive macros in the MacroExpander #39442

keeperofdakeys · 2017-02-01T10:37:59Z

This removes the expand_derives function, and sprinkles the functionality throughout the Invocation Collector, Expander and Resolver.

Fixes #39326

r? @jseyfried

keeperofdakeys · 2017-02-01T10:38:28Z

r? @jseyfried

jseyfried · 2017-02-01T11:08:40Z

src/librustc_resolve/macros.rs

+            },
+            Err(Determinacy::Undetermined) if force => {
+                let path: Vec<_> = segments.iter().map(|seg| seg.identifier).collect();
+                let msg = format!("derive macro does not exist: '{}'", path[0].name);


Can you just the original path here directly?
Eventually we'll allow e.g. #![derive(foo::bar)].
path.segments[0].identifier.name would also work for now, but I see no reason to collect a path: Vec<_>.

Also, how about "cannot find derive macro {} in this scope"?
c.f. this error

jseyfried · 2017-02-01T13:05:17Z

src/libsyntax/ext/derive.rs

+        add_derived_markers(cx, attrs);
+        get_derive_attr(cx, attrs, DeriveType::Builtin)
+    }).or_else(|| {
+        get_derive_attr(cx, attrs, DeriveType::ProcMacro)


Could we switch the order here (i.e. ProcMacro first, then add_derived_markers/Builtin) for back-compat?

Since we want the input of proc macro derives to be fully expanded, we'll want to move in the direction of not including builtin derives, but I'd like to do that all at once with a Crater run.

keeperofdakeys · 2017-02-01T22:14:24Z

So this PR has removed derive from the macro resolver, which has an interesting consequence. The first piece of code works on stable today, the second piece doesn't (but should with this change as it is currently). Obviously derive as an attribute macro is not going to work. Should anything be changed here?

// Works today
macro_rules! derive {
    () => { println!("HI!"); }
}

fn main() {
    derive!();
}

macro_rules! derive {
    () => { println!("HI!"); }
}

// Doesn't work today, except if this appears before the macro_rules!.
#[derive(Default)]
struct A;

fn main() {
    derive!();
}

jseyfried · 2017-02-01T22:41:35Z

@keeperofdakeys
I don't think anything needs changing. I see this is a minor improvement, but since we're already fully special-casing #[derive(...)] I don't think it matters that much so long as we don't break things :)

jseyfried · 2017-02-02T04:07:33Z

@bors r+

bors · 2017-02-02T04:07:33Z

📌 Commit b117bee has been approved by jseyfried

@jseyfried

…eyfried Expand derive macros in the MacroExpander This removes the expand_derives function, and sprinkles the functionality throughout the Invocation Collector, Expander and Resolver. r? @jseyfried

@jseyfried

…eyfried Expand derive macros in the MacroExpander This removes the expand_derives function, and sprinkles the functionality throughout the Invocation Collector, Expander and Resolver. r? @jseyfried

frewsxcv · 2017-02-04T16:10:40Z

This PR might have resulted in this error during the rollup:

https://travis-ci.org/rust-lang/rust/jobs/198341670

#39537

---- [pretty] pretty/attr-variant-data.rs stdout ----

	

error: pretty-printed source does not typecheck

status: exit code: 101

command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc - -Zno-trans --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/pretty/attr-variant-data.pretty-out --target=x86_64-unknown-linux-gnu -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/pretty -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/pretty/attr-variant-data.stage2-x86_64-unknown-linux-gnu.pretty.libaux -Crpath -O -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers

stdout:

------------------------------------------

------------------------------------------

stderr:

------------------------------------------

error: cannot find derive macro `Serialize` in this scope

  --> <anon>:46:10

   |

46 | #[derive(Serialize, Deserialize)]

   |          ^^^^^^^^^

error: cannot find derive macro `Serialize` in this scope

  --> <anon>:30:10

   |

30 | #[derive(Serialize, Deserialize)]

   |          ^^^^^^^^^

error: cannot find derive macro `Serialize` in this scope

  --> <anon>:22:10

   |

22 | #[derive(Serialize, Deserialize)]

   |          ^^^^^^^^^

error: cannot find derive macro `Serialize` in this scope

  --> <anon>:19:10

   |

19 | #[derive(Serialize, Deserialize)]

   |          ^^^^^^^^^

error: aborting due to 4 previous errors

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

This removes the expand_derives function, and sprinkles the functionality throughout the Invocation Collector, Expander and Resolver.

keeperofdakeys · 2017-02-05T00:30:35Z

@jseyfried So it looks like travis didn't run the pretty tests, one of which I'm failing (which I've confirmed locally). It involves parsing and pretty printing derive attributes? I'm not sure how the pretty printer is linked to the invocation collector though. Any thoughts?

keeperofdakeys · 2017-02-05T01:40:33Z

Oh I think I understand now. With the custom_derive feature active, unknown derive macros were previously a warning. So the pretty print didn't bail out - however now it's an error.

The simple fix would be to remove the derives, or change them to builtin variants. Then maybe rename the serde attributes to something nonsensical, because it no longer makes semantic sense if the derive macros are no longer serde's.

Remove attr-variant-data.rs since it relies on quirks in legacy custom derive resolution (undefined derives only print a warning). Add a new test which uses a defined proc macro derive, and tests pretty printing of proc macro derive attributes.

keeperofdakeys · 2017-02-05T02:00:21Z

I deleted the failing test, since it didn't seem to add much after removing the custom derive parts (other tests test custom attributes). I also added a new test using a #[proc_macro_derive(Foo, attributes(Bar))], which should cover as much as the old test and proc macro derive attributes.

jseyfried · 2017-02-05T05:21:53Z

@keeperofdakeys Great, thanks!
@bors r+

bors · 2017-02-05T05:21:54Z

📌 Commit a201348 has been approved by jseyfried

@jseyfried

…eyfried Expand derive macros in the MacroExpander This removes the expand_derives function, and sprinkles the functionality throughout the Invocation Collector, Expander and Resolver. Fixes rust-lang#39326 r? @jseyfried

Rollup of 19 pull requests - Successful merges: #38518, #38921, #38959, #38983, #39009, #39107, #39193, #39289, #39312, #39393, #39442, #39443, #39453, #39454, #39471, #39477, #39478, #39527, #39552 - Failed merges:

@jseyfried

[beta] Give a better error message for unknown derive messages This PR improves the error message for unknown derive macros. Currently unknown derives give the following error, which is very misleading: ``` `#[derive]` for custom traits is not stable enough for use. It is deprecated and will be removed in v1.15 (see issue #29644) ``` I'm currently working on a PR that will change this (#39442) to the following: ``` cannot find derive macro `Foo` in this scope ``` This PR backports the (as yet unmerged) error message to beta/1.16 (it's a pity that this is probably too late for 1.15). r? @jseyfried

rust-highfive assigned nrc Feb 1, 2017

rust-highfive assigned jseyfried and unassigned nrc Feb 1, 2017

keeperofdakeys mentioned this pull request Feb 1, 2017

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

Closed

jseyfried reviewed Feb 1, 2017

View reviewed changes

keeperofdakeys force-pushed the expand-derives branch from 79db0dc to a3d0d48 Compare February 1, 2017 11:32

jseyfried reviewed Feb 1, 2017

View reviewed changes

keeperofdakeys mentioned this pull request Feb 1, 2017

[beta] Give a better error message for unknown derive messages #39444

Merged

keeperofdakeys force-pushed the expand-derives branch from b94a0c3 to 2c2505e Compare February 1, 2017 21:30

keeperofdakeys force-pushed the expand-derives branch from 2c2505e to b117bee Compare February 1, 2017 23:34

keeperofdakeys changed the title ~~[WIP] Expand derive macros in the MacroExpander~~ Expand derive macros in the MacroExpander Feb 2, 2017

frewsxcv mentioned this pull request Feb 4, 2017

Rollup of 11 pull requests #39536

Closed

frewsxcv mentioned this pull request Feb 4, 2017

Rollup of 10 pull requests #39537

Closed

keeperofdakeys added 3 commits February 5, 2017 09:31

Make builtin derives a SyntaxExtension

0477daf

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

Rename CustomDerive to ProcMacroDerive for macros 1.1

0a7380d

Move derive macro expansion into the MacroExpander

fbdd038

This removes the expand_derives function, and sprinkles the functionality throughout the Invocation Collector, Expander and Resolver.

Update pretty test for derive attributes

a201348

Remove attr-variant-data.rs since it relies on quirks in legacy custom derive resolution (undefined derives only print a warning). Add a new test which uses a defined proc macro derive, and tests pretty printing of proc macro derive attributes.

keeperofdakeys force-pushed the expand-derives branch from b117bee to a201348 Compare February 5, 2017 01:52

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 20 pull requests #39562

Closed

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 19 pull requests #39563

Merged

bors merged commit a201348 into rust-lang:master Feb 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand derive macros in the MacroExpander #39442

Expand derive macros in the MacroExpander #39442

keeperofdakeys commented Feb 1, 2017 •

edited

Loading

keeperofdakeys commented Feb 1, 2017

jseyfried Feb 1, 2017

jseyfried Feb 1, 2017

jseyfried Feb 1, 2017

keeperofdakeys commented Feb 1, 2017

jseyfried commented Feb 1, 2017

jseyfried commented Feb 2, 2017

bors commented Feb 2, 2017

frewsxcv commented Feb 4, 2017

keeperofdakeys commented Feb 5, 2017 •

edited

Loading

keeperofdakeys commented Feb 5, 2017 •

edited

Loading

keeperofdakeys commented Feb 5, 2017

jseyfried commented Feb 5, 2017

bors commented Feb 5, 2017

Expand derive macros in the MacroExpander #39442

Expand derive macros in the MacroExpander #39442

Conversation

keeperofdakeys commented Feb 1, 2017 • edited Loading

keeperofdakeys commented Feb 1, 2017

jseyfried Feb 1, 2017

Choose a reason for hiding this comment

jseyfried Feb 1, 2017

Choose a reason for hiding this comment

jseyfried Feb 1, 2017

Choose a reason for hiding this comment

keeperofdakeys commented Feb 1, 2017

jseyfried commented Feb 1, 2017

jseyfried commented Feb 2, 2017

bors commented Feb 2, 2017

frewsxcv commented Feb 4, 2017

keeperofdakeys commented Feb 5, 2017 • edited Loading

keeperofdakeys commented Feb 5, 2017 • edited Loading

keeperofdakeys commented Feb 5, 2017

jseyfried commented Feb 5, 2017

bors commented Feb 5, 2017

keeperofdakeys commented Feb 1, 2017 •

edited

Loading

keeperofdakeys commented Feb 5, 2017 •

edited

Loading

keeperofdakeys commented Feb 5, 2017 •

edited

Loading