Suggest macro import from crate root.

davidtwco · davidtwco · commit d84907bbccf0 · 2019-04-12T01:54:32.000+02:00
This commit suggests importing a macro from the root of a crate as the
intent may have been to import a macro from the definition location that
was annotated with `#[macro_export]`.
diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs
@@ -5,15 +5,16 @@ use log::debug;
 use rustc::hir::def::{Def, CtorKind, Namespace::*};
 use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId};
 use rustc::session::config::nightly_options;
-use syntax::ast::{Expr, ExprKind};
+use syntax::ast::{Expr, ExprKind, Ident};
+use syntax::ext::base::MacroKind;
 use syntax::symbol::keywords;
 use syntax_pos::Span;
 
 use crate::macros::ParentScope;
-use crate::resolve_imports::ImportResolver;
+use crate::resolve_imports::{ImportDirective, ImportResolver};
 use crate::{import_candidate_to_enum_paths, is_self_type, is_self_value, path_names_to_string};
 use crate::{AssocSuggestion, CrateLint, ImportSuggestion, ModuleOrUniformRoot, PathResult,
-            PathSource, Resolver, Segment};
+            PathSource, Resolver, Segment, Suggestion};
 
 impl<'a> Resolver<'a> {
     /// Handles error reporting for `smart_resolve_path_fragment` function.
@@ -428,7 +429,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         span: Span,
         mut path: Vec<Segment>,
         parent_scope: &ParentScope<'b>,
-    ) -> Option<(Vec<Segment>, Option<String>)> {
+    ) -> Option<(Vec<Segment>, Vec<String>)> {
         debug!("make_path_suggestion: span={:?} path={:?}", span, path);
 
         match (path.get(0), path.get(1)) {
@@ -463,13 +464,13 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         span: Span,
         mut path: Vec<Segment>,
         parent_scope: &ParentScope<'b>,
-    ) -> Option<(Vec<Segment>, Option<String>)> {
+    ) -> Option<(Vec<Segment>, Vec<String>)> {
         // Replace first ident with `self` and check if that is valid.
         path[0].ident.name = keywords::SelfLower.name();
         let result = self.resolve_path(&path, None, parent_scope, false, span, CrateLint::No);
         debug!("make_missing_self_suggestion: path={:?} result={:?}", path, result);
         if let PathResult::Module(..) = result {
-            Some((path, None))
+            Some((path, Vec::new()))
         } else {
             None
         }
@@ -487,19 +488,19 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         span: Span,
         mut path: Vec<Segment>,
         parent_scope: &ParentScope<'b>,
-    ) -> Option<(Vec<Segment>, Option<String>)> {
+    ) -> Option<(Vec<Segment>, Vec<String>)> {
         // Replace first ident with `crate` and check if that is valid.
         path[0].ident.name = keywords::Crate.name();
         let result = self.resolve_path(&path, None, parent_scope, false, span, CrateLint::No);
         debug!("make_missing_crate_suggestion:  path={:?} result={:?}", path, result);
         if let PathResult::Module(..) = result {
             Some((
                 path,
-                Some(
+                vec![
                     "`use` statements changed in Rust 2018; read more at \
                      <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-\
                      clarity.html>".to_string()
-                ),
+                ],
             ))
         } else {
             None
@@ -518,13 +519,13 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         span: Span,
         mut path: Vec<Segment>,
         parent_scope: &ParentScope<'b>,
-    ) -> Option<(Vec<Segment>, Option<String>)> {
+    ) -> Option<(Vec<Segment>, Vec<String>)> {
         // Replace first ident with `crate` and check if that is valid.
         path[0].ident.name = keywords::Super.name();
         let result = self.resolve_path(&path, None, parent_scope, false, span, CrateLint::No);
         debug!("make_missing_super_suggestion:  path={:?} result={:?}", path, result);
         if let PathResult::Module(..) = result {
-            Some((path, None))
+            Some((path, Vec::new()))
         } else {
             None
         }
@@ -545,7 +546,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         span: Span,
         mut path: Vec<Segment>,
         parent_scope: &ParentScope<'b>,
-    ) -> Option<(Vec<Segment>, Option<String>)> {
+    ) -> Option<(Vec<Segment>, Vec<String>)> {
         if path[1].ident.span.rust_2015() {
             return None;
         }
@@ -564,10 +565,65 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
             debug!("make_external_crate_suggestion: name={:?} path={:?} result={:?}",
                     name, path, result);
             if let PathResult::Module(..) = result {
-                return Some((path, None));
+                return Some((path, Vec::new()));
             }
         }
 
         None
     }
+
+    /// Suggests importing a macro from the root of the crate rather than a module within
+    /// the crate.
+    ///
+    /// ```
+    /// help: a macro with this name exists at the root of the crate
+    ///    |
+    /// LL | use issue_59764::makro;
+    ///    |     ^^^^^^^^^^^^^^^^^^
+    ///    |
+    ///    = note: this could be because a macro annotated with `#[macro_export]` will be exported
+    ///            at the root of the crate instead of the module where it is defined
+    /// ```
+    pub(crate) fn check_for_module_export_macro(
+        &self,
+        directive: &'b ImportDirective<'b>,
+        module: ModuleOrUniformRoot<'b>,
+        ident: Ident,
+    ) -> Option<(Option<Suggestion>, Vec<String>)> {
+        let mut crate_module = if let ModuleOrUniformRoot::Module(module) = module {
+            module
+        } else {
+            return None;
+        };
+
+        while let Some(parent) = crate_module.parent {
+            crate_module = parent;
+        }
+
+        if ModuleOrUniformRoot::same_def(ModuleOrUniformRoot::Module(crate_module), module) {
+            // Don't make a suggestion if the import was already from the root of the
+            // crate.
+            return None;
+        }
+
+        let resolutions = crate_module.resolutions.borrow();
+        let resolution = resolutions.get(&(ident, MacroNS))?;
+        let binding = resolution.borrow().binding()?;
+        if let Def::Macro(_, MacroKind::Bang) = binding.def() {
+            let name = crate_module.kind.name().unwrap();
+            let suggestion = Some((
+                directive.span,
+                String::from("a macro with this name exists at the root of the crate"),
+                format!("{}::{}", name, ident),
+                Applicability::MaybeIncorrect,
+            ));
+            let note = vec![
+                "this could be because a macro annotated with `#[macro_export]` will be exported \
+                 at the root of the crate instead of the module where it is defined".to_string(),
+            ];
+            Some((suggestion, note))
+        } else {
+            None
+        }
+    }
 }
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
@@ -1091,6 +1091,16 @@ enum ModuleKind {
     Def(Def, Name),
 }
 
