Auto merge of rust-lang#40501 - jseyfried:shadow_builtin_macros, r=nrc

Allow `use` macro imports to shadow global macros

Terminology:
 - global scope: builtin macros, macros from the prelude, `#[macro_use]`, or `#![plugin(..)]`.
 - legacy scope: crate-local `macro_rules!`.
 - modern scope: `use` macro imports, `macro` (once implemented).

Today, the legacy scope can shadow the global scope (modulo RFC 1560 expanded shadowing restrictions). However, the modern scope cannot shadow or be shadowed by either the global or legacy scopes, leading to ambiguity errors.

This PR allows the modern scope to shadow the global scope (subject to some restrictions).
More specifically, a name in the global scope is as shadowable as a glob import in the module `self`. In other words, we imagine a special, implicit glob import in each module item:
```rust
mod foo {
    #[lexical_only] // Not accessible via `foo::<name>`, like pre-RFC 1560 `use` imports.
    #[shadowable_by_legacy_scope] // for back-compat
    use <global_macros>::*;
}
```

r? @nrc

Author: bors

src/librustc_resolve/build_reduced_graph.rs

@@ -539,7 +539,7 @@ impl<'a> Resolver<'a> {
                            binding: &'a NameBinding<'a>,
                            span: Span,
                            allow_shadowing: bool) {
-        if self.builtin_macros.insert(name, binding).is_some() && !allow_shadowing {
+        if self.global_macros.insert(name, binding).is_some() && !allow_shadowing {
             let msg = format!("`{}` is already in scope", name);
             let note =
                 "macro-expanded `#[macro_use]`s may not shadow existing macros (see RFC 1560)";

src/librustc_resolve/lib.rs

@@ -75,7 +75,7 @@ use std::mem::replace;
 use std::rc::Rc;
 
 use resolve_imports::{ImportDirective, ImportDirectiveSubclass, NameResolution, ImportResolver};
-use macros::{InvocationData, LegacyBinding, LegacyScope};
+use macros::{InvocationData, LegacyBinding, LegacyScope, MacroBinding};
 
 // NB: This module needs to be declared first so diagnostics are
 // registered before they are used.
@@ -1174,7 +1174,7 @@ pub struct Resolver<'a> {
 
     crate_loader: &'a mut CrateLoader,
     macro_names: FxHashSet<Name>,
-    builtin_macros: FxHashMap<Name, &'a NameBinding<'a>>,
+    global_macros: FxHashMap<Name, &'a NameBinding<'a>>,
     lexical_macro_resolutions: Vec<(Name, &'a Cell<LegacyScope<'a>>)>,
     macro_map: FxHashMap<DefId, Rc<SyntaxExtension>>,
     macro_defs: FxHashMap<Mark, DefId>,
@@ -1372,7 +1372,7 @@ impl<'a> Resolver<'a> {
 
             crate_loader: crate_loader,
             macro_names: FxHashSet(),
-            builtin_macros: FxHashMap(),
+            global_macros: FxHashMap(),
             lexical_macro_resolutions: Vec::new(),
             macro_map: FxHashMap(),
             macro_exports: Vec::new(),
@@ -2429,9 +2429,9 @@ impl<'a> Resolver<'a> {
                 };
             }
         }
-        let is_builtin = self.builtin_macros.get(&path[0].name).cloned()
+        let is_global = self.global_macros.get(&path[0].name).cloned()
             .map(|binding| binding.get_macro(self).kind() == MacroKind::Bang).unwrap_or(false);
-        if primary_ns != MacroNS && (is_builtin || self.macro_names.contains(&path[0].name)) {
+        if primary_ns != MacroNS && (is_global || self.macro_names.contains(&path[0].name)) {
             // Return some dummy definition, it's enough for error reporting.
             return Some(
                 PathResolution::new(Def::Macro(DefId::local(CRATE_DEF_INDEX), MacroKind::Bang))
@@ -2566,6 +2566,7 @@ impl<'a> Resolver<'a> {
                 self.resolve_ident_in_module(module, ident, ns, false, record_used)
             } else if opt_ns == Some(MacroNS) {
                 self.resolve_lexical_macro_path_segment(ident, ns, record_used)
+                    .map(MacroBinding::binding)
             } else {
                 match self.resolve_ident_in_lexical_scope(ident, ns, record_used) {
                     Some(LexicalScopeBinding::Item(binding)) => Ok(binding),
@@ -3223,7 +3224,7 @@ impl<'a> Resolver<'a> {
             };
             let msg1 = format!("`{}` could refer to the name {} here", name, participle(b1));
             let msg2 = format!("`{}` could also refer to the name {} here", name, participle(b2));
-            let note = if !lexical && b1.is_glob_import() {
+            let note = if b1.expansion == Mark::root() || !lexical && b1.is_glob_import() {
                 format!("consider adding an explicit import of `{}` to disambiguate", name)
             } else if let Def::Macro(..) = b1.def() {
                 format!("macro-expanded {} do not shadow",
@@ -3243,11 +3244,15 @@ impl<'a> Resolver<'a> {
                 let msg = format!("`{}` is ambiguous", name);
                 self.session.add_lint(lint::builtin::LEGACY_IMPORTS, id, span, msg);
             } else {
-                self.session.struct_span_err(span, &format!("`{}` is ambiguous", name))
-                    .span_note(b1.span, &msg1)
-                    .span_note(b2.span, &msg2)
-                    .note(&note)
-                    .emit();
+                let mut err =
+                    self.session.struct_span_err(span, &format!("`{}` is ambiguous", name));
+                err.span_note(b1.span, &msg1);
+                match b2.def() {
+                    Def::Macro(..) if b2.span == DUMMY_SP =>
+                        err.note(&format!("`{}` is also a builtin macro", name)),
+                    _ => err.span_note(b2.span, &msg2),
+                };
+                err.note(&note).emit();
             }
         }
 
@@ -3361,22 +3366,21 @@ impl<'a> Resolver<'a> {
         if self.proc_macro_enabled { return; }
 
         for attr in attrs {
-            let name = unwrap_or!(attr.name(), continue);
-            let maybe_binding = self.builtin_macros.get(&name).cloned().or_else(|| {
-                let ident = Ident::with_empty_ctxt(name);
-                self.resolve_lexical_macro_path_segment(ident, MacroNS, None).ok()
-            });
-
-            if let Some(binding) = maybe_binding {
-                if let SyntaxExtension::AttrProcMacro(..) = *binding.get_macro(self) {
+            if attr.path.segments.len() > 1 {
+                continue
+            }
+            let ident = attr.path.segments[0].identifier;
+            let result = self.resolve_lexical_macro_path_segment(ident, MacroNS, None);
+            if let Ok(binding) = result {
+                if let SyntaxExtension::AttrProcMacro(..) = *binding.binding().get_macro(self) {
                     attr::mark_known(attr);
 
                     let msg = "attribute procedural macros are experimental";
                     let feature = "proc_macro";
 
                     feature_err(&self.session.parse_sess, feature,
                                 attr.span, GateIssue::Language, msg)
-                        .span_note(binding.span, "procedural macro imported here")
+                        .span_note(binding.span(), "procedural macro imported here")
                         .emit();
                 }
             }

src/librustc_resolve/macros.rs

@@ -81,11 +81,29 @@ pub struct LegacyBinding<'a> {
     pub span: Span,
 }
 
+#[derive(Copy, Clone)]
 pub enum MacroBinding<'a> {
     Legacy(&'a LegacyBinding<'a>),
+    Global(&'a NameBinding<'a>),
     Modern(&'a NameBinding<'a>),
 }
 
+impl<'a> MacroBinding<'a> {
+    pub fn span(self) -> Span {
+        match self {
+            MacroBinding::Legacy(binding) => binding.span,
+            MacroBinding::Global(binding) | MacroBinding::Modern(binding) => binding.span,
+        }
+    }
+
+    pub fn binding(self) -> &'a NameBinding<'a> {
+        match self {
+            MacroBinding::Global(binding) | MacroBinding::Modern(binding) => binding,
+            MacroBinding::Legacy(_) => panic!("unexpected MacroBinding::Legacy"),
+        }
+    }
+}
+
 impl<'a> base::Resolver for Resolver<'a> {
     fn next_node_id(&mut self) -> ast::NodeId {
         self.session.next_node_id()
@@ -171,7 +189,7 @@ impl<'a> base::Resolver for Resolver<'a> {
             vis: ty::Visibility::Invisible,
             expansion: Mark::root(),
         });
-        self.builtin_macros.insert(ident.name, binding);
+        self.global_macros.insert(ident.name, binding);
     }
 
     fn resolve_imports(&mut self) {
@@ -189,7 +207,7 @@ impl<'a> base::Resolver for Resolver<'a> {
                 attr::mark_known(&attrs[i]);
             }
 
-            match self.builtin_macros.get(&name).cloned() {
+            match self.global_macros.get(&name).cloned() {
                 Some(binding) => match *binding.get_macro(self) {
                     MultiModifier(..) | MultiDecorator(..) | SyntaxExtension::AttrProcMacro(..) => {
                         return Some(attrs.remove(i))
@@ -221,7 +239,7 @@ impl<'a> base::Resolver for Resolver<'a> {
                     }
                     let trait_name = traits[j].segments[0].identifier.name;
                     let legacy_name = Symbol::intern(&format!("derive_{}", trait_name));
-                    if !self.builtin_macros.contains_key(&legacy_name) {
+                    if !self.global_macros.contains_key(&legacy_name) {
                         continue
                     }
                     let span = traits.remove(j).span;
@@ -378,18 +396,18 @@ impl<'a> Resolver<'a> {
         }
 
         let name = path[0].name;
-        let result = match self.resolve_legacy_scope(&invocation.legacy_scope, name, false) {
-            Some(MacroBinding::Legacy(binding)) => Ok(Def::Macro(binding.def_id, MacroKind::Bang)),
-            Some(MacroBinding::Modern(binding)) => Ok(binding.def_ignoring_ambiguity()),
-            None => match self.resolve_lexical_macro_path_segment(path[0], MacroNS, None) {
-                Ok(binding) => Ok(binding.def_ignoring_ambiguity()),
-                Err(Determinacy::Undetermined) if !force =>
-                    return Err(Determinacy::Undetermined),
+        let legacy_resolution = self.resolve_legacy_scope(&invocation.legacy_scope, name, false);
+        let result = if let Some(MacroBinding::Legacy(binding)) = legacy_resolution {
+            Ok(Def::Macro(binding.def_id, MacroKind::Bang))
+        } else {
+            match self.resolve_lexical_macro_path_segment(path[0], MacroNS, None) {
+                Ok(binding) => Ok(binding.binding().def_ignoring_ambiguity()),
+                Err(Determinacy::Undetermined) if !force => return Err(Determinacy::Undetermined),
                 Err(_) => {
                     self.found_unresolved_macro = true;
                     Err(Determinacy::Determined)
                 }
-            },
+            }
         };
 
         self.current_module.legacy_macro_resolutions.borrow_mut()
@@ -403,42 +421,56 @@ impl<'a> Resolver<'a> {
                                               ident: Ident,
                                               ns: Namespace,
                                               record_used: Option<Span>)
-                                              -> Result<&'a NameBinding<'a>, Determinacy> {
-        let mut module = self.current_module;
-        let mut potential_expanded_shadower: Option<&NameBinding> = None;
+                                              -> Result<MacroBinding<'a>, Determinacy> {
+        let mut module = Some(self.current_module);
+        let mut potential_illegal_shadower = Err(Determinacy::Determined);
+        let determinacy =
+            if record_used.is_some() { Determinacy::Determined } else { Determinacy::Undetermined };
         loop {
-            // Since expanded macros may not shadow the lexical scope (enforced below),
-            // we can ignore unresolved invocations (indicated by the penultimate argument).
-            match self.resolve_ident_in_module(module, ident, ns, true, record_used) {
+            let result = if let Some(module) = module {
+                // Since expanded macros may not shadow the lexical scope and
+                // globs may not shadow global macros (both enforced below),
+                // we resolve with restricted shadowing (indicated by the penultimate argument).
+                self.resolve_ident_in_module(module, ident, ns, true, record_used)
+                    .map(MacroBinding::Modern)
+            } else {
+                self.global_macros.get(&ident.name).cloned().ok_or(determinacy)
+                    .map(MacroBinding::Global)
+            };
+
+            match result.map(MacroBinding::binding) {
                 Ok(binding) => {
                     let span = match record_used {
                         Some(span) => span,
-                        None => return Ok(binding),
+                        None => return result,
                     };
-                    match potential_expanded_shadower {
-                        Some(shadower) if shadower.def() != binding.def() => {
+                    if let Ok(MacroBinding::Modern(shadower)) = potential_illegal_shadower {
+                        if shadower.def() != binding.def() {
                             let name = ident.name;
                             self.ambiguity_errors.push(AmbiguityError {
                                 span: span, name: name, b1: shadower, b2: binding, lexical: true,
                                 legacy: false,
                             });
-                            return Ok(shadower);
+                            return potential_illegal_shadower;
                         }
-                        _ if binding.expansion == Mark::root() => return Ok(binding),
-                        _ => potential_expanded_shadower = Some(binding),
+                    }
+                    if binding.expansion != Mark::root() ||
+                       (binding.is_glob_import() && module.unwrap().def().is_some()) {
+                        potential_illegal_shadower = result;
+                    } else {
+                        return result;
                     }
                 },
                 Err(Determinacy::Undetermined) => return Err(Determinacy::Undetermined),
                 Err(Determinacy::Determined) => {}
             }
 
-            match module.kind {
-                ModuleKind::Block(..) => module = module.parent.unwrap(),
-                ModuleKind::Def(..) => return match potential_expanded_shadower {
-                    Some(binding) => Ok(binding),
-                    None if record_used.is_some() => Err(Determinacy::Determined),
-                    None => Err(Determinacy::Undetermined),
+            module = match module {
+                Some(module) => match module.kind {
+                    ModuleKind::Block(..) => module.parent,
+                    ModuleKind::Def(..) => None,
                 },
+                None => return potential_illegal_shadower,
             }
         }
     }
@@ -488,11 +520,11 @@ impl<'a> Resolver<'a> {
 
         let binding = if let Some(binding) = binding {
             MacroBinding::Legacy(binding)
-        } else if let Some(binding) = self.builtin_macros.get(&name).cloned() {
+        } else if let Some(binding) = self.global_macros.get(&name).cloned() {
             if !self.use_extern_macros {
                 self.record_use(Ident::with_empty_ctxt(name), MacroNS, binding, DUMMY_SP);
             }
-            MacroBinding::Modern(binding)
+            MacroBinding::Global(binding)
         } else {
             return None;
         };
@@ -524,21 +556,15 @@ impl<'a> Resolver<'a> {
             let legacy_resolution = self.resolve_legacy_scope(legacy_scope, ident.name, true);
             let resolution = self.resolve_lexical_macro_path_segment(ident, MacroNS, Some(span));
             match (legacy_resolution, resolution) {
-                (Some(legacy_resolution), Ok(resolution)) => {
-                    let (legacy_span, participle) = match legacy_resolution {
-                        MacroBinding::Modern(binding)
-                            if binding.def() == resolution.def() => continue,
-                        MacroBinding::Modern(binding) => (binding.span, "imported"),
-                        MacroBinding::Legacy(binding) => (binding.span, "defined"),
-                    };
-                    let msg1 = format!("`{}` could refer to the macro {} here", ident, participle);
+                (Some(MacroBinding::Legacy(legacy_binding)), Ok(MacroBinding::Modern(binding))) => {
+                    let msg1 = format!("`{}` could refer to the macro defined here", ident);
                     let msg2 = format!("`{}` could also refer to the macro imported here", ident);
                     self.session.struct_span_err(span, &format!("`{}` is ambiguous", ident))
-                        .span_note(legacy_span, &msg1)
-                        .span_note(resolution.span, &msg2)
+                        .span_note(legacy_binding.span, &msg1)
+                        .span_note(binding.span, &msg2)
                         .emit();
                 },
-                (Some(MacroBinding::Modern(binding)), Err(_)) => {
+                (Some(MacroBinding::Global(binding)), Ok(MacroBinding::Global(_))) => {
                     self.record_use(ident, MacroNS, binding, span);
                     self.err_if_macro_use_proc_macro(ident.name, span, binding);
                 },
@@ -567,11 +593,11 @@ impl<'a> Resolver<'a> {
             find_best_match_for_name(self.macro_names.iter(), name, None)
         } else {
             None
-        // Then check builtin macros.
+        // Then check global macros.
         }.or_else(|| {
             // FIXME: get_macro needs an &mut Resolver, can we do it without cloning?
-            let builtin_macros = self.builtin_macros.clone();
-            let names = builtin_macros.iter().filter_map(|(name, binding)| {
+            let global_macros = self.global_macros.clone();
+            let names = global_macros.iter().filter_map(|(name, binding)| {
                 if binding.get_macro(self).kind() == kind {
                     Some(name)
                 } else {

src/librustc_resolve/resolve_imports.rs

@@ -145,7 +145,7 @@ impl<'a> Resolver<'a> {
                                    module: Module<'a>,
                                    ident: Ident,
                                    ns: Namespace,
-                                   ignore_unresolved_invocations: bool,
+                                   restricted_shadowing: bool,
                                    record_used: Option<Span>)
                                    -> Result<&'a NameBinding<'a>, Determinacy> {
         self.populate_module_if_necessary(module);
@@ -158,9 +158,8 @@ impl<'a> Resolver<'a> {
             if let Some(binding) = resolution.binding {
                 if let Some(shadowed_glob) = resolution.shadows_glob {
                     let name = ident.name;
-                    // If we ignore unresolved invocations, we must forbid
-                    // expanded shadowing to avoid time travel.
-                    if ignore_unresolved_invocations &&
+                    // Forbid expanded shadowing to avoid time travel.
+                    if restricted_shadowing &&
                        binding.expansion != Mark::root() &&
                        ns != MacroNS && // In MacroNS, `try_define` always forbids this shadowing
                        binding.def() != shadowed_glob.def() {
@@ -215,7 +214,7 @@ impl<'a> Resolver<'a> {
         }
 
         let no_unresolved_invocations =
-            ignore_unresolved_invocations || module.unresolved_invocations.borrow().is_empty();
+            restricted_shadowing || module.unresolved_invocations.borrow().is_empty();
         match resolution.binding {
             // In `MacroNS`, expanded bindings do not shadow (enforced in `try_define`).
             Some(binding) if no_unresolved_invocations || ns == MacroNS =>
@@ -225,6 +224,9 @@ impl<'a> Resolver<'a> {
         }
 
         // Check if the globs are determined
+        if restricted_shadowing && module.def().is_some() {
+            return Err(Determined);
+        }
         for directive in module.globs.borrow().iter() {
             if self.is_accessible(directive.vis.get()) {
                 if let Some(module) = directive.imported_module.get() {