Auto merge of #39752 - keeperofdakeys:macro-error, r=keeperofdakeys

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

Author: bors

src/librustc_driver/driver.rs

@@ -726,6 +726,8 @@ pub fn phase_2_configure_and_expand<F>(sess: &Session,
         });
     }
 
+    after_expand(&krate)?;
+
     if sess.opts.debugging_opts.input_stats {
         println!("Post-expansion node count: {}", count_nodes(&krate));
     }
@@ -751,14 +753,14 @@ pub fn phase_2_configure_and_expand<F>(sess: &Session,
          || ast_validation::check_crate(sess, &krate));
 
     time(time_passes, "name resolution", || -> CompileResult {
-        // Since import resolution will eventually happen in expansion,
-        // don't perform `after_expand` until after import resolution.
-        after_expand(&krate)?;
-
         resolver.resolve_crate(&krate);
         Ok(())
     })?;
 
+    if resolver.found_unresolved_macro {
+        sess.parse_sess.span_diagnostic.abort_if_errors();
+    }
+
     // Needs to go *after* expansion to be able to check the results of macro expansion.
     time(time_passes, "complete gated feature checking", || {
         sess.track_errors(|| {

src/librustc_resolve/lib.rs

@@ -51,6 +51,7 @@ use syntax::ext::hygiene::{Mark, SyntaxContext};
 use syntax::ast::{self, Name, NodeId, Ident, SpannedIdent, FloatTy, IntTy, UintTy};
 use syntax::ext::base::SyntaxExtension;
 use syntax::ext::base::Determinacy::{Determined, Undetermined};
+use syntax::ext::base::MacroKind;
 use syntax::symbol::{Symbol, keywords};
 use syntax::util::lev_distance::find_best_match_for_name;
 
@@ -785,7 +786,7 @@ pub struct ModuleData<'a> {
     normal_ancestor_id: DefId,
 
     resolutions: RefCell<FxHashMap<(Ident, Namespace), &'a RefCell<NameResolution<'a>>>>,
-    legacy_macro_resolutions: RefCell<Vec<(Mark, Ident, Span)>>,
+    legacy_macro_resolutions: RefCell<Vec<(Mark, Ident, Span, MacroKind)>>,
     macro_resolutions: RefCell<Vec<(Box<[Ident]>, Span)>>,
 
     // Macro invocations that can expand into items in this module.
@@ -1117,6 +1118,7 @@ pub struct Resolver<'a> {
     macro_map: FxHashMap<DefId, Rc<SyntaxExtension>>,
     macro_exports: Vec<Export>,
     pub whitelisted_legacy_custom_derives: Vec<Name>,
+    pub found_unresolved_macro: bool,
 
     // Maps the `Mark` of an expansion to its containing module or block.
     invocations: FxHashMap<Mark, &'a InvocationData<'a>>,
@@ -1315,6 +1317,7 @@ impl<'a> Resolver<'a> {
             warned_proc_macros: FxHashSet(),
             potentially_unused_imports: Vec::new(),
             struct_constructors: DefIdMap(),
+            found_unresolved_macro: false,
         }
     }
 

src/librustc_resolve/macros.rs

@@ -24,6 +24,7 @@ use syntax::attr;
 use syntax::errors::DiagnosticBuilder;
 use syntax::ext::base::{self, Determinacy, MultiModifier, MultiDecorator};
 use syntax::ext::base::{NormalTT, Resolver as SyntaxResolver, SyntaxExtension};
+use syntax::ext::base::MacroKind;
 use syntax::ext::expand::{Expansion, mark_tts};
 use syntax::ext::hygiene::Mark;
 use syntax::ext::tt::macro_rules;
@@ -236,8 +237,8 @@ impl<'a> base::Resolver for Resolver<'a> {
         None
     }
 
-    fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, force: bool)
-                     -> Result<Rc<SyntaxExtension>, Determinacy> {
+    fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind,
+                     force: bool) -> Result<Rc<SyntaxExtension>, Determinacy> {
         let ast::Path { ref segments, span } = *path;
         if segments.iter().any(|segment| segment.parameters.is_some()) {
             let kind =
@@ -256,6 +257,7 @@ impl<'a> base::Resolver for Resolver<'a> {
                 let msg = "non-ident macro paths are experimental";
                 let feature = "use_extern_macros";
                 emit_feature_err(&self.session.parse_sess, feature, span, GateIssue::Language, msg);
+                self.found_unresolved_macro = true;
                 return Err(Determinacy::Determined);
             }
 
@@ -266,7 +268,10 @@ impl<'a> base::Resolver for Resolver<'a> {
                 },
                 PathResult::Module(..) => unreachable!(),
                 PathResult::Indeterminate if !force => return Err(Determinacy::Undetermined),
-                _ => Err(Determinacy::Determined),
+                _ => {
+                    self.found_unresolved_macro = true;
+                    Err(Determinacy::Determined)
+                },
             };
             self.current_module.macro_resolutions.borrow_mut()
                 .push((path.into_boxed_slice(), span));
@@ -279,40 +284,19 @@ impl<'a> base::Resolver for Resolver<'a> {
             Some(MacroBinding::Modern(binding)) => Ok(binding.get_macro(self)),
             None => match self.resolve_lexical_macro_path_segment(path[0], MacroNS, None) {
                 Ok(binding) => Ok(binding.get_macro(self)),
-                Err(Determinacy::Undetermined) if !force => return Err(Determinacy::Undetermined),
-                _ => {
-                    let msg = format!("macro undefined: `{}`", name);
-                    let mut err = self.session.struct_span_err(span, &msg);
-                    self.suggest_macro_name(&name.as_str(), &mut err);
-                    err.emit();
-                    return Err(Determinacy::Determined);
-                },
+                Err(Determinacy::Undetermined) if !force =>
+                    return Err(Determinacy::Undetermined),
+                Err(_) => {
+                    self.found_unresolved_macro = true;
+                    Err(Determinacy::Determined)
+                }
             },
         };
 
-        if self.use_extern_macros {
-            self.current_module.legacy_macro_resolutions.borrow_mut().push((scope, path[0], span));
-        }
-        result
-    }
+        self.current_module.legacy_macro_resolutions.borrow_mut()
+            .push((scope, path[0], span, kind));
 
-    fn resolve_derive_macro(&mut self, scope: Mark, path: &ast::Path, force: bool)
-                            -> Result<Rc<SyntaxExtension>, Determinacy> {
-        let ast::Path { span, .. } = *path;
-        match self.resolve_macro(scope, path, false) {
-            Ok(ext) => match *ext {
-                SyntaxExtension::BuiltinDerive(..) |
-                SyntaxExtension::ProcMacroDerive(..) => Ok(ext),
-                _ => Err(Determinacy::Determined),
-            },
-            Err(Determinacy::Undetermined) if force => {
-                let msg = format!("cannot find derive macro `{}` in this scope", path);
-                let mut err = self.session.struct_span_err(span, &msg);
-                err.emit();
-                Err(Determinacy::Determined)
-            },
-            Err(err) => Err(err),
-        }
+        result
     }
 }
 
@@ -438,37 +422,74 @@ impl<'a> Resolver<'a> {
             }
         }
 
