resolve: Model resolve_legacy_scope after resolve_lexical_macro_path_segment

Author: petrochenkov

src/librustc_resolve/lib.rs

@@ -1402,8 +1402,6 @@ pub struct Resolver<'a, 'b: 'a> {
     proc_mac_errors: Vec<macros::ProcMacError>,
     /// crate-local macro expanded `macro_export` referred to by a module-relative path
     macro_expanded_macro_export_errors: BTreeSet<(Span, Span)>,
-    /// macro-expanded `macro_rules` shadowing existing macros
-    disallowed_shadowing: Vec<&'a LegacyBinding<'a>>,
 
     arenas: &'a ResolverArenas<'a>,
     dummy_binding: &'a NameBinding<'a>,
@@ -1714,7 +1712,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
             ambiguity_errors: Vec::new(),
             use_injections: Vec::new(),
             proc_mac_errors: Vec::new(),
-            disallowed_shadowing: Vec::new(),
             macro_expanded_macro_export_errors: BTreeSet::new(),
 
             arenas,
@@ -4532,7 +4529,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
     }
 
     fn report_errors(&mut self, krate: &Crate) {
-        self.report_shadowing_errors();
         self.report_with_use_injections(krate);
         self.report_proc_macro_import(krate);
         let mut reported_spans = FxHashSet();
@@ -4570,20 +4566,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
         }
     }
 
-    fn report_shadowing_errors(&mut self) {
-        let mut reported_errors = FxHashSet();
-        for binding in replace(&mut self.disallowed_shadowing, Vec::new()) {
-            if self.resolve_legacy_scope(&binding.parent, binding.ident, false).is_some() &&
-               reported_errors.insert((binding.ident, binding.binding.span)) {
-                let msg = format!("`{}` is already in scope", binding.ident);
-                self.session.struct_span_err(binding.binding.span, &msg)
-                    .note("macro-expanded `macro_rules!`s may not shadow \
-                           existing macros (see RFC 1560)")
-                    .emit();
-            }
-        }
-    }
-
     fn report_conflict<'b>(&mut self,
                        parent: Module,
                        ident: Ident,

src/librustc_resolve/macros.rs

@@ -81,9 +81,9 @@ pub enum LegacyScope<'a> {
 // Binding produced by a `macro_rules` item.
 // Not modularized, can shadow previous legacy bindings, etc.
 pub struct LegacyBinding<'a> {
-    pub binding: &'a NameBinding<'a>,
-    pub parent: Cell<LegacyScope<'a>>,
-    pub ident: Ident,
+    binding: &'a NameBinding<'a>,
+    parent: Cell<LegacyScope<'a>>,
+    ident: Ident,
 }
 
 pub struct ProcMacError {
@@ -761,42 +761,101 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
         }
     }
 
