Refactor macro resolution errors + add derive macro suggestions #39752

keeperofdakeys · 2017-02-12T00:16:52Z

Move legacy macro resolution error reporting to finalize_current_module_macro_resolutions, and provide suggestions for derive macros.

Fixes #39323

cc #30197

r? @jseyfried

keeperofdakeys · 2017-02-12T00:23:05Z

All tests pass expect a few compile-fail tests. For some reason some macro invocations are no longer giving an error, I suspect this is a bug in my code (eg: src/test/compile-fail/macro-error.rs). Also type errors are also occuring in some tests, which leads to interesting bugs with dummy expansions (eg. src/test/compile-fail/macro-expansion-tests.rs).

jseyfried · 2017-02-12T01:49:16Z

src/librustc_resolve/macros.rs

@@ -260,21 +255,7 @@ impl<'a> base::Resolver for Resolver<'a> {

    fn resolve_derive_macro(&mut self, scope: Mark, path: &ast::Path, force: bool)
                            -> Result<Rc<SyntaxExtension>, Determinacy> {


nit: I'd refactor away this function now that it is a one-liner.

jseyfried · 2017-02-12T01:52:10Z

src/librustc_resolve/macros.rs

+                find_best_match_for_name(self.macro_names.iter(), name, None),
+            MacroInvocKind::Attr |
+            MacroInvocKind::Derive => {
+                // FIXME: get_macro needs an &mut Resolver, can we do it without cloning?


We could probably mem::replace the builtins -- I think the clone on an error path is fine though.

jseyfried · 2017-02-12T01:57:23Z

src/libsyntax/ext/base.rs

@@ -474,6 +474,35 @@ impl MacResult for DummyResult {
 pub type BuiltinDeriveFn =
    for<'cx> fn(&'cx mut ExtCtxt, Span, &MetaItem, &Annotatable, &mut FnMut(Annotatable));

+/// Represents different kinds of macro invocations that can be resolved.
+#[derive(Clone, Copy)]
+pub enum MacroInvocKind {


nit: I'd name this MacroKind to be more general -- for example, it also describes macro definitions. Also, the current name is pretty close to expand::InvocationKind.

jseyfried · 2017-02-12T01:59:52Z

src/libsyntax/ext/base.rs

+
+impl MacroInvocKind {
+    /// Returns true if the syntax extension is allowed for this macro invocation.
+    pub fn allow_ext(&self, _ext: &Rc<SyntaxExtension>) -> bool {


I'd instead add a method fn kind(&self) -> MacroKind { ... } to SyntaxExtension.

I didn't think that there was a 1-1 mapping, but I can see that there seems to be.

jseyfried · 2017-02-12T02:05:11Z

src/test/compile-fail/deriving-meta-unknown-trait.rs

@@ -11,7 +11,7 @@
 // ignore-tidy-linelength

 #[derive(Eqr)]
-//~^ ERROR cannot find derive macro `Eqr` in this scope
+//~^ ERROR cannot find macro `Eqr` in this scope


Not sure that this is an improvement -- I think we should use "derive macro" (status quo) or "custom derive" here.
cc @nrc

I actually agree with this, and I was meaning to change it back. Should attribute macros show something different to "macro"?

I think attribute macros should say "cannot find attribute macro `...` in this scope".

jseyfried · 2017-02-12T02:24:06Z

src/librustc_resolve/macros.rs

            if suggestion != name {
-                err.help(&format!("did you mean `{}!`?", suggestion));
+                if let MacroInvocKind::Bang = kind {
+                  err.help(&format!("did you mean `{}!`?", suggestion));


nit: indent

jseyfried · 2017-02-12T02:33:28Z

src/test/compile-fail/macros-nonfatal-errors.rs

@@ -14,14 +14,14 @@
 #![feature(asm)]
 #![feature(trace_macros, concat_idents)]

-#[derive(Zero)] //~ ERROR
+#[derive(Zero)]
 struct CantDeriveThis;


I'd remove this -- confusing to have erroneous code that doesn't emit errors since we abort early.

I was actually a bit confused about why we don't see a resolve error here.

I believe it's because we abort here due to other errors during expansion, so we never get to finalize_current_module_macro_resolutions.

jseyfried · 2017-02-12T02:33:46Z

src/test/compile-fail/self_type_keyword.rs

@@ -15,19 +15,24 @@ struct Self;

 struct Bar<'Self>;
 //~^ ERROR lifetimes cannot use keyword names
+//~^^ ERROR parameter `'Self` is never used


Could we move the struct Self; in this test into a sub-module to avoid these extra errors?

jseyfried · 2017-02-12T02:57:18Z

@keeperofdakeys It looks like all test failures are due

aborting before we get a chance to emit the unresolved macro errors that the test expects, or
not aborting after emitting unresolved macro errors, leading to type errors later on.

I think the best solution would be to make sure we abort before ast->hir lowering if there were unresolved macro errors, at least until we figure out how to suppress the spurious type errors.

keeperofdakeys · 2017-02-12T03:00:28Z

That sounds good, but I don't know what to change to do that. Where is the code that starts the lowering?

jseyfried · 2017-02-12T03:12:11Z

I would add a pub flag to Resolver that we set if there is an unresolved invocation and we need to abort before type checking. Then, we could check the flag and abort just after name resolution, see here for an example.

Btw, import resolution happens during expansion today, so this comment is outdated.

keeperofdakeys · 2017-02-12T08:36:05Z

Are attribute macros stored in builtin_macros, or somewhere else? The attribute macro suggestions aren't working.

jseyfried · 2017-02-12T08:39:52Z

@keeperofdakeys
Depends on the attribute macro. Attribute macros from #![plugin(..)] are in builtin_macros, use-imported attribute macros are stored in the field resolutions of the containing module's resolver::ModuleData.

Normally, builtin_macros would also have #[macro_use]-imported macros, but this is forbidden for attribute proc macros (we tell people to use use instead).

jseyfried · 2017-02-12T09:02:50Z

Also, we can import proc macro derives today via use (behind #![feature(proc_macro)]), I believe these aren't included in the suggestions.

I think the best path forward here (in this PR or later) is to check resolve::lookup_typo_candidate first when resolver.use_extern_macros is true (includes #![feature(proc_macro)]). You shouldn't have to change the filter function's signature; you can do Def -(retain Def::Macro)-> DefId -> SyntaxExtension -> MacroKind -(retain expected)-> ().

bors · 2017-02-13T01:57:47Z

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

keeperofdakeys · 2017-02-13T10:10:52Z

That sounds like a good idea, however I'm unsure how to go from a DefId -> SyntaxExtension.

keeperofdakeys · 2017-02-13T12:53:26Z

Mmm, for some reason the lookup_typo_candidate call is causing a panic here.

keeperofdakeys · 2017-02-14T14:04:54Z

Oh I see now, lookup_typo_candidate uses self.ribs, which hasn't got MacroNS initialised. Would this be easy to fix? Or would it be easier to leave suggestions for extern macros for another PR?

jseyfried · 2017-02-15T00:02:31Z

@keeperofdakeys

DefId -> SyntaxExtension

This should do the trick.

lookup_typo_candidate uses self.ribs, which hasn't got MacroNS initialised. Would this be easy to fix? Or would it be easier to leave suggestions for extern macros for another PR?

Right, I don't think this is too hard to fix -- wherever we add ModuleRibKind to value and type ribs today (should always happen in pairs and always in resolve/lib.rs), we'd also want to add to the macro ribs.

This should actually be a simplification, since we could use e.g. self.per_ns(|this, ns| this.ribs[ns].push(...)); instead of duplicating for ValueNS and TypeNS manually.

keeperofdakeys · 2017-02-15T14:11:09Z

It seems like it will be a bit of work to support use macro suggestions, so I think it's best if that's dealt with in another PR.

jseyfried

Nice! r=me modulo comments

jseyfried · 2017-02-16T02:33:40Z

src/librustc_driver/driver.rs

-        // Since import resolution will eventually happen in expansion,
-        // don't perform `after_expand` until after import resolution.
+        // Since import resolution happs in expansion, don't perform
+        // `after_expand` until after import resolution.


I would just remove this comment -- since expansion and import resolution are interleaved, there's no way to perform after_expand before import resolution, so we don't need to warn people about this.

Also, we can move after_expand(&krate)? outside of time(time_passes, "name resolution", ...) now, I think it would be best to put it just before expansion debugging diagnostics ("Post-expansion node count", etc.).

jseyfried · 2017-02-16T02:36:52Z

src/libsyntax/ext/base.rs

@@ -474,6 +474,17 @@ impl MacResult for DummyResult {
 pub type BuiltinDeriveFn =
    for<'cx> fn(&'cx mut ExtCtxt, Span, &MetaItem, &Annotatable, &mut FnMut(Annotatable));

+/// Represents different kinds of macro invocations that can be resolved.
+#[derive(Clone, Copy, PartialEq)]


nit: I'd add Eq just to be idiomatic.

jseyfried · 2017-02-16T02:40:27Z

src/test/compile-fail/self_type_keyword.rs

@@ -20,14 +20,18 @@ pub fn main() {
    match 15 {
        ref Self => (),
        //~^ ERROR expected identifier, found keyword `Self`
+        //~^^ ERROR match bindings cannot shadow unit structs


(optional) Could we avoid these extra errors by moving `struct (from earlier in the file) into a sub-module?

jseyfried · 2017-02-16T02:43:51Z

src/test/compile-fail/macros-nonfatal-errors.rs

 #[derive(Default)] //~ ERROR
 enum OrDeriveThis {}

 fn main() {
-    doesnt_exist!(); //~ ERROR
+    doesnt_exist!();


I'd remove this -- it's only not an error here since we abort early.

jseyfried · 2017-02-16T02:45:32Z

@bors delegate=keeperofdakeys

bors · 2017-02-16T02:45:33Z

✌️ @keeperofdakeys can now approve this pull request

keeperofdakeys · 2017-02-16T13:06:18Z

@bors r+

bors · 2017-02-16T13:06:19Z

📌 Commit 2d91e7a has been approved by keeperofdakeys

bors · 2017-02-16T13:46:01Z

⌛ Testing commit 2d91e7a with merge 82aed0b...

@jseyfried

Refactor macro resolution errors + add derive macro suggestions Move legacy macro resolution error reporting to `finalize_current_module_macro_resolutions`, and provide suggestions for derive macros. Fixes #39323 cc #30197 r? @jseyfried

bors · 2017-02-16T16:47:03Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-16T17:37:31Z

@bors: retry * appveyor timeout

…

On Thu, Feb 16, 2017 at 10:47 AM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1980> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#39752 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95CTYupbGjZxeqlDAVuPhuO7BqsXcks5rdH2JgaJpZM4L-WDv> .

bors · 2017-02-17T04:02:23Z

⌛ Testing commit 2d91e7a with merge 16c94cd...

@jseyfried

Refactor macro resolution errors + add derive macro suggestions Move legacy macro resolution error reporting to `finalize_current_module_macro_resolutions`, and provide suggestions for derive macros. Fixes #39323 cc #30197 r? @jseyfried

bors · 2017-02-17T06:45:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: keeperofdakeys
Pushing 16c94cd to master...

rust-highfive assigned jseyfried Feb 12, 2017

jseyfried reviewed Feb 12, 2017

View reviewed changes

keeperofdakeys force-pushed the macro-error branch from a1898f4 to 479c781 Compare February 13, 2017 12:49

keeperofdakeys force-pushed the macro-error branch 2 times, most recently from a8f30a6 to 71679b5 Compare February 15, 2017 14:10

keeperofdakeys changed the title ~~[WIP] Refactor macro resolution errors + add suggestions~~ Refactor macro resolution errors + add suggestions Feb 15, 2017

keeperofdakeys changed the title ~~Refactor macro resolution errors + add suggestions~~ Refactor macro resolution errors + add derive macro suggestions Feb 15, 2017

jseyfried reviewed Feb 16, 2017

View reviewed changes

Refactor macro resolution errors + add derive macro suggestions

2d91e7a

keeperofdakeys force-pushed the macro-error branch from 71679b5 to 2d91e7a Compare February 16, 2017 11:35

bors merged commit 2d91e7a into rust-lang:master Feb 17, 2017

bors mentioned this pull request Feb 17, 2017

Simplify TokenTree and fix macro_rules! bugs #39419

Merged

petrochenkov mentioned this pull request Feb 17, 2017

Add levenshtein distance based suggestions everywhere #30197

Open

keeperofdakeys mentioned this pull request Feb 18, 2017

Internal compiler error with serde::de::Error::custom #39924

Closed

		@@ -260,21 +255,7 @@ impl<'a> base::Resolver for Resolver<'a> {

		fn resolve_derive_macro(&mut self, scope: Mark, path: &ast::Path, force: bool)
		-> Result<Rc<SyntaxExtension>, Determinacy> {

Refactor macro resolution errors + add derive macro suggestions #39752

Refactor macro resolution errors + add derive macro suggestions #39752

Conversation

keeperofdakeys commented Feb 12, 2017 • edited Loading

keeperofdakeys commented Feb 12, 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

Choose a reason for hiding this comment

keeperofdakeys Feb 12, 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 commented Feb 12, 2017

keeperofdakeys commented Feb 12, 2017

jseyfried commented Feb 12, 2017

keeperofdakeys commented Feb 12, 2017

jseyfried commented Feb 12, 2017 • edited Loading

jseyfried commented Feb 12, 2017 • edited Loading

bors commented Feb 13, 2017

keeperofdakeys commented Feb 13, 2017

keeperofdakeys commented Feb 13, 2017

keeperofdakeys commented Feb 14, 2017

jseyfried commented Feb 15, 2017

keeperofdakeys commented Feb 15, 2017

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

jseyfried commented Feb 16, 2017

bors commented Feb 16, 2017

keeperofdakeys commented Feb 16, 2017

bors commented Feb 16, 2017

bors commented Feb 16, 2017

bors commented Feb 16, 2017

alexcrichton commented Feb 16, 2017 via email

bors commented Feb 17, 2017

bors commented Feb 17, 2017

keeperofdakeys commented Feb 12, 2017 •

edited

Loading

keeperofdakeys Feb 12, 2017 •

edited

Loading

jseyfried commented Feb 12, 2017 •

edited

Loading

jseyfried commented Feb 12, 2017 •

edited

Loading