resolve: Turn the binding from #[macro_export] into a proper Import

Author: petrochenkov

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -56,21 +56,7 @@ impl<'a, Id: Into<DefId>> ToNameBinding<'a>
 impl<'a, Id: Into<DefId>> ToNameBinding<'a> for (Res, ty::Visibility<Id>, Span, LocalExpnId) {
     fn to_name_binding(self, arenas: &'a ResolverArenas<'a>) -> &'a NameBinding<'a> {
         arenas.alloc_name_binding(NameBinding {
-            kind: NameBindingKind::Res(self.0, false),
-            ambiguity: None,
-            vis: self.1.to_def_id(),
-            span: self.2,
-            expansion: self.3,
-        })
-    }
-}
-
-struct IsMacroExport;
-
-impl<'a> ToNameBinding<'a> for (Res, ty::Visibility, Span, LocalExpnId, IsMacroExport) {
-    fn to_name_binding(self, arenas: &'a ResolverArenas<'a>) -> &'a NameBinding<'a> {
-        arenas.alloc_name_binding(NameBinding {
-            kind: NameBindingKind::Res(self.0, true),
+            kind: NameBindingKind::Res(self.0),
             ambiguity: None,
             vis: self.1.to_def_id(),
             span: self.2,
@@ -1267,8 +1253,22 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
             let binding = (res, vis, span, expansion).to_name_binding(self.r.arenas);
             self.r.set_binding_parent_module(binding, parent_scope.module);
             if is_macro_export {
-                let module = self.r.graph_root;
-                self.r.define(module, ident, MacroNS, (res, vis, span, expansion, IsMacroExport));
+                let import = self.r.arenas.alloc_import(Import {
+                    kind: ImportKind::MacroExport,
+                    root_id: item.id,
+                    parent_scope: self.parent_scope,
+                    imported_module: Cell::new(None),
+                    has_attributes: false,
+                    use_span_with_attributes: span,
+                    use_span: span,
+                    root_span: span,
+                    span: span,
+                    module_path: Vec::new(),
+                    vis: Cell::new(Some(vis)),
+                    used: Cell::new(true),
+                });
+                let import_binding = self.r.import(binding, import);
+                self.r.define(self.r.graph_root, ident, MacroNS, import_binding);
             } else {
                 self.r.check_reserved_macro_name(ident, res);
                 self.insert_unused_macro(ident, def_id, item.id, &rule_spans);

compiler/rustc_resolve/src/diagnostics.rs

@@ -190,12 +190,12 @@ impl<'a> Resolver<'a> {
             ModuleKind::Block => "block",
         };
 
-        let old_noun = match old_binding.is_import() {
+        let old_noun = match old_binding.is_import_user_facing() {
             true => "import",
             false => "definition",
         };
 
-        let new_participle = match new_binding.is_import() {
+        let new_participle = match new_binding.is_import_user_facing() {
             true => "imported",
             false => "defined",
         };
@@ -226,7 +226,7 @@ impl<'a> Resolver<'a> {
                 true => struct_span_err!(self.session, span, E0254, "{}", msg),
                 false => struct_span_err!(self.session, span, E0260, "{}", msg),
             },
-            _ => match (old_binding.is_import(), new_binding.is_import()) {
+            _ => match (old_binding.is_import_user_facing(), new_binding.is_import_user_facing()) {
                 (false, false) => struct_span_err!(self.session, span, E0428, "{}", msg),
                 (true, true) => struct_span_err!(self.session, span, E0252, "{}", msg),
                 _ => struct_span_err!(self.session, span, E0255, "{}", msg),
@@ -248,14 +248,18 @@ impl<'a> Resolver<'a> {
 
         // See https://github.com/rust-lang/rust/issues/32354
         use NameBindingKind::Import;
+        let can_suggest = |binding: &NameBinding<'_>, import: &self::Import<'_>| {
+            !binding.span.is_dummy()
+                && !matches!(import.kind, ImportKind::MacroUse | ImportKind::MacroExport)
+        };
         let import = match (&new_binding.kind, &old_binding.kind) {
             // If there are two imports where one or both have attributes then prefer removing the
             // import without attributes.
             (Import { import: new, .. }, Import { import: old, .. })
                 if {
-                    !new_binding.span.is_dummy()
-                        && !old_binding.span.is_dummy()
-                        && (new.has_attributes || old.has_attributes)
+                    (new.has_attributes || old.has_attributes)
+                        && can_suggest(old_binding, old)
+                        && can_suggest(new_binding, new)
                 } =>
             {
                 if old.has_attributes {
@@ -265,10 +269,10 @@ impl<'a> Resolver<'a> {
                 }
             }
             // Otherwise prioritize the new binding.
-            (Import { import, .. }, other) if !new_binding.span.is_dummy() => {
+            (Import { import, .. }, other) if can_suggest(new_binding, import) => {
                 Some((import, new_binding.span, other.is_import()))
             }
-            (other, Import { import, .. }) if !old_binding.span.is_dummy() => {
+            (other, Import { import, .. }) if can_suggest(old_binding, import) => {
                 Some((import, old_binding.span, other.is_import()))
             }
             _ => None,
@@ -1683,7 +1687,7 @@ impl<'a> Resolver<'a> {
             let a = if built_in.is_empty() { res.article() } else { "a" };
             format!("{a}{built_in} {thing}{from}", thing = res.descr())
         } else {
-            let introduced = if b.is_import() { "imported" } else { "defined" };
+            let introduced = if b.is_import_user_facing() { "imported" } else { "defined" };
             format!("the {thing} {introduced} here", thing = res.descr())
         }
     }
@@ -1742,10 +1746,10 @@ impl<'a> Resolver<'a> {
     /// If the binding refers to a tuple struct constructor with fields,
     /// returns the span of its fields.
     fn ctor_fields_span(&self, binding: &NameBinding<'_>) -> Option<Span> {
-        if let NameBindingKind::Res(
-            Res::Def(DefKind::Ctor(CtorOf::Struct, CtorKind::Fn), ctor_def_id),
-            _,
-        ) = binding.kind
+        if let NameBindingKind::Res(Res::Def(
+            DefKind::Ctor(CtorOf::Struct, CtorKind::Fn),
+            ctor_def_id,
+        )) = binding.kind
         {
             let def_id = self.parent(ctor_def_id);
             let fields = self.field_names.get(&def_id)?;
@@ -1789,7 +1793,9 @@ impl<'a> Resolver<'a> {
                         next_ident = source;
                         Some(binding)
                     }
-                    ImportKind::Glob { .. } | ImportKind::MacroUse => Some(binding),
+                    ImportKind::Glob { .. } | ImportKind::MacroUse | ImportKind::MacroExport => {
+                        Some(binding)
+                    }
                     ImportKind::ExternCrate { .. } => None,
                 },
                 _ => None,

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -88,6 +88,11 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
                                 // here, but `macro_use` imports always refer to external items,
                                 // so it doesn't matter and we can just do nothing.
                             }
+                            ImportKind::MacroExport => {
+                                // In theory we should reset the parent id to something public
+                                // here, but it has the same effect as leaving the previous parent,
+                                // so we can just do nothing.
+                            }
                         }
 
                         level = Level::Reexported;
@@ -152,13 +157,6 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> {
                 self.update(def_id, Visibility::Public, parent_id, Level::Direct);
             }
 
-            // Only exported `macro_rules!` items are public, but they always are
-            ast::ItemKind::MacroDef(ref macro_def) if macro_def.macro_rules => {
-                let parent_id = self.r.local_parent(def_id);
-                let vis = self.r.visibilities[&def_id];
-                self.update(def_id, vis, parent_id, Level::Direct);
-            }
-
             ast::ItemKind::Mod(..) => {
                 self.set_bindings_effective_visibilities(def_id);
                 visit::walk_item(self, item);

compiler/rustc_resolve/src/ident.rs

@@ -19,7 +19,7 @@ use crate::late::{
 };
 use crate::macros::{sub_namespace_match, MacroRulesScope};
 use crate::{AmbiguityError, AmbiguityErrorMisc, AmbiguityKind, Determinacy, Finalize};
-use crate::{ImportKind, LexicalScopeBinding, Module, ModuleKind, ModuleOrUniformRoot};
+use crate::{Import, ImportKind, LexicalScopeBinding, Module, ModuleKind, ModuleOrUniformRoot};
 use crate::{NameBinding, NameBindingKind, ParentScope, PathResult, PrivacyError, Res};
 use crate::{ResolutionError, Resolver, Scope, ScopeSet, Segment, ToNameBinding, Weak};
 
@@ -915,7 +915,11 @@ impl<'a> Resolver<'a> {
             }
 
             if !restricted_shadowing && binding.expansion != LocalExpnId::ROOT {
-                if let NameBindingKind::Res(_, true) = binding.kind {
+                if let NameBindingKind::Import {
+                    import: Import { kind: ImportKind::MacroExport, .. },
+                    ..
+                } = binding.kind
+                {
                     self.macro_expanded_macro_export_errors.insert((path_span, binding.span));
                 }
             }

compiler/rustc_resolve/src/imports.rs

@@ -73,6 +73,7 @@ pub enum ImportKind<'a> {
         id: NodeId,
     },
     MacroUse,
+    MacroExport,
 }
 
 /// Manually implement `Debug` for `ImportKind` because the `source/target_bindings`
@@ -113,6 +114,7 @@ impl<'a> std::fmt::Debug for ImportKind<'a> {
                 .field("id", id)
                 .finish(),
             MacroUse => f.debug_struct("MacroUse").finish(),
+            MacroExport => f.debug_struct("MacroExport").finish(),
         }
     }
 }
@@ -177,7 +179,7 @@ impl<'a> Import<'a> {
             ImportKind::Single { id, .. }
             | ImportKind::Glob { id, .. }
             | ImportKind::ExternCrate { id, .. } => Some(id),
-            ImportKind::MacroUse => None,
+            ImportKind::MacroUse | ImportKind::MacroExport => None,
         }
     }
 }
@@ -883,7 +885,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
                                         match binding.kind {
                                             // Never suggest the name that has binding error
                                             // i.e., the name that cannot be previously resolved
-                                            NameBindingKind::Res(Res::Err, _) => None,
+                                            NameBindingKind::Res(Res::Err) => None,
                                             _ => Some(i.name),
                                         }
                                     }
@@ -1014,7 +1016,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
                     let mut err =
                         struct_span_err!(self.r.session, import.span, E0364, "{error_msg}");
                     match binding.kind {
-                        NameBindingKind::Res(Res::Def(DefKind::Macro(_), def_id), _)
+                        NameBindingKind::Res(Res::Def(DefKind::Macro(_), def_id))
                             // exclude decl_macro
                             if self.r.get_macro_by_def_id(def_id).macro_rules =>
                         {
@@ -1235,5 +1237,6 @@ fn import_kind_to_string(import_kind: &ImportKind<'_>) -> String {
         ImportKind::Glob { .. } => "*".to_string(),
         ImportKind::ExternCrate { .. } => "<extern crate>".to_string(),
         ImportKind::MacroUse => "#[macro_use]".to_string(),
+        ImportKind::MacroExport => "#[macro_export]".to_string(),
     }
 }

compiler/rustc_resolve/src/lib.rs

@@ -646,7 +646,7 @@ impl<'a> ToNameBinding<'a> for &'a NameBinding<'a> {
 
 #[derive(Clone, Debug)]
 enum NameBindingKind<'a> {
-    Res(Res, /* is_macro_export */ bool),
+    Res(Res),
     Module(Module<'a>),
     Import { binding: &'a NameBinding<'a>, import: &'a Import<'a>, used: Cell<bool> },
 }
@@ -745,7 +745,7 @@ impl<'a> NameBinding<'a> {
 
     fn res(&self) -> Res {
         match self.kind {
-            NameBindingKind::Res(res, _) => res,
+            NameBindingKind::Res(res) => res,
             NameBindingKind::Module(module) => module.res().unwrap(),
             NameBindingKind::Import { binding, .. } => binding.res(),
         }
@@ -762,10 +762,10 @@ impl<'a> NameBinding<'a> {
     fn is_possibly_imported_variant(&self) -> bool {
         match self.kind {
             NameBindingKind::Import { binding, .. } => binding.is_possibly_imported_variant(),
-            NameBindingKind::Res(
-                Res::Def(DefKind::Variant | DefKind::Ctor(CtorOf::Variant, ..), _),
+            NameBindingKind::Res(Res::Def(
+                DefKind::Variant | DefKind::Ctor(CtorOf::Variant, ..),
                 _,
-            ) => true,
+            )) => true,
             NameBindingKind::Res(..) | NameBindingKind::Module(..) => false,
         }
     }
@@ -788,6 +788,13 @@ impl<'a> NameBinding<'a> {
         matches!(self.kind, NameBindingKind::Import { .. })
     }
 
+    /// The binding introduced by `#[macro_export] macro_rules` is a public import, but it might
+    /// not be perceived as such by users, so treat it as a non-import in some diagnostics.
+    fn is_import_user_facing(&self) -> bool {
+        matches!(self.kind, NameBindingKind::Import { import, .. }
+            if !matches!(import.kind, ImportKind::MacroExport))
+    }
+
     fn is_glob_import(&self) -> bool {
         match self.kind {
             NameBindingKind::Import { import, .. } => import.is_glob(),
@@ -1283,7 +1290,7 @@ impl<'a> Resolver<'a> {
 
             arenas,
             dummy_binding: arenas.alloc_name_binding(NameBinding {
-                kind: NameBindingKind::Res(Res::Err, false),
+                kind: NameBindingKind::Res(Res::Err),
                 ambiguity: None,
                 expansion: LocalExpnId::ROOT,
                 span: DUMMY_SP,
@@ -1998,11 +2005,7 @@ impl<'a> Resolver<'a> {
 
     // Items that go to reexport table encoded to metadata and visible through it to other crates.
     fn is_reexport(&self, binding: &NameBinding<'a>) -> Option<def::Res<!>> {
-        // FIXME: Consider changing the binding inserted by `#[macro_export] macro_rules`
-        // into the crate root to actual `NameBindingKind::Import`.
-        if binding.is_import()
-            || matches!(binding.kind, NameBindingKind::Res(_, _is_macro_export @ true))
-        {
+        if binding.is_import() {
             let res = binding.res().expect_non_local();
             // Ambiguous imports are treated as errors at this point and are
             // not exposed to other crates (see #36837 for more details).

src/test/ui/macros/issue-38715.rs

@@ -1,7 +1,17 @@
 #[macro_export]
-macro_rules! foo { ($i:ident) => {} }
+macro_rules! foo { () => {} }
 
 #[macro_export]
 macro_rules! foo { () => {} } //~ ERROR the name `foo` is defined multiple times
 
+mod inner1 {
+    #[macro_export]
+    macro_rules! bar { () => {} }
+}
+
+mod inner2 {
+    #[macro_export]
+    macro_rules! bar { () => {} } //~ ERROR the name `bar` is defined multiple times
+}
+
 fn main() {}

src/test/ui/macros/issue-38715.stderr

@@ -1,14 +1,25 @@
 error[E0428]: the name `foo` is defined multiple times
   --> $DIR/issue-38715.rs:5:1
    |
-LL | macro_rules! foo { ($i:ident) => {} }
+LL | macro_rules! foo { () => {} }
    | ---------------- previous definition of the macro `foo` here
 ...
 LL | macro_rules! foo { () => {} }
    | ^^^^^^^^^^^^^^^^ `foo` redefined here
    |
    = note: `foo` must be defined only once in the macro namespace of this module
 
-error: aborting due to previous error
+error[E0428]: the name `bar` is defined multiple times
+  --> $DIR/issue-38715.rs:14:5
+   |
+LL |     macro_rules! bar { () => {} }
+   |     ---------------- previous definition of the macro `bar` here
+...
+LL |     macro_rules! bar { () => {} }
+   |     ^^^^^^^^^^^^^^^^ `bar` redefined here
+   |
+   = note: `bar` must be defined only once in the macro namespace of this module
+
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0428`.

src/test/ui/privacy/effective_visibilities.rs

@@ -38,13 +38,13 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub
     }
 
     #[rustc_effective_visibility]
-    macro_rules! none_macro { //~ Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate)
+    macro_rules! none_macro { //~ ERROR not in the table
         () => {};
     }
 
     #[macro_export]
     #[rustc_effective_visibility]
-    macro_rules! public_macro { //~ Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+    macro_rules! public_macro { //~ ERROR Direct: pub(self), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
         () => {};
     }
 

src/test/ui/privacy/effective_visibilities.stderr

@@ -64,13 +64,13 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
 LL |                 PubUnion,
    |                 ^^^^^^^^
 
-error: Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate)
+error: not in the table
   --> $DIR/effective_visibilities.rs:41:5
    |
 LL |     macro_rules! none_macro {
    |     ^^^^^^^^^^^^^^^^^^^^^^^
 
-error: Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+error: Direct: pub(self), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
   --> $DIR/effective_visibilities.rs:47:5
    |
 LL |     macro_rules! public_macro {