-    crate fn resolve_legacy_scope(&mut self,
-                                  mut scope: &'a Cell<LegacyScope<'a>>,
-                                  ident: Ident,
-                                  record_used: bool)
-                                  -> Option<(&'a NameBinding<'a>, FromExpansion)> {
+    fn resolve_legacy_scope(&mut self,
+                            scope: &'a Cell<LegacyScope<'a>>,
+                            ident: Ident,
+                            record_used: bool)
+                            -> Option<(&'a NameBinding<'a>, FromExpansion)> {
         let ident = ident.modern();
-        let mut relative_depth: u32 = 0;
+
+        // Names from inner scope that can't shadow names from outer scopes, e.g.
+        // macro_rules! mac { ... }
+        // {
+        //     define_mac!(); // if this generates another `macro_rules! mac`, then it can't shadow
+        //                    // the outer `mac` and we have and ambiguity error
+        //     mac!();
+        // }
+        let mut potentially_ambiguous_result: Option<(&NameBinding, FromExpansion)> = None;
+
+        // Go through all the scopes and try to resolve the name.
+        let mut where_to_resolve = scope;
+        let mut relative_depth = 0u32;
         loop {
-            match scope.get() {
-                LegacyScope::Empty => break,
-                LegacyScope::Expansion(invocation) => {
-                    match invocation.expansion.get() {
-                        LegacyScope::Invocation(_) => scope.set(invocation.legacy_scope.get()),
-                        LegacyScope::Empty => {
-                            scope = &invocation.legacy_scope;
-                        }
-                        _ => {
+            let result = match where_to_resolve.get() {
+                LegacyScope::Binding(legacy_binding) => if ident == legacy_binding.ident {
+                    Some((legacy_binding.binding, FromExpansion(relative_depth > 0)))
+                } else {
+                    None
+                }
+                _ => None,
+            };
+
+            macro_rules! continue_search { () => {
+                where_to_resolve = match where_to_resolve.get() {
+                    LegacyScope::Binding(binding) => &binding.parent,
+                    LegacyScope::Invocation(invocation) => {
+                        relative_depth = relative_depth.saturating_sub(1);
+                        &invocation.legacy_scope
+                    }
+                    LegacyScope::Expansion(invocation) => match invocation.expansion.get() {
+                        LegacyScope::Empty => &invocation.legacy_scope,
+                        LegacyScope::Binding(..) | LegacyScope::Expansion(..) => {
                             relative_depth += 1;
-                            scope = &invocation.expansion;
+                            &invocation.expansion
+                        }
+                        LegacyScope::Invocation(..) => {
+                            where_to_resolve.set(invocation.legacy_scope.get());
+                            where_to_resolve
                         }
                     }
-                }
-                LegacyScope::Invocation(invocation) => {
-                    relative_depth = relative_depth.saturating_sub(1);
-                    scope = &invocation.legacy_scope;
-                }
-                LegacyScope::Binding(potential_binding) => {
-                    if potential_binding.ident == ident {
-                        if record_used && relative_depth > 0 {
-                            self.disallowed_shadowing.push(potential_binding);
+                    LegacyScope::Empty => break, // nowhere else to search
+                };
+
+                continue;
+            }}
+
+            match result {
+                Some(result) => {
+                    if !record_used {
+                        return Some(result);
+                    }
+
+                    // Found a solution that is ambiguous with a previously found solution.
+                    // Push an ambiguity error for later reporting and
+                    // return something for better recovery.
+                    if let Some(previous_result) = potentially_ambiguous_result {
+                        if result.0.def() != previous_result.0.def() {
+                            self.ambiguity_errors.push(AmbiguityError {
+                                span: ident.span,
+                                name: ident.name,
+                                b1: previous_result.0,
+                                b2: result.0,
+                            });
+                            return Some(previous_result);
                         }
-                        return Some((potential_binding.binding, FromExpansion(relative_depth > 0)));
                     }
-                    scope = &potential_binding.parent;
+
+                    // Found a solution that's not an ambiguity yet, but is "suspicious" and
+                    // can participate in ambiguities later on.
+                    // Remember it and go search for other solutions in outer scopes.
+                    if (result.1).0 {
+                        potentially_ambiguous_result = Some(result);
+
+                        continue_search!();
+                    }
+
+                    // Found a solution that can't be ambiguous.
+                    return Some(result);
                 }
-            };
+                None => {
+                    continue_search!();
+                }
+            }
+        }
+
+        // Previously found potentially ambiguous result turned out to not be ambiguous after all.
+        if let Some(previous_result) = potentially_ambiguous_result {
+            return Some(previous_result);
         }
 
         None
@@ -823,8 +882,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
 
             let check_consistency = |this: &Self, new_def: Def| {
                 if let Some(def) = def {
-                    if this.ambiguity_errors.is_empty() && this.disallowed_shadowing.is_empty() &&
-                       new_def != def && new_def != Def::Err {
+                    if this.ambiguity_errors.is_empty() && new_def != def && new_def != Def::Err {
                         // Make sure compilation does not succeed if preferred macro resolution
                         // has changed after the macro had been expanded. In theory all such
                         // situations should be reported as ambiguity errors, so this is span-bug.

src/test/ui/macros/macro-shadowing.rs

@@ -17,14 +17,14 @@ macro_rules! macro_one { () => {} }
 #[macro_use(macro_two)] extern crate two_macros;
 
 macro_rules! m1 { () => {
-    macro_rules! foo { () => {} } //~ ERROR `foo` is already in scope
+    macro_rules! foo { () => {} }
 
     #[macro_use] //~ ERROR `macro_two` is already in scope
     extern crate two_macros as __;
 }}
 m1!();
 
-foo!();
+foo!(); //~ ERROR `foo` is ambiguous
 
 macro_rules! m2 { () => {
     macro_rules! foo { () => {} }

src/test/ui/macros/macro-shadowing.stderr

@@ -9,16 +9,27 @@ LL | m1!();
    |
    = note: macro-expanded `#[macro_use]`s may not shadow existing macros (see RFC 1560)
 
-error: `foo` is already in scope
+error[E0659]: `foo` is ambiguous
+  --> $DIR/macro-shadowing.rs:27:1
+   |
+LL | foo!(); //~ ERROR `foo` is ambiguous
+   | ^^^
+   |
+note: `foo` could refer to the name defined here
   --> $DIR/macro-shadowing.rs:20:5
    |
-LL |     macro_rules! foo { () => {} } //~ ERROR `foo` is already in scope
+LL |     macro_rules! foo { () => {} }
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 ...
 LL | m1!();
    | ------ in this macro invocation
+note: `foo` could also refer to the name defined here
+  --> $DIR/macro-shadowing.rs:15:1
    |
-   = note: macro-expanded `macro_rules!`s may not shadow existing macros (see RFC 1560)
+LL | macro_rules! foo { () => {} }
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: macro-expanded macros do not shadow
 
 error: aborting due to 2 previous errors
 
+For more information about this error, try `rustc --explain E0659`.

src/test/ui/out-of-order-shadowing.rs

@@ -9,11 +9,10 @@
 // except according to those terms.
 
 // aux-build:define_macro.rs
-// error-pattern: `bar` is already in scope
 
 macro_rules! bar { () => {} }
 define_macro!(bar);
-bar!();
+bar!(); //~ ERROR `bar` is ambiguous
 
 macro_rules! m { () => { #[macro_use] extern crate define_macro; } }
 m!();

src/test/ui/out-of-order-shadowing.stderr

@@ -1,11 +1,22 @@
-error: `bar` is already in scope
+error[E0659]: `bar` is ambiguous
   --> $DIR/out-of-order-shadowing.rs:15:1
    |
+LL | bar!(); //~ ERROR `bar` is ambiguous
+   | ^^^
+   |
+note: `bar` could refer to the name defined here
+  --> $DIR/out-of-order-shadowing.rs:14:1
+   |
 LL | define_macro!(bar);
    | ^^^^^^^^^^^^^^^^^^^
+note: `bar` could also refer to the name defined here
+  --> $DIR/out-of-order-shadowing.rs:13:1
    |
-   = note: macro-expanded `macro_rules!`s may not shadow existing macros (see RFC 1560)
+LL | macro_rules! bar { () => {} }
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: macro-expanded macros do not shadow
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
 
 error: aborting due to previous error
 
+For more information about this error, try `rustc --explain E0659`.