Provide suggestions for unknown macros imported with `use` #39953

keeperofdakeys · 2017-02-19T09:39:54Z

petrochenkov · 2017-02-19T10:07:36Z

It looks like MacroKind is better moved into Def, this way you won't have to pass the resolver around.
Def is expected to be self-sufficient when deciding if some resolution is expected in some context or not.
I hope all this special code for macros will be replaced by using the common smart_resolve_path infrastructure later.

petrochenkov · 2017-02-19T10:12:16Z

Def::Macro(DefId) -> Def::Macro(DefId, MacroKind) would be enough for a start, but in principle by-example macros and attribute macros are as different as, e.g., structs and traits, and expected in different contexts, so they could even have separate Def variants (this could be less convenient in practice though).

jseyfried · 2017-02-19T23:36:04Z

It looks like MacroKind is better moved into Def
Def::Macro(DefId) -> Def::Macro(DefId, MacroKind)

Agreed, this looks like the cleanest solution -- thanks @petrochenkov!

@keeperofdakeys would you mind doing this instead of passing around the resolver more?

keeperofdakeys · 2017-02-19T23:46:06Z

Sure, sounds like a good idea.

keeperofdakeys · 2017-02-20T10:27:56Z

@jseyfried
Two questions:

Is a syntax::ast::MacroDef always a macro_rules!? (If not I'll need to store a MacroKind in it).
Does rustc_metadata::cstore::CrateMetadata.proc_macros only ever contain derive proc macros? (Otherwise it will need to be a (Name, MacroKind, SyntaxExtension)).

jseyfried · 2017-02-20T10:31:52Z

@keeperofdakeys

Is a syntax::ast::MacroDef always a macro_rules!?

Yes.

Does rustc_metadata::cstore::CrateMetadata.proc_macros only ever contain derive proc macros?

No, it can contain attribute proc macros today and bang proc macros once they are implemented.

(Otherwise it will need to be a (Name, MacroKind, SyntaxExtension))

Can't you get the MacroKind from the SyntaxExtension, so (Name, SyntaxExtension) is still sufficient?

keeperofdakeys · 2017-02-20T14:00:50Z

I added some corner cases to the test cases, which I'm guessing were failing before all the changes in this PR. To fix it, I need to filter macro_names for only MacroKind::Bang. Can I use builtin_macros to check the syntax extension of these?

Also, are the changes to get_def correct?

keeperofdakeys · 2017-02-20T22:22:55Z

So for some reason derive proc-macros are appearing in macro_names, which from what I know shouldn't be happening (attribute macros, and builtin derive macros do not). This is what is causing the test failure. I'll need to investigate whether this was caused by my changes here, or if this has always been the case.

jseyfried · 2017-02-20T23:04:44Z

src/librustc_metadata/decoder.rs

        }
+        unreachable!();


nit: I think this would be clearer as an if !self.is_proc_macro(index) { ... } else { ... } to avoid early returns.

Also, I would do let kind = self.proc_macros.as_ref().unwrap()[index.as_usize() - 1].1.kind(); in the else case to avoid unreachable!().

jseyfried · 2017-02-20T23:06:44Z

src/librustc_resolve/macros.rs

@@ -151,7 +151,7 @@ impl<'a> base::Resolver for Resolver<'a> {
        invocation.expansion.set(visitor.legacy_scope);
    }

