Auto merge of #54605 - petrochenkov:mambig, r=alexcrichton

resolve: Disambiguate a subset of conflicts "macro_rules" vs "macro name in module"

Currently if macro name may refer to both a `macro_rules` macro definition and a macro defined/imported into module we conservatively report an ambiguity error.
Unfortunately, these errors became a source of regressions when macro modularization was enabled - see issue #54472.

This PR disambiguates such conflicts in favor of `macro_rules` if both the `macro_rules` item and in-module macro name are defined in the same normal (named) module and `macro_rules` is closer in scope to the point of use (see the tests for examples).
This is a subset of more general approach described in #54472 (comment).
The subset is enough to fix all the regressions from #54472, but it can be extended to apply to all "macro_rules" vs "macro name in module" conflicts in the future.

To give an analogy, this is equivalent to scoping rules for `let` variables and items defined in blocks (`macro_rules` behaves like "`let` at module level" in general).
```rust
{ // beginning of the block
    use xxx::m; // (1)

    // Starting from the beginning of the block and until here m!() refers to (1)
    macro_rules! m { ... } // (2)
    // Starting from here and until the end of the block m!() refers to (2)
} // end of the block
```
More complex examples with `use` and `macro_rules` from different modules still report ambiguity errors, even if equivalent examples with `let` are legal.

Fixes #54472 (stable-to-beta regression)

Author: bors

src/librustc_resolve/lib.rs

@@ -72,11 +72,10 @@ use syntax_pos::{Span, DUMMY_SP, MultiSpan};
 use errors::{Applicability, DiagnosticBuilder, DiagnosticId};
 
 use std::cell::{Cell, RefCell};
-use std::cmp;
+use std::{cmp, fmt, iter, ptr};
 use std::collections::BTreeSet;
-use std::fmt;
-use std::iter;
 use std::mem::replace;
+use rustc_data_structures::ptr_key::PtrKey;
 use rustc_data_structures::sync::Lrc;
 
 use resolve_imports::{ImportDirective, ImportDirectiveSubclass, NameResolution, ImportResolver};
