Auto merge of rust-lang#53936 - petrochenkov:2macpre, r=alexcrichton

resolve: Split macro prelude into built-in and user-defined parts

This is a refactoring that will help to remove `unshadowable_attrs` when rust-lang#53410 lands.

UPDATE: The second commit actually removes `unshadowable_attrs`.

Author: bors

src/librustc_resolve/build_reduced_graph.rs

@@ -833,7 +833,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                            binding: &'a NameBinding<'a>,
                            span: Span,
                            allow_shadowing: bool) {
-        if self.macro_prelude.insert(name, binding).is_some() && !allow_shadowing {
+        if self.macro_use_prelude.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

@@ -1278,6 +1278,13 @@ impl<'a> NameBinding<'a> {
         }
     }
 
+    fn macro_kind(&self) -> Option<MacroKind> {
+        match self.def_ignoring_ambiguity() {
+            Def::Macro(_, kind) => Some(kind),
+            _ => None,
+        }
+    }
+
     fn descr(&self) -> &'static str {
         if self.is_extern_crate() { "extern crate" } else { self.def().kind_name() }
     }
@@ -1440,8 +1447,8 @@ pub struct Resolver<'a, 'b: 'a> {
 
     crate_loader: &'a mut CrateLoader<'b>,
     macro_names: FxHashSet<Ident>,
-    macro_prelude: FxHashMap<Name, &'a NameBinding<'a>>,
-    unshadowable_attrs: FxHashMap<Name, &'a NameBinding<'a>>,
+    builtin_macros: FxHashMap<Name, &'a NameBinding<'a>>,
+    macro_use_prelude: FxHashMap<Name, &'a NameBinding<'a>>,
     pub all_macros: FxHashMap<Name, Def>,
     macro_map: FxHashMap<DefId, Lrc<SyntaxExtension>>,
     macro_defs: FxHashMap<Mark, DefId>,
@@ -1757,8 +1764,8 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
 
             crate_loader,
             macro_names: FxHashSet(),
-            macro_prelude: FxHashMap(),
-            unshadowable_attrs: FxHashMap(),
+            builtin_macros: FxHashMap(),
+            macro_use_prelude: FxHashMap(),
             all_macros: FxHashMap(),
             macro_map: FxHashMap(),
             invocations,
@@ -3340,10 +3347,12 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
                 };
             }
         }
