From a25c841e884517eb48b67c36326d31192aff82d9 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 4 Sep 2018 01:14:58 +0300 Subject: [PATCH 1/2] resolve: Split macro prelude into built-in and user-defined parts --- src/librustc_resolve/build_reduced_graph.rs | 2 +- src/librustc_resolve/lib.rs | 23 ++++++--- src/librustc_resolve/macros.rs | 56 +++++++++++---------- 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 63f643d7a2964..21c0f13baa4a9 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/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)"; diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 5cb615554ee68..98fbcd1fb183b 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1278,6 +1278,13 @@ impl<'a> NameBinding<'a> { } } + fn macro_kind(&self) -> Option { + 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,7 +1447,8 @@ pub struct Resolver<'a, 'b: 'a> { crate_loader: &'a mut CrateLoader<'b>, macro_names: FxHashSet, - macro_prelude: FxHashMap>, + builtin_macros: FxHashMap>, + macro_use_prelude: FxHashMap>, unshadowable_attrs: FxHashMap>, pub all_macros: FxHashMap, macro_map: FxHashMap>, @@ -1757,7 +1765,8 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> { crate_loader, macro_names: FxHashSet(), - macro_prelude: FxHashMap(), + builtin_macros: FxHashMap(), + macro_use_prelude: FxHashMap(), unshadowable_attrs: FxHashMap(), all_macros: FxHashMap(), macro_map: FxHashMap(), @@ -3340,10 +3349,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)) diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index 93874ee0e85d5..b4c772cb8c63f 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -214,7 +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); + 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 add_unshadowable_attr(&mut self, ident: ast::Ident, ext: Lrc) { @@ -249,7 +252,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 +288,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; @@ -585,14 +588,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 +614,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 +641,14 @@ 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) => Ok((binding, FromPrelude(true))), + 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 +716,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 +736,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; @@ -958,14 +967,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. From 62c7d78a9a39688e6445aefbd4fe1d051b7a9886 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 5 Sep 2018 22:43:11 +0300 Subject: [PATCH 2/2] resolve: Remove `unshadowable_attrs` --- src/librustc_resolve/lib.rs | 2 - src/librustc_resolve/macros.rs | 51 +++++++++---------- src/libsyntax/ext/base.rs | 2 - src/libsyntax_ext/lib.rs | 14 +---- src/test/ui/issues/issue-11692-2.rs | 3 +- src/test/ui/issues/issue-11692-2.stderr | 4 +- .../test-shadowing/test-cant-be-shadowed.rs | 5 ++ 7 files changed, 34 insertions(+), 47 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 98fbcd1fb183b..66f80af363227 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1449,7 +1449,6 @@ pub struct Resolver<'a, 'b: 'a> { macro_names: FxHashSet, builtin_macros: FxHashMap>, macro_use_prelude: FxHashMap>, - unshadowable_attrs: FxHashMap>, pub all_macros: FxHashMap, macro_map: FxHashMap>, macro_defs: FxHashMap, @@ -1767,7 +1766,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> { macro_names: FxHashSet(), builtin_macros: FxHashMap(), macro_use_prelude: FxHashMap(), - unshadowable_attrs: FxHashMap(), all_macros: FxHashMap(), macro_map: FxHashMap(), invocations, diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index b4c772cb8c63f..fe0cb523a1580 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -220,23 +220,6 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> { } } - fn add_unshadowable_attr(&mut self, ident: ast::Ident, ext: Lrc) { - 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); - } - fn resolve_imports(&mut self) { ImportResolver { resolver: self }.resolve_imports() } @@ -493,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()) @@ -643,7 +620,19 @@ impl<'a, 'cl> Resolver<'a, 'cl> { } WhereToResolve::MacroUsePrelude => { match self.macro_use_prelude.get(&ident.name).cloned() { - Some(binding) => Ok((binding, FromPrelude(true))), + 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), } } @@ -811,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. @@ -898,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 diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index 0e059bc4a6ce5..1ea710097661a 100644 --- a/src/libsyntax/ext/base.rs +++ b/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); - fn add_unshadowable_attr(&mut self, ident: ast::Ident, ext: Lrc); 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) {} - fn add_unshadowable_attr(&mut self, _ident: ast::Ident, _ext: Lrc) {} fn resolve_imports(&mut self) {} fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec, _allow_derive: bool) diff --git a/src/libsyntax_ext/lib.rs b/src/libsyntax_ext/lib.rs index e16f3b1ccb3f3..88af4a73a1515 100644 --- a/src/libsyntax_ext/lib.rs +++ b/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"), diff --git a/src/test/ui/issues/issue-11692-2.rs b/src/test/ui/issues/issue-11692-2.rs index 424cbd981c87d..2e94a27838e43 100644 --- a/src/test/ui/issues/issue-11692-2.rs +++ b/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 } diff --git a/src/test/ui/issues/issue-11692-2.stderr b/src/test/ui/issues/issue-11692-2.stderr index 51d6041e92220..186c59a61493d 100644 --- a/src/test/ui/issues/issue-11692-2.stderr +++ b/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 diff --git a/src/test/ui/test-shadowing/test-cant-be-shadowed.rs b/src/test/ui/test-shadowing/test-cant-be-shadowed.rs index 4b1a437818f87..4e24b17bdd587 100644 --- a/src/test/ui/test-shadowing/test-cant-be-shadowed.rs +++ b/src/test/ui/test-shadowing/test-cant-be-shadowed.rs @@ -16,3 +16,8 @@ #[test] fn foo(){} + +macro_rules! test { () => () } + +#[test] +fn bar() {}