@@ -1114,6 +1113,17 @@ impl<'a> ModuleData<'a> {
     fn nearest_item_scope(&'a self) -> Module<'a> {
         if self.is_trait() { self.parent.unwrap() } else { self }
     }
+
+    fn is_ancestor_of(&self, mut other: &Self) -> bool {
+        while !ptr::eq(self, other) {
+            if let Some(parent) = other.parent {
+                other = parent;
+            } else {
+                return false;
+            }
+        }
+        true
+    }
 }
 
 impl<'a> fmt::Debug for ModuleData<'a> {
@@ -1411,6 +1421,7 @@ pub struct Resolver<'a, 'b: 'a> {
     block_map: NodeMap<Module<'a>>,
     module_map: FxHashMap<DefId, Module<'a>>,
     extern_module_map: FxHashMap<(DefId, bool /* MacrosOnly? */), Module<'a>>,
+    binding_parent_modules: FxHashMap<PtrKey<'a, NameBinding<'a>>, Module<'a>>,
 
     pub make_glob_map: bool,
     /// Maps imports to the names of items actually imported (this actually maps
@@ -1714,6 +1725,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
             module_map,
             block_map: NodeMap(),
             extern_module_map: FxHashMap(),
+            binding_parent_modules: FxHashMap(),
 
             make_glob_map: make_glob_map == MakeGlobMap::Yes,
             glob_map: NodeMap(),
@@ -4596,6 +4608,31 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
         vis.is_accessible_from(module.normal_ancestor_id, self)
     }
 
+    fn set_binding_parent_module(&mut self, binding: &'a NameBinding<'a>, module: Module<'a>) {
+        if let Some(old_module) = self.binding_parent_modules.insert(PtrKey(binding), module) {
+            if !ptr::eq(module, old_module) {
+                span_bug!(binding.span, "parent module is reset for binding");
+            }
+        }
+    }
+
+    fn disambiguate_legacy_vs_modern(
+        &self,
+        legacy: &'a NameBinding<'a>,
+        modern: &'a NameBinding<'a>,
+    ) -> bool {
+        // Some non-controversial subset of ambiguities "modern macro name" vs "macro_rules"
+        // is disambiguated to mitigate regressions from macro modularization.
+        // Scoping for `macro_rules` behaves like scoping for `let` at module level, in general.
+        match (self.binding_parent_modules.get(&PtrKey(legacy)),
+               self.binding_parent_modules.get(&PtrKey(modern))) {
+            (Some(legacy), Some(modern)) =>
+                legacy.normal_ancestor_id == modern.normal_ancestor_id &&
+                modern.is_ancestor_of(legacy),
+            _ => false,
+        }
+    }
+
     fn report_ambiguity_error(&self, ident: Ident, b1: &NameBinding, b2: &NameBinding) {
         let participle = |is_import: bool| if is_import { "imported" } else { "defined" };
         let msg1 =

src/librustc_resolve/macros.rs

@@ -957,11 +957,13 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                 },
                 (Some(legacy_binding), Ok((binding, FromPrelude(from_prelude))))
                         if legacy_binding.def() != binding.def_ignoring_ambiguity() &&
-                           (!from_prelude ||
+                           (!from_prelude &&
+                            !self.disambiguate_legacy_vs_modern(legacy_binding, binding) ||
                             legacy_binding.may_appear_after(parent_scope.expansion, binding)) => {
                     self.report_ambiguity_error(ident, legacy_binding, binding);
                 },
                 // OK, non-macro-expanded legacy wins over prelude even if defs are different
+                // Also, non-macro-expanded legacy wins over modern from the same module
                 // Also, legacy and modern can co-exist if their defs are same
                 (Some(legacy_binding), Ok(_)) |
                 // OK, unambiguous resolution
@@ -1097,6 +1099,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
             let def = Def::Macro(def_id, MacroKind::Bang);
             let vis = ty::Visibility::Invisible; // Doesn't matter for legacy bindings
             let binding = (def, vis, item.span, expansion).to_name_binding(self.arenas);
+            self.set_binding_parent_module(binding, self.current_module);
             let legacy_binding = self.arenas.alloc_legacy_binding(LegacyBinding {
                 parent_legacy_scope: *current_legacy_scope, binding, ident
             });

src/librustc_resolve/resolve_imports.rs

@@ -478,6 +478,7 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
                       binding: &'a NameBinding<'a>)
                       -> Result<(), &'a NameBinding<'a>> {
         self.check_reserved_macro_name(ident, ns);
+        self.set_binding_parent_module(binding, module);
         self.update_resolution(module, ident, ns, |this, resolution| {
             if let Some(old_binding) = resolution.binding {
                 if binding.is_glob_import() {

src/test/ui/imports/macros.rs

@@ -45,7 +45,7 @@ mod m3 {
 mod m4 {
     macro_rules! m { () => {} }
     use two_macros::m;
-    m!(); //~ ERROR ambiguous
+    m!();
 }
 
 fn main() {}

src/test/ui/imports/macros.stderr

@@ -1,20 +1,3 @@
-error[E0659]: `m` is ambiguous
-  --> $DIR/macros.rs:48:5
-   |
-LL |     m!(); //~ ERROR ambiguous
-   |     ^ ambiguous name
-   |
-note: `m` could refer to the name defined here
-  --> $DIR/macros.rs:46:5
-   |
-LL |     macro_rules! m { () => {} }
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
-note: `m` could also refer to the name imported here
-  --> $DIR/macros.rs:47:9
-   |
-LL |     use two_macros::m;
-   |         ^^^^^^^^^^^^^
-
 error[E0659]: `m` is ambiguous
   --> $DIR/macros.rs:26:5
    |
@@ -51,6 +34,6 @@ LL |     use two_macros::m;
    |         ^^^^^^^^^^^^^
    = note: macro-expanded macro imports do not shadow
 
-error: aborting due to 3 previous errors
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0659`.

src/test/ui/macros/ambiguity-legacy-vs-modern.rs

@@ -0,0 +1,46 @@
+// Some non-controversial subset of ambiguities "modern macro name" vs "macro_rules"
+// is disambiguated to mitigate regressions from macro modularization.
+// Scoping for `macro_rules` behaves like scoping for `let` at module level, in general.
+
+#![feature(decl_macro)]
+
+fn same_unnamed_mod() {
+    macro m() { 0 }
+
+    macro_rules! m { () => (()) }
+
+    m!() // OK
+}
+
+fn nested_unnamed_mod() {
+    macro m() { 0 }
+
+    {
+        macro_rules! m { () => (()) }
+
+        m!() // OK
+    }
+}
+
+fn nested_unnamed_mod_fail() {
+    macro_rules! m { () => (()) }
+
+    {
+        macro m() { 0 }
+
+        m!() //~ ERROR `m` is ambiguous
+    }
+}
+
+fn nexted_named_mod_fail() {
+    macro m() { 0 }
+
+    #[macro_use]
+    mod inner {
+        macro_rules! m { () => (()) }
+    }
+
+    m!() //~ ERROR `m` is ambiguous
+}
+
+fn main() {}

src/test/ui/macros/ambiguity-legacy-vs-modern.stderr

@@ -0,0 +1,37 @@
+error[E0659]: `m` is ambiguous
+  --> $DIR/ambiguity-legacy-vs-modern.rs:31:9
+   |
+LL |         m!() //~ ERROR `m` is ambiguous
+   |         ^ ambiguous name
+   |
+note: `m` could refer to the name defined here
+  --> $DIR/ambiguity-legacy-vs-modern.rs:26:5
+   |
+LL |     macro_rules! m { () => (()) }
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+note: `m` could also refer to the name defined here
+  --> $DIR/ambiguity-legacy-vs-modern.rs:29:9
+   |
+LL |         macro m() { 0 }
+   |         ^^^^^^^^^^^^^^^
+
+error[E0659]: `m` is ambiguous
+  --> $DIR/ambiguity-legacy-vs-modern.rs:43:5
+   |
+LL |     m!() //~ ERROR `m` is ambiguous
+   |     ^ ambiguous name
+   |
+note: `m` could refer to the name defined here
+  --> $DIR/ambiguity-legacy-vs-modern.rs:40:9
+   |
+LL |         macro_rules! m { () => (()) }
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+note: `m` could also refer to the name defined here
+  --> $DIR/ambiguity-legacy-vs-modern.rs:36:5
+   |
+LL |     macro m() { 0 }
+   |     ^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0659`.