-        let is_global = self.macro_prelude.get(&path[0].name).cloned()
-            .map(|binding| binding.get_macro(self).kind() == MacroKind::Bang).unwrap_or(false);
-        if primary_ns != MacroNS && (is_global ||
-                                     self.macro_names.contains(&path[0].modern())) {
+        if primary_ns != MacroNS &&
+           (self.macro_names.contains(&path[0].modern()) ||
+            self.builtin_macros.get(&path[0].name).cloned()
+                               .and_then(NameBinding::macro_kind) == Some(MacroKind::Bang) ||
+            self.macro_use_prelude.get(&path[0].name).cloned()
+                                  .and_then(NameBinding::macro_kind) == Some(MacroKind::Bang)) {
             // Return some dummy definition, it's enough for error reporting.
             return Some(
                 PathResolution::new(Def::Macro(DefId::local(CRATE_DEF_INDEX), MacroKind::Bang))

src/librustc_resolve/macros.rs

@@ -214,24 +214,10 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {
             vis: ty::Visibility::Invisible,
             expansion: Mark::root(),
         });
-        self.macro_prelude.insert(ident.name, binding);
-    }
-
-    fn add_unshadowable_attr(&mut self, ident: ast::Ident, ext: Lrc<SyntaxExtension>) {
-        let def_id = DefId {
-            krate: BUILTIN_MACROS_CRATE,
-            index: DefIndex::from_array_index(self.macro_map.len(),
-                                              DefIndexAddressSpace::Low),
-        };
-        let kind = ext.kind();
-        self.macro_map.insert(def_id, ext);
-        let binding = self.arenas.alloc_name_binding(NameBinding {
-            kind: NameBindingKind::Def(Def::Macro(def_id, kind), false),
-            span: DUMMY_SP,
-            vis: ty::Visibility::Invisible,
-            expansion: Mark::root(),
-        });
-        self.unshadowable_attrs.insert(ident.name, binding);
+        if self.builtin_macros.insert(ident.name, binding).is_some() {
+            self.session.span_err(ident.span,
+                                  &format!("built-in macro `{}` was already defined", ident));
+        }
     }
 
     fn resolve_imports(&mut self) {
@@ -249,7 +235,7 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {
                 attr::mark_known(&attrs[i]);
             }
 
-            match self.macro_prelude.get(&name).cloned() {
+            match self.builtin_macros.get(&name).cloned() {
                 Some(binding) => match *binding.get_macro(self) {
                     MultiModifier(..) | MultiDecorator(..) | SyntaxExtension::AttrProcMacro(..) => {
                         return Some(attrs.remove(i))
@@ -285,7 +271,7 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {
                     }
                     let trait_name = traits[j].segments[0].ident.name;
                     let legacy_name = Symbol::intern(&format!("derive_{}", trait_name));
-                    if !self.macro_prelude.contains_key(&legacy_name) {
+                    if !self.builtin_macros.contains_key(&legacy_name) {
                         continue
                     }
                     let span = traits.remove(j).span;
@@ -490,14 +476,8 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
             return def;
         }
 
-        if kind == MacroKind::Attr {
-            if let Some(ext) = self.unshadowable_attrs.get(&path[0].name) {
-                return Ok(ext.def());
-            }
-        }
-
         let legacy_resolution = self.resolve_legacy_scope(
-            path[0], invoc_id, invocation.parent_legacy_scope.get(), false
+            path[0], invoc_id, invocation.parent_legacy_scope.get(), false, kind == MacroKind::Attr
         );
         let result = if let Some(legacy_binding) = legacy_resolution {
             Ok(legacy_binding.def())
@@ -585,14 +565,12 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
         // (Macro NS)
         // 1. Names in modules (both normal `mod`ules and blocks), loop through hygienic parents
         //    (open, not controlled).
-        // 2. Macro prelude (language, standard library, user-defined legacy plugins lumped into
-        //    one set) (open, the open part is from macro expansions, not controlled).
+        // 2. `macro_use` prelude (open, the open part is from macro expansions, not controlled).
         // 2a. User-defined prelude from macro-use
         //    (open, the open part is from macro expansions, not controlled).
-        // 2b. Standard library prelude, currently just a macro-use (closed, controlled)
-        // 2c. Language prelude, perhaps including builtin attributes
-        //    (closed, controlled, except for legacy plugins).
-        // 3. Builtin attributes (closed, controlled).
+        // 2b. Standard library prelude is currently implemented as `macro-use` (closed, controlled)
+        // 3. Language prelude: builtin macros (closed, controlled, except for legacy plugins).
+        // 4. Language prelude: builtin attributes (closed, controlled).
 
         assert!(ns == TypeNS  || ns == MacroNS);
         assert!(force || !record_used); // `record_used` implies `force`
@@ -613,12 +591,13 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
 
         enum WhereToResolve<'a> {
             Module(Module<'a>),
-            MacroPrelude,
+            MacroUsePrelude,
+            BuiltinMacros,
             BuiltinAttrs,
             ExternPrelude,
             ToolPrelude,
             StdLibPrelude,
-            PrimitiveTypes,
+            BuiltinTypes,
         }
 
         // Go through all the scopes and try to resolve the name.
@@ -639,8 +618,26 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                     self.current_module = orig_current_module;
                     binding.map(|binding| (binding, FromPrelude(false)))
                 }
-                WhereToResolve::MacroPrelude => {
-                    match self.macro_prelude.get(&ident.name).cloned() {
+                WhereToResolve::MacroUsePrelude => {
+                    match self.macro_use_prelude.get(&ident.name).cloned() {
+                        Some(binding) => {
+                            let mut result = Ok((binding, FromPrelude(true)));
+                            // FIXME: Keep some built-in macros working even if they are
+                            // shadowed by non-attribute macros imported with `macro_use`.
+                            // We need to come up with some more principled approach instead.
+                            if is_attr && (ident.name == "test" || ident.name == "bench") {
+                                if let Def::Macro(_, MacroKind::Bang) =
+                                        binding.def_ignoring_ambiguity() {
+                                    result = Err(Determinacy::Determined);
+                                }
+                            }
+                            result
+                        }
+                        None => Err(Determinacy::Determined),
+                    }
+                }
+                WhereToResolve::BuiltinMacros => {
+                    match self.builtin_macros.get(&ident.name).cloned() {
                         Some(binding) => Ok((binding, FromPrelude(true))),
                         None => Err(Determinacy::Determined),
                     }
@@ -708,7 +705,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                     }
                     result
                 }
-                WhereToResolve::PrimitiveTypes => {
+                WhereToResolve::BuiltinTypes => {
                     if let Some(prim_ty) =
                             self.primitive_type_table.primitive_types.get(&ident.name).cloned() {
                         let binding = (Def::PrimTy(prim_ty), ty::Visibility::Public,
@@ -728,19 +725,20 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                             None => {
                                 use_prelude = !module.no_implicit_prelude;
                                 if ns == MacroNS {
-                                    WhereToResolve::MacroPrelude
+                                    WhereToResolve::MacroUsePrelude
                                 } else {
                                     WhereToResolve::ExternPrelude
                                 }
                             }
                         }
                     }
-                    WhereToResolve::MacroPrelude => WhereToResolve::BuiltinAttrs,
+                    WhereToResolve::MacroUsePrelude => WhereToResolve::BuiltinMacros,
+                    WhereToResolve::BuiltinMacros => WhereToResolve::BuiltinAttrs,
                     WhereToResolve::BuiltinAttrs => break, // nowhere else to search
                     WhereToResolve::ExternPrelude => WhereToResolve::ToolPrelude,
                     WhereToResolve::ToolPrelude => WhereToResolve::StdLibPrelude,
-                    WhereToResolve::StdLibPrelude => WhereToResolve::PrimitiveTypes,
-                    WhereToResolve::PrimitiveTypes => break, // nowhere else to search
+                    WhereToResolve::StdLibPrelude => WhereToResolve::BuiltinTypes,
+                    WhereToResolve::BuiltinTypes => break, // nowhere else to search
                 };
 
                 continue;
@@ -802,8 +800,16 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                             ident: Ident,
                             invoc_id: Mark,
                             invoc_parent_legacy_scope: LegacyScope<'a>,
-                            record_used: bool)
+                            record_used: bool,
+                            is_attr: bool)
                             -> Option<&'a NameBinding<'a>> {
+        if is_attr && (ident.name == "test" || ident.name == "bench") {
+            // FIXME: Keep some built-in macros working even if they are
+            // shadowed by user-defined `macro_rules`.
+            // We need to come up with some more principled approach instead.
+            return None;
+        }
+
         let ident = ident.modern();
 
         // This is *the* result, resolution from the scope closest to the resolved identifier.
@@ -889,7 +895,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
             let span = ident.span;
             let invocation = self.invocations[&invoc_id];
             let legacy_resolution = self.resolve_legacy_scope(
-                ident, invoc_id, invocation.parent_legacy_scope.get(), true
+                ident, invoc_id, invocation.parent_legacy_scope.get(), true, kind == MacroKind::Attr
             );
             let resolution = self.resolve_lexical_macro_path_segment(
                 ident, MacroNS, invoc_id, true, true, kind == MacroKind::Attr, span
@@ -958,14 +964,9 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
             None
         // Then check global macros.
         }.or_else(|| {
-            // FIXME: get_macro needs an &mut Resolver, can we do it without cloning?
-            let macro_prelude = self.macro_prelude.clone();
-            let names = macro_prelude.iter().filter_map(|(name, binding)| {
-                if binding.get_macro(self).kind() == kind {
-                    Some(name)
-                } else {
-                    None
-                }
+            let names = self.builtin_macros.iter().chain(self.macro_use_prelude.iter())
+                                                  .filter_map(|(name, binding)| {
+                if binding.macro_kind() == Some(kind) { Some(name) } else { None }
             });
             find_best_match_for_name(names, name, None)
         // Then check modules.

src/libsyntax/ext/base.rs

@@ -721,7 +721,6 @@ pub trait Resolver {
     fn visit_ast_fragment_with_placeholders(&mut self, mark: Mark, fragment: &AstFragment,
                                             derives: &[Mark]);
     fn add_builtin(&mut self, ident: ast::Ident, ext: Lrc<SyntaxExtension>);
-    fn add_unshadowable_attr(&mut self, ident: ast::Ident, ext: Lrc<SyntaxExtension>);
 
     fn resolve_imports(&mut self);
     // Resolves attribute and derive legacy macros from `#![plugin(..)]`.
@@ -761,7 +760,6 @@ impl Resolver for DummyResolver {
     fn visit_ast_fragment_with_placeholders(&mut self, _invoc: Mark, _fragment: &AstFragment,
                                             _derives: &[Mark]) {}
     fn add_builtin(&mut self, _ident: ast::Ident, _ext: Lrc<SyntaxExtension>) {}
-    fn add_unshadowable_attr(&mut self, _ident: ast::Ident, _ext: Lrc<SyntaxExtension>) {}
 
     fn resolve_imports(&mut self) {}
     fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>, _allow_derive: bool)

src/libsyntax_ext/lib.rs

@@ -72,18 +72,6 @@ pub fn register_builtins(resolver: &mut dyn syntax::ext::base::Resolver,
                          enable_quotes: bool) {
     deriving::register_builtin_derives(resolver);
 
-    {
-        let mut register_unshadowable = |name, ext| {
-            resolver.add_unshadowable_attr(ast::Ident::with_empty_ctxt(name), Lrc::new(ext));
-        };
-
-        register_unshadowable(Symbol::intern("test"),
-            MultiModifier(Box::new(test::expand_test)));
-
-        register_unshadowable(Symbol::intern("bench"),
-            MultiModifier(Box::new(test::expand_bench)));
-    }
-
     let mut register = |name, ext| {
         resolver.add_builtin(ast::Ident::with_empty_ctxt(name), Lrc::new(ext));
     };
@@ -147,6 +135,8 @@ pub fn register_builtins(resolver: &mut dyn syntax::ext::base::Resolver,
     }
 
     register(Symbol::intern("test_case"), MultiModifier(Box::new(test_case::expand)));
+    register(Symbol::intern("test"), MultiModifier(Box::new(test::expand_test)));
+    register(Symbol::intern("bench"), MultiModifier(Box::new(test::expand_bench)));
 
     // format_args uses `unstable` things internally.
     register(Symbol::intern("format_args"),

src/test/ui/issues/issue-11692-2.rs

@@ -9,6 +9,5 @@
 // except according to those terms.
 
 fn main() {
-    concat!(test!());
-    //~^ error: cannot find macro `test!` in this scope
+    concat!(test!()); //~ ERROR `test` can only be used in attributes
 }

src/test/ui/issues/issue-11692-2.stderr

@@ -1,7 +1,7 @@
-error: cannot find macro `test!` in this scope
+error: `test` can only be used in attributes
   --> $DIR/issue-11692-2.rs:12:13
    |
-LL |     concat!(test!());
+LL |     concat!(test!()); //~ ERROR `test` can only be used in attributes
    |             ^^^^
 
 error: aborting due to previous error

src/test/ui/test-shadowing/test-cant-be-shadowed.rs

@@ -16,3 +16,8 @@
 
 #[test]
 fn foo(){}
+
+macro_rules! test { () => () }
+
+#[test]
+fn bar() {}