-    fn add_ext(&mut self, ident: ast::Ident, ext: Rc<SyntaxExtension>) {
+    fn add_ext(&mut self, ident: ast::Ident, kind: MacroKind, ext: Rc<SyntaxExtension>) {


Since we have the SyntaxExtension, I don't think we need to add the MacroKind argument.

jseyfried · 2017-02-20T23:07:51Z

src/librustc_resolve/macros.rs

-            MacroKind::Attr |
-            MacroKind::Derive => {
+            kind @ MacroKind::Attr |
+            kind @ MacroKind::Derive => {


nit: no need to add kind @ here -- kind is already in scope.

jseyfried · 2017-02-20T23:38:24Z

I need to filter macro_names for only MacroKind::Bang

macro_names was used for the legacy suggestion system, so I think we should put as few names as possible in it.
Specifically, I think it should only have crate-local macro_rules! macros -- all other macros will be in builtin_macros (built-ins, #![plugin(..)], #[macro_use]) or individual Modules (use).

It looks like we add to macro_names here, here, and here -- looks like we can remove the second two and just keep the first one.

keeperofdakeys · 2017-02-21T22:04:47Z

@jseyfried Is the second place you told me to remove the insertion into macro_names correct? I suspect this is causing the current test failure. Do you think this is the cause? https://github.com/keeperofdakeys/rust/blob/928c6ee2e00cfddff8786209154ce986659218e1/src/librustc_resolve/lib.rs#L2329. Perhaps adding builting macros here as well might work? Or using resolve_macro(name, kind: MacroKind::Bang, false)?

keeperofdakeys · 2017-02-22T14:21:50Z

Ah, I understand now. It was usually imported by the legacy_import function. For now I've added builtin_macros to the macro name check.

jseyfried · 2017-02-22T21:10:18Z

For now I've added builtin_macros to the macro name check

Great, that's what I was going to suggest :)

keeperofdakeys · 2017-02-22T21:13:29Z

I think all the issues have been fixed now, so feel free to merge if you're happy with it.

jseyfried

Excellent! All nits are optional.

jseyfried · 2017-02-22T21:15:49Z

src/librustc_metadata/decoder.rs

+            self.entry(index).kind.to_def(self.local_def_id(index))
+        } else {
+            let kind = self.proc_macros.as_ref()
+                .unwrap()[index.as_usize() - 1].1.kind();


formatting nit: this line break here looks weird to me -- I'd put it all on one line or break after the ].

jseyfried · 2017-02-22T22:10:09Z

src/librustc_resolve/lib.rs

+        if let Some(binding) = binding {
+            contains = contains || binding.get_macro(self).kind() == MacroKind::Bang;
+        }
+        if primary_ns != MacroNS && path.len() == 1 && contains {


nit: I would write this as

let is_builtin = self.builtin_macros.get(&path[0].name).cloned() .map(|binding| binding.get_macro(self).kind() == MacroKind::Bang).unwrap_or(false); if primary_ns != MacroNS && (is_builtin || self.macro_names.contains(&path[0].name)) { ... }

jseyfried · 2017-02-22T22:14:54Z

src/librustc_resolve/macros.rs

+            find_best_match_for_name(self.macro_names.iter(), name, None)
+        } else {
+            None
+        // Then check the legacy namespace.


I would say "built-in macros' legacy namespace" or just "built-in macros" for clarity (I've referred to the scope of local macro_rules! as "legacy" in item names and in comments).

jseyfried · 2017-02-22T22:16:46Z

src/librustc_resolve/macros.rs

+            };
+            let ident = Ident::with_empty_ctxt(Symbol::intern(name));
+            self.lookup_typo_candidate(&vec![ident], MacroNS, is_macro)
+                .map(|s| Symbol::intern(&s))


nit: .as_ref().map(Symbol::intern) might work here

jseyfried · 2017-02-22T22:22:44Z

src/librustc_resolve/macros.rs

+                    false
+                }
+            };
+            let ident = Ident::with_empty_ctxt(Symbol::intern(name));


nit: Ident::from_str should do the trick here.

jseyfried · 2017-02-22T22:29:20Z

@bors delegate=keeperofdakeys

bors · 2017-02-22T22:29:21Z

✌️ @keeperofdakeys can now approve this pull request

This commit searchs modules for macro suggestions. It also removes imported macro_rules from macro_names, and adds more corner case checks for which macros should be suggested in specific contexts.

keeperofdakeys · 2017-02-23T11:28:49Z

@bors r=jseyfried

bors · 2017-02-23T11:28:50Z

📌 Commit da6dc53 has been approved by jseyfried

@jseyfried

…ried Provide suggestions for unknown macros imported with `use` cc rust-lang#30197 r? @jseyfried

@jseyfried

…ried Provide suggestions for unknown macros imported with `use` cc rust-lang#30197 r? @jseyfried

Rollup of 28 pull requests - Successful merges: #39859, #39864, #39888, #39903, #39905, #39914, #39945, #39950, #39953, #39961, #39980, #39988, #39993, #39995, #40019, #40020, #40022, #40024, #40025, #40026, #40027, #40031, #40035, #40037, #40038, #40064, #40069, #40086 - Failed merges: #39927, #40008, #40047

rust-highfive assigned jseyfried Feb 19, 2017

jseyfried reviewed Feb 20, 2017

View reviewed changes

jseyfried reviewed Feb 22, 2017

View reviewed changes

keeperofdakeys added 2 commits February 23, 2017 20:12

Move MacroKind into Def::Macro

4ecdc68

Add macro suggestions for macros imported with use

da6dc53

This commit searchs modules for macro suggestions. It also removes imported macro_rules from macro_names, and adds more corner case checks for which macros should be suggested in specific contexts.

keeperofdakeys force-pushed the macro-error branch from 3c4e1ed to da6dc53 Compare February 23, 2017 09:58

eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017

Rollup merge of rust-lang#39953 - keeperofdakeys:macro-error, r=jseyf…

fb3836a

…ried Provide suggestions for unknown macros imported with `use` cc rust-lang#30197 r? @jseyfried

eddyb mentioned this pull request Feb 25, 2017

Rollup of 29 pull requests #40089

Closed

eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017

Rollup merge of rust-lang#39953 - keeperofdakeys:macro-error, r=jseyf…

a6a5c32

…ried Provide suggestions for unknown macros imported with `use` cc rust-lang#30197 r? @jseyfried

eddyb mentioned this pull request Feb 25, 2017

Rollup of 28 pull requests #40091

Merged

bors merged commit da6dc53 into rust-lang:master Feb 25, 2017

topecongiro mentioned this pull request Dec 28, 2017

Add tests on fixed ICEs #47044

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide suggestions for unknown macros imported with `use` #39953

Provide suggestions for unknown macros imported with `use` #39953

keeperofdakeys commented Feb 19, 2017

petrochenkov commented Feb 19, 2017

petrochenkov commented Feb 19, 2017

jseyfried commented Feb 19, 2017

keeperofdakeys commented Feb 19, 2017

keeperofdakeys commented Feb 20, 2017

jseyfried commented Feb 20, 2017 •

edited

Loading

keeperofdakeys commented Feb 20, 2017 •

edited

Loading

keeperofdakeys commented Feb 20, 2017

jseyfried Feb 20, 2017

jseyfried Feb 20, 2017

jseyfried Feb 20, 2017

jseyfried commented Feb 20, 2017

keeperofdakeys commented Feb 21, 2017

keeperofdakeys commented Feb 22, 2017

jseyfried commented Feb 22, 2017

keeperofdakeys commented Feb 22, 2017

jseyfried left a comment

jseyfried Feb 22, 2017

jseyfried Feb 22, 2017

jseyfried Feb 22, 2017

jseyfried Feb 22, 2017

jseyfried Feb 22, 2017

jseyfried commented Feb 22, 2017

bors commented Feb 22, 2017

keeperofdakeys commented Feb 23, 2017

bors commented Feb 23, 2017

Provide suggestions for unknown macros imported with use #39953

Provide suggestions for unknown macros imported with use #39953

Conversation

keeperofdakeys commented Feb 19, 2017

petrochenkov commented Feb 19, 2017

petrochenkov commented Feb 19, 2017

jseyfried commented Feb 19, 2017

keeperofdakeys commented Feb 19, 2017

keeperofdakeys commented Feb 20, 2017

jseyfried commented Feb 20, 2017 • edited Loading

keeperofdakeys commented Feb 20, 2017 • edited Loading

keeperofdakeys commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried commented Feb 20, 2017

keeperofdakeys commented Feb 21, 2017

keeperofdakeys commented Feb 22, 2017

jseyfried commented Feb 22, 2017

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

Choose a reason for hiding this comment

jseyfried commented Feb 22, 2017

bors commented Feb 22, 2017

keeperofdakeys commented Feb 23, 2017

bors commented Feb 23, 2017

Provide suggestions for unknown macros imported with `use` #39953

Provide suggestions for unknown macros imported with `use` #39953

jseyfried commented Feb 20, 2017 •

edited

Loading

keeperofdakeys commented Feb 20, 2017 •

edited

Loading