-        for &(mark, ident, span) in module.legacy_macro_resolutions.borrow().iter() {
+        for &(mark, ident, span, kind) in module.legacy_macro_resolutions.borrow().iter() {
             let legacy_scope = &self.invocations[&mark].legacy_scope;
             let legacy_resolution = self.resolve_legacy_scope(legacy_scope, ident.name, true);
             let resolution = self.resolve_lexical_macro_path_segment(ident, MacroNS, Some(span));
-            let (legacy_resolution, resolution) = match (legacy_resolution, resolution) {
-                (Some(legacy_resolution), Ok(resolution)) => (legacy_resolution, resolution),
+            match (legacy_resolution, resolution) {
+                (Some(legacy_resolution), Ok(resolution)) => {
+                    let (legacy_span, participle) = match legacy_resolution {
+                        MacroBinding::Modern(binding)
+                            if binding.def() == resolution.def() => continue,
+                        MacroBinding::Modern(binding) => (binding.span, "imported"),
+                        MacroBinding::Legacy(binding) => (binding.span, "defined"),
+                    };
+                    let msg1 = format!("`{}` could refer to the macro {} here", ident, participle);
+                    let msg2 = format!("`{}` could also refer to the macro imported here", ident);
+                    self.session.struct_span_err(span, &format!("`{}` is ambiguous", ident))
+                        .span_note(legacy_span, &msg1)
+                        .span_note(resolution.span, &msg2)
+                        .emit();
+                },
                 (Some(MacroBinding::Modern(binding)), Err(_)) => {
                     self.record_use(ident, MacroNS, binding, span);
                     self.err_if_macro_use_proc_macro(ident.name, span, binding);
-                    continue
                 },
-                _ => continue,
-            };
-            let (legacy_span, participle) = match legacy_resolution {
-                MacroBinding::Modern(binding) if binding.def() == resolution.def() => continue,
-                MacroBinding::Modern(binding) => (binding.span, "imported"),
-                MacroBinding::Legacy(binding) => (binding.span, "defined"),
+                (None, Err(_)) => {
+                    let msg = match kind {
+                        MacroKind::Bang =>
+                            format!("cannot find macro `{}!` in this scope", ident),
+                        MacroKind::Attr =>
+                            format!("cannot find attribute macro `{}` in this scope", ident),
+                        MacroKind::Derive =>
+                            format!("cannot find derive macro `{}` in this scope", ident),
+                    };
+                    let mut err = self.session.struct_span_err(span, &msg);
+                    self.suggest_macro_name(&ident.name.as_str(), kind, &mut err);
+                    err.emit();
+                },
+                _ => {},
             };
-            let msg1 = format!("`{}` could refer to the macro {} here", ident, participle);
-            let msg2 = format!("`{}` could also refer to the macro imported here", ident);
-            self.session.struct_span_err(span, &format!("`{}` is ambiguous", ident))
-                .span_note(legacy_span, &msg1)
-                .span_note(resolution.span, &msg2)
-                .emit();
         }
     }
 
-    fn suggest_macro_name(&mut self, name: &str, err: &mut DiagnosticBuilder<'a>) {
-        if let Some(suggestion) = find_best_match_for_name(self.macro_names.iter(), name, None) {
+    fn suggest_macro_name(&mut self, name: &str, kind: MacroKind,
+                          err: &mut DiagnosticBuilder<'a>) {
+        let suggestion = match kind {
+            MacroKind::Bang =>
+                find_best_match_for_name(self.macro_names.iter(), name, None),
+            MacroKind::Attr |
+            MacroKind::Derive => {
+                // Find a suggestion from the legacy namespace.
+                // FIXME: get_macro needs an &mut Resolver, can we do it without cloning?
+                let builtin_macros = self.builtin_macros.clone();
+                let names = builtin_macros.iter().filter_map(|(name, binding)| {
+                    if binding.get_macro(self).kind() == kind {
+                        Some(name)
+                    } else {
+                        None
+                    }
+                });
+                find_best_match_for_name(names, name, None)
+            }
+        };
+        if let Some(suggestion) = suggestion {
             if suggestion != name {
-                err.help(&format!("did you mean `{}!`?", suggestion));
+                if let MacroKind::Bang = kind {
+                    err.help(&format!("did you mean `{}!`?", suggestion));
+                } else {
+                    err.help(&format!("did you mean `{}`?", suggestion));
+                }
             } else {
                 err.help(&format!("have you added the `#[macro_use]` on the module/import?"));
             }

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, Eq)]
+pub enum MacroKind {
+    /// A bang macro - foo!()
+    Bang,
+    /// An attribute macro - #[foo]
+    Attr,
+    /// A derive attribute macro - #[derive(Foo)]
+    Derive,
+}
+
 /// An enum representing the different kinds of syntax extensions.
 pub enum SyntaxExtension {
     /// A syntax extension that is attached to an item and creates new items
@@ -520,6 +531,25 @@ pub enum SyntaxExtension {
     BuiltinDerive(BuiltinDeriveFn),
 }
 
+impl SyntaxExtension {
+    /// Return which kind of macro calls this syntax extension.
+    pub fn kind(&self) -> MacroKind {
+        match *self {
+            SyntaxExtension::NormalTT(..) |
+            SyntaxExtension::IdentTT(..) |
+            SyntaxExtension::ProcMacro(..) =>
+                MacroKind::Bang,
+            SyntaxExtension::MultiDecorator(..) |
+            SyntaxExtension::MultiModifier(..) |
+            SyntaxExtension::AttrProcMacro(..) =>
+                MacroKind::Attr,
+            SyntaxExtension::ProcMacroDerive(..) |
+            SyntaxExtension::BuiltinDerive(..) =>
+                MacroKind::Derive,
+        }
+    }
+}
+
 pub type NamedSyntaxExtension = (Name, SyntaxExtension);
 
 pub trait Resolver {
@@ -535,10 +565,8 @@ pub trait Resolver {
     fn resolve_imports(&mut self);
     // Resolves attribute and derive legacy macros from `#![plugin(..)]`.
     fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>) -> Option<Attribute>;
-    fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, force: bool)
-                     -> Result<Rc<SyntaxExtension>, Determinacy>;
-    fn resolve_derive_macro(&mut self, scope: Mark, path: &ast::Path, force: bool)
-                            -> Result<Rc<SyntaxExtension>, Determinacy>;
+    fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind,
+                     force: bool) -> Result<Rc<SyntaxExtension>, Determinacy>;
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -561,12 +589,8 @@ impl Resolver for DummyResolver {
 
     fn resolve_imports(&mut self) {}
     fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>) -> Option<Attribute> { None }
-    fn resolve_macro(&mut self, _scope: Mark, _path: &ast::Path, _force: bool)
-                     -> Result<Rc<SyntaxExtension>, Determinacy> {
-        Err(Determinacy::Determined)
-    }
-    fn resolve_derive_macro(&mut self, _scope: Mark, _path: &ast::Path, _force: bool)
-                            -> Result<Rc<SyntaxExtension>, Determinacy> {
+    fn resolve_macro(&mut self, _scope: Mark, _path: &ast::Path, _kind: MacroKind,
+                     _force: bool) -> Result<Rc<SyntaxExtension>, Determinacy> {
         Err(Determinacy::Determined)
     }
 }

src/libsyntax/ext/expand.rs

@@ -282,8 +282,8 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                         let mark = Mark::fresh();
                         derives.push(mark);
                         let path = ast::Path::from_ident(span, Ident::with_empty_ctxt(name));
-                        let item = match self.cx.resolver
-                                             .resolve_macro(Mark::root(), &path, false) {
+                        let item = match self.cx.resolver.resolve_macro(
+                                Mark::root(), &path, MacroKind::Derive, false) {
                             Ok(ext) => match *ext {
                                 SyntaxExtension::BuiltinDerive(..) => item_with_markers.clone(),
                                 _ => item.clone(),
@@ -369,12 +369,14 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                      -> Result<Option<Rc<SyntaxExtension>>, Determinacy> {
         let (attr, traits, item) = match invoc.kind {
             InvocationKind::Bang { ref mac, .. } => {
-                return self.cx.resolver.resolve_macro(scope, &mac.node.path, force).map(Some);
+                return self.cx.resolver.resolve_macro(scope, &mac.node.path,
+                                                      MacroKind::Bang, force).map(Some);
             }
             InvocationKind::Attr { attr: None, .. } => return Ok(None),
             InvocationKind::Derive { name, span, .. } => {
                 let path = ast::Path::from_ident(span, Ident::with_empty_ctxt(name));
-                return self.cx.resolver.resolve_derive_macro(scope, &path, force).map(Some);
+                return self.cx.resolver.resolve_macro(scope, &path,
+                                                      MacroKind::Derive, force).map(Some)
             }
             InvocationKind::Attr { ref mut attr, ref traits, ref mut item } => (attr, traits, item),
         };
@@ -385,7 +387,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
         };
 
         let mut determined = true;
-        match self.cx.resolver.resolve_macro(scope, &path, force) {
+        match self.cx.resolver.resolve_macro(scope, &path, MacroKind::Attr, force) {
             Ok(ext) => return Ok(Some(ext)),
             Err(Determinacy::Undetermined) => determined = false,
             Err(Determinacy::Determined) if force => return Err(Determinacy::Determined),
@@ -394,7 +396,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
 
         for &(name, span) in traits {
             let path = ast::Path::from_ident(span, Ident::with_empty_ctxt(name));
-            match self.cx.resolver.resolve_macro(scope, &path, force) {
+            match self.cx.resolver.resolve_macro(scope, &path, MacroKind::Derive, force) {
                 Ok(ext) => if let SyntaxExtension::ProcMacroDerive(_, ref inert_attrs) = *ext {
                     if inert_attrs.contains(&attr_name) {
                         // FIXME(jseyfried) Avoid `mem::replace` here.