+impl ModuleKind {
+    /// Get name of the module.
+    pub fn name(&self) -> Option<Name> {
+        match self {
+            ModuleKind::Block(..) => None,
+            ModuleKind::Def(_, name) => Some(*name),
+        }
+    }
+}
+
 /// One node in the tree of modules.
 pub struct ModuleData<'a> {
     parent: Option<Module<'a>>,
diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs
@@ -145,7 +145,7 @@ pub struct NameResolution<'a> {
 
 impl<'a> NameResolution<'a> {
     // Returns the binding for the name if it is known or None if it not known.
-    fn binding(&self) -> Option<&'a NameBinding<'a>> {
+    pub(crate) fn binding(&self) -> Option<&'a NameBinding<'a>> {
         self.binding.and_then(|binding| {
             if !binding.is_glob_import() ||
                self.single_imports.is_empty() { Some(binding) } else { None }
@@ -636,7 +636,7 @@ impl<'a> Resolver<'a> {
 struct UnresolvedImportError {
     span: Span,
     label: Option<String>,
-    note: Option<String>,
+    note: Vec<String>,
     suggestion: Option<Suggestion>,
 }
 
@@ -756,8 +756,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         /// Upper limit on the number of `span_label` messages.
         const MAX_LABEL_COUNT: usize = 10;
 
-        let (span, msg, note) = if errors.is_empty() {
-            (span.unwrap(), "unresolved import".to_string(), None)
+        let (span, msg) = if errors.is_empty() {
+            (span.unwrap(), "unresolved import".to_string())
         } else {
             let span = MultiSpan::from_spans(
                 errors
@@ -766,11 +766,6 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                     .collect(),
             );
 
-            let note = errors
-                .iter()
-                .filter_map(|(_, err)| err.note.as_ref())
-                .last();
-
             let paths = errors
                 .iter()
                 .map(|(path, _)| format!("`{}`", path))
@@ -782,13 +777,15 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                 paths.join(", "),
             );
 
-            (span, msg, note)
+            (span, msg)
         };
 
         let mut diag = struct_span_err!(self.resolver.session, span, E0432, "{}", &msg);
 
-        if let Some(note) = &note {
-            diag.note(note);
+        if let Some((_, UnresolvedImportError { note, .. })) = errors.iter().last() {
+            for message in note {
+                diag.note(&message);
+            }
         }
 
         for (_, err) in errors.into_iter().take(MAX_LABEL_COUNT) {
@@ -961,7 +958,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                             UnresolvedImportError {
                                 span,
                                 label: Some(label),
-                                note: None,
+                                note: Vec::new(),
                                 suggestion,
                             }
                         }
@@ -1006,7 +1003,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                         return Some(UnresolvedImportError {
                             span: directive.span,
                             label: Some(String::from("cannot glob-import a module into itself")),
-                            note: None,
+                            note: Vec::new(),
                             suggestion: None,
                         });
                     }
@@ -1114,15 +1111,22 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                     }
                 });
 
-                let lev_suggestion =
-                    find_best_match_for_name(names, &ident.as_str(), None).map(|suggestion| {
-                        (
-                            ident.span,
-                            String::from("a similar name exists in the module"),
-                            suggestion.to_string(),
-                            Applicability::MaybeIncorrect,
-                        )
-                    });
+                let (suggestion, note) = if let Some((suggestion, note)) =
+                    self.check_for_module_export_macro(directive, module, ident)
+                {
+
+                    (
+                        suggestion.or_else(||
+                           find_best_match_for_name(names, &ident.as_str(), None)
+                           .map(|suggestion|
+                                (ident.span, String::from("a similar name exists in the module"),
+                                 suggestion.to_string(), Applicability::MaybeIncorrect)
+                            )),
+                        note,
+                    )
+                } else {
+                    (None, Vec::new())
+                };
 
                 let label = match module {
                     ModuleOrUniformRoot::Module(module) => {
@@ -1143,11 +1147,12 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                         }
                     }
                 };
+
                 Some(UnresolvedImportError {
                     span: directive.span,
                     label: Some(label),
-                    note: None,
-                    suggestion: lev_suggestion,
+                    note,
+                    suggestion,
                 })
             } else {
                 // `resolve_ident_in_module` reported a privacy error.
diff --git a/src/test/ui/issue-59764.fixed b/src/test/ui/issue-59764.fixed
@@ -0,0 +1,15 @@
+// aux-build:issue-59764.rs
+// compile-flags:--extern issue_59764
+// edition:2018
+// run-rustfix
+
+use issue_59764::makro;
+//~^ ERROR unresolved import `issue_59764::foo::makro` [E0432]
+
+makro!(bar);
+//~^ ERROR cannot determine resolution for the macro `makro`
+
+fn main() {
+    bar();
+    //~^ ERROR cannot find function `bar` in this scope [E0425]
+}
diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs
@@ -1,6 +1,7 @@
 // aux-build:issue-59764.rs
 // compile-flags:--extern issue_59764
 // edition:2018
+// run-rustfix
 
 use issue_59764::foo::makro;
 //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432]
diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr
@@ -1,19 +1,25 @@
 error[E0432]: unresolved import `issue_59764::foo::makro`
-  --> $DIR/issue-59764.rs:5:5
+  --> $DIR/issue-59764.rs:6:5
    |
 LL | use issue_59764::foo::makro;
    |     ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo`
+   |
+   = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined
+help: a macro with this name exists at the root of the crate
+   |
+LL | use issue_59764::makro;
+   |     ^^^^^^^^^^^^^^^^^^
 
 error: cannot determine resolution for the macro `makro`
-  --> $DIR/issue-59764.rs:8:1
+  --> $DIR/issue-59764.rs:9:1
    |
 LL | makro!(bar);
    | ^^^^^
    |
    = note: import resolution is stuck, try simplifying macro imports
 
 error[E0425]: cannot find function `bar` in this scope
-  --> $DIR/issue-59764.rs:12:5
+  --> $DIR/issue-59764.rs:13:5
    |
 LL |     bar();
    |     ^^^ not found in this scope
