Auto merge of #37506 - jseyfried:improve_shadowing_checks, r=nrc

macros: improve shadowing checks

This PR improves macro-expanded shadowing checks to work with out-of-(pre)order expansion.

Out-of-order expansion became possible in #37084, so this technically a [breaking-change] for nightly.
The regression test from this PR is an example of code that would break.

r? @nrc

4 files changed, 66 insertions, 16 deletions

diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
index 0b382fcbfdd..e7d83a64e03 100644
--- a/src/librustc_resolve/lib.rs
+++ b/src/librustc_resolve/lib.rs
@@ -1067,7 +1067,7 @@ pub struct Resolver<'a> {
 
     privacy_errors: Vec<PrivacyError<'a>>,
     ambiguity_errors: Vec<AmbiguityError<'a>>,
-    disallowed_shadowing: Vec<(Name, Span, LegacyScope<'a>)>,
+    disallowed_shadowing: Vec<&'a LegacyBinding<'a>>,
 
     arenas: &'a ResolverArenas<'a>,
     dummy_binding: &'a NameBinding<'a>,
@@ -1077,6 +1077,7 @@ pub struct Resolver<'a> {
     crate_loader: &'a mut CrateLoader,
     macro_names: FnvHashSet<Name>,
     builtin_macros: FnvHashMap<Name, Rc<SyntaxExtension>>,
+    lexical_macro_resolutions: Vec<(Name, LegacyScope<'a>)>,
 
     // Maps the `Mark` of an expansion to its containing module or block.
     invocations: FnvHashMap<Mark, &'a InvocationData<'a>>,
@@ -1267,6 +1268,7 @@ impl<'a> Resolver<'a> {
             crate_loader: crate_loader,
             macro_names: FnvHashSet(),
             builtin_macros: FnvHashMap(),
+            lexical_macro_resolutions: Vec::new(),
             invocations: invocations,
         }
     }
@@ -3363,12 +3365,16 @@ impl<'a> Resolver<'a> {
     }
 
     fn report_shadowing_errors(&mut self) {
+        for (name, scope) in replace(&mut self.lexical_macro_resolutions, Vec::new()) {
+            self.resolve_macro_name(scope, name);
+        }
+
         let mut reported_errors = FnvHashSet();
-        for (name, span, scope) in replace(&mut self.disallowed_shadowing, Vec::new()) {
-            if self.resolve_macro_name(scope, name, false).is_some() &&
-               reported_errors.insert((name, span)) {
-                let msg = format!("`{}` is already in scope", name);
-                self.session.struct_span_err(span, &msg)
+        for binding in replace(&mut self.disallowed_shadowing, Vec::new()) {
+            if self.resolve_macro_name(binding.parent, binding.name).is_some() &&
+               reported_errors.insert((binding.name, binding.span)) {
+                let msg = format!("`{}` is already in scope", binding.name);
+                self.session.struct_span_err(binding.span, &msg)
                     .note("macro-expanded `macro_rules!`s may not shadow \
                            existing macros (see RFC 1560)")
                     .emit();
diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs
index ed46c1d96ad..e3078a42f65 100644
--- a/src/librustc_resolve/macros.rs
+++ b/src/librustc_resolve/macros.rs
@@ -73,10 +73,10 @@ impl<'a> LegacyScope<'a> {
 }
 
 pub struct LegacyBinding<'a> {
-    parent: LegacyScope<'a>,
-    name: ast::Name,
+    pub parent: LegacyScope<'a>,
+    pub name: ast::Name,
     ext: Rc<SyntaxExtension>,
-    span: Span,
+    pub span: Span,
 }
 
 impl<'a> base::Resolver for Resolver<'a> {
@@ -171,7 +171,7 @@ impl<'a> base::Resolver for Resolver<'a> {
         if let LegacyScope::Expansion(parent) = invocation.legacy_scope.get() {
             invocation.legacy_scope.set(LegacyScope::simplify_expansion(parent));
         }
-        self.resolve_macro_name(invocation.legacy_scope.get(), name, true).ok_or_else(|| {
+        self.resolve_macro_name(invocation.legacy_scope.get(), name).ok_or_else(|| {
             if force {
                 let msg = format!("macro undefined: '{}!'", name);
                 let mut err = self.session.struct_span_err(path.span, &msg);
@@ -186,17 +186,18 @@ impl<'a> base::Resolver for Resolver<'a> {
 }
 
 impl<'a> Resolver<'a> {
-    pub fn resolve_macro_name(&mut self,
-                              mut scope: LegacyScope<'a>,
-                              name: ast::Name,
-                              record_used: bool)
+    pub fn resolve_macro_name(&mut self, mut scope: LegacyScope<'a>, name: ast::Name)
                               -> Option<Rc<SyntaxExtension>> {
+        let mut possible_time_travel = None;
         let mut relative_depth: u32 = 0;
         loop {
             scope = match scope {
                 LegacyScope::Empty => break,
                 LegacyScope::Expansion(invocation) => {
                     if let LegacyScope::Empty = invocation.expansion.get() {
+                        if possible_time_travel.is_none() {
+                            possible_time_travel = Some(scope);
+                        }
                         invocation.legacy_scope.get()
                     } else {
                         relative_depth += 1;
@@ -209,8 +210,11 @@ impl<'a> Resolver<'a> {
                 }
                 LegacyScope::Binding(binding) => {
                     if binding.name == name {
-                        if record_used && relative_depth > 0 {
-                            self.disallowed_shadowing.push((name, binding.span, binding.parent));
+                        if let Some(scope) = possible_time_travel {
+                            // Check for disallowed shadowing later
+                            self.lexical_macro_resolutions.push((name, scope));
+                        } else if relative_depth > 0 {
+                            self.disallowed_shadowing.push(binding);
                         }
                         return Some(binding.ext.clone());
                     }
@@ -219,6 +223,9 @@ impl<'a> Resolver<'a> {
             };
         }
 
+        if let Some(scope) = possible_time_travel {
+            self.lexical_macro_resolutions.push((name, scope));
+        }
         self.builtin_macros.get(&name).cloned()
     }
 
diff --git a/src/test/compile-fail/auxiliary/define_macro.rs b/src/test/compile-fail/auxiliary/define_macro.rs
new file mode 100644
index 00000000000..6b6b14a896b
--- /dev/null
+++ b/src/test/compile-fail/auxiliary/define_macro.rs
@@ -0,0 +1,16 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#[macro_export]
+macro_rules! define_macro {
+    ($i:ident) => {
+        macro_rules! $i { () => {} }
+    }
+}
diff --git a/src/test/compile-fail/out-of-order-shadowing.rs b/src/test/compile-fail/out-of-order-shadowing.rs
new file mode 100644
index 00000000000..1fafaf85112
--- /dev/null
+++ b/src/test/compile-fail/out-of-order-shadowing.rs
@@ -0,0 +1,21 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// aux-build:define_macro.rs
+// error-pattern: `bar` is already in scope
+
+macro_rules! bar { () => {} }
+define_macro!(bar);
+bar!();
+
+macro_rules! m { () => { #[macro_use] extern crate define_macro; } }
+m!();
+
+fn main() {}