Auto merge of #36767 - jseyfried:enforce_rfc_1560_shadowing, r=nrc

Enforce the shadowing restrictions from RFC 1560 for today's macros

This PR enforces a weakened version of the shadowing restrictions from RFC 1560. More specifically,
 - If a macro expansion contains a `macro_rules!` macro definition that is used outside of the expansion, the defined macro may not shadow an existing macro.
 - If a macro expansion contains a `#[macro_use] extern crate` macro import that is used outside of the expansion, the imported macro may not shadow an existing macro.

This is a [breaking-change]. For example,
```rust
macro_rules! m { () => {} }
macro_rules! n { () => {
    macro_rules! m { () => {} } //< This shadows an existing macro.
    m!(); //< This is inside the expansion that generated `m`'s definition, so it is OK.
} }
n!();
m!(); //< This use of `m` is outside the expansion, so it causes the shadowing to be an error.
```

r? @nrc

src/librustc/hir/map/def_collector.rs

@@ -17,6 +17,7 @@ use hir::def_id::{CRATE_DEF_INDEX, DefId, DefIndex};
 use middle::cstore::InlinedItem;
 
 use syntax::ast::*;
+use syntax::ext::hygiene::Mark;
 use syntax::visit;
 use syntax::parse::token::{self, keywords};
 
@@ -31,7 +32,7 @@ pub struct DefCollector<'a> {
 }
 
 pub struct MacroInvocationData {
-    pub id: NodeId,
+    pub mark: Mark,
     pub def_index: DefIndex,
     pub const_integer: bool,
 }
@@ -126,7 +127,7 @@ impl<'a> DefCollector<'a> {
     fn visit_macro_invoc(&mut self, id: NodeId, const_integer: bool) {
         if let Some(ref mut visit) = self.visit_macro_invoc {
             visit(MacroInvocationData {
-                id: id,
+                mark: Mark::from_placeholder_id(id),
                 const_integer: const_integer,
                 def_index: self.parent_def.unwrap(),
             })

src/librustc/middle/cstore.rs

@@ -423,7 +423,12 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {
     fn metadata_encoding_version(&self) -> &[u8] { bug!("metadata_encoding_version") }
 }
 
-pub enum LoadedMacro {
+pub struct LoadedMacro {
+    pub import_site: Span,
+    pub kind: LoadedMacroKind,
+}
+
+pub enum LoadedMacroKind {
     Def(ast::MacroDef),
     CustomDerive(String, Rc<MultiItemModifier>),
 }

src/librustc_metadata/macro_import.rs

@@ -18,7 +18,7 @@ use std::mem;
 use creader::{CrateLoader, Macros};
 
 use rustc::hir::def_id::DefIndex;
-use rustc::middle::cstore::LoadedMacro;
+use rustc::middle::cstore::{LoadedMacro, LoadedMacroKind};
 use rustc::session::Session;
 use rustc::util::nodemap::FnvHashMap;
 use rustc_back::dynamic_lib::DynamicLibrary;
@@ -28,14 +28,19 @@ use syntax::ast;
 use syntax::attr;
 use syntax::parse::token;
 use syntax_ext::deriving::custom::CustomDerive;
-use syntax_pos::Span;
+use syntax_pos::{Span, DUMMY_SP};
 
 pub fn call_bad_macro_reexport(a: &Session, b: Span) {
     span_err!(a, b, E0467, "bad macro reexport");
 }
 
 pub type MacroSelection = FnvHashMap<token::InternedString, Span>;
 
+enum ImportSelection {
+    All(Span),
+    Some(MacroSelection),
+}
+
 pub fn load_macros(loader: &mut CrateLoader, extern_crate: &ast::Item, allows_macros: bool)
                    -> Vec<LoadedMacro> {
     loader.load_crate(extern_crate, allows_macros)
@@ -46,7 +51,7 @@ impl<'a> CrateLoader<'a> {
                   extern_crate: &ast::Item,
                   allows_macros: bool) -> Vec<LoadedMacro> {
         // Parse the attributes relating to macros.
-        let mut import = Some(FnvHashMap());  // None => load all
+        let mut import = ImportSelection::Some(FnvHashMap());
         let mut reexport = FnvHashMap();
 
         for attr in &extern_crate.attrs {
@@ -55,11 +60,9 @@ impl<'a> CrateLoader<'a> {
                 "macro_use" => {
                     let names = attr.meta_item_list();
                     if names.is_none() {
-                        // no names => load all
-                        import = None;
-                    }
-                    if let (Some(sel), Some(names)) = (import.as_mut(), names) {
-                        for attr in names {
+                        import = ImportSelection::All(attr.span);
+                    } else if let ImportSelection::Some(ref mut sel) = import {
+                        for attr in names.unwrap() {
                             if let Some(word) = attr.word() {
                                 sel.insert(word.name().clone(), attr.span());
                             } else {
@@ -98,10 +101,10 @@ impl<'a> CrateLoader<'a> {
     fn load_macros<'b>(&mut self,
                        vi: &ast::Item,
                        allows_macros: bool,
-                       import: Option<MacroSelection>,
+                       import: ImportSelection,
                        reexport: MacroSelection)
                        -> Vec<LoadedMacro> {
-        if let Some(sel) = import.as_ref() {
+        if let ImportSelection::Some(ref sel) = import {
             if sel.is_empty() && reexport.is_empty() {
                 return Vec::new();
             }
@@ -120,15 +123,19 @@ impl<'a> CrateLoader<'a> {
         for mut def in macros.macro_rules.drain(..) {
             let name = def.ident.name.as_str();
 
-            def.use_locally = match import.as_ref() {
-                None => true,
-                Some(sel) => sel.contains_key(&name),
+            let import_site = match import {
+                ImportSelection::All(span) => Some(span),
+                ImportSelection::Some(ref sel) => sel.get(&name).cloned()
             };
+            def.use_locally = import_site.is_some();
             def.export = reexport.contains_key(&name);
             def.allow_internal_unstable = attr::contains_name(&def.attrs,
                                                               "allow_internal_unstable");
             debug!("load_macros: loaded: {:?}", def);
-            ret.push(LoadedMacro::Def(def));
+            ret.push(LoadedMacro {
+                kind: LoadedMacroKind::Def(def),
+                import_site: import_site.unwrap_or(DUMMY_SP),
+            });
             seen.insert(name);
         }
 
@@ -137,7 +144,7 @@ impl<'a> CrateLoader<'a> {
             // exported macros, enforced elsewhere
             assert_eq!(ret.len(), 0);
 
-            if import.is_some() {
+            if let ImportSelection::Some(..) = import {
                 self.sess.span_err(vi.span, "`rustc-macro` crates cannot be \
                                              selectively imported from, must \
                                              use `#[macro_use]`");
@@ -151,10 +158,10 @@ impl<'a> CrateLoader<'a> {
             self.load_derive_macros(vi.span, &macros, index, &mut ret);
         }
 
-        if let Some(sel) = import.as_ref() {
+        if let ImportSelection::Some(sel) = import {
             for (name, span) in sel {
                 if !seen.contains(&name) {
-                    span_err!(self.sess, *span, E0469,
+                    span_err!(self.sess, span, E0469,
                               "imported macro not found");
                 }
             }
@@ -199,18 +206,21 @@ impl<'a> CrateLoader<'a> {
             mem::transmute::<*mut u8, fn(&mut Registry)>(sym)
         };
 
-        struct MyRegistrar<'a>(&'a mut Vec<LoadedMacro>);
+        struct MyRegistrar<'a>(&'a mut Vec<LoadedMacro>, Span);
 
         impl<'a> Registry for MyRegistrar<'a> {
             fn register_custom_derive(&mut self,
                                       trait_name: &str,
                                       expand: fn(TokenStream) -> TokenStream) {
                 let derive = Rc::new(CustomDerive::new(expand));
-                self.0.push(LoadedMacro::CustomDerive(trait_name.to_string(), derive));
+                self.0.push(LoadedMacro {
+                    kind: LoadedMacroKind::CustomDerive(trait_name.to_string(), derive),
+                    import_site: self.1,
+                });
             }
         }
 
-        registrar(&mut MyRegistrar(ret));
+        registrar(&mut MyRegistrar(ret, span));
 
         // Intentionally leak the dynamic library. We can't ever unload it
         // since the library can make things that will live arbitrarily long.

src/librustc_resolve/build_reduced_graph.rs

@@ -13,14 +13,15 @@
 //! Here we build the "reduced graph": the graph of the module tree without
 //! any imports resolved.
 
+use macros;
 use resolve_imports::ImportDirectiveSubclass::{self, GlobImport};
 use {Module, ModuleS, ModuleKind};
 use Namespace::{self, TypeNS, ValueNS};
 use {NameBinding, NameBindingKind, ToNameBinding};
 use Resolver;
 use {resolve_error, resolve_struct_error, ResolutionError};
 
-use rustc::middle::cstore::LoadedMacro;
+use rustc::middle::cstore::LoadedMacroKind;
 use rustc::hir::def::*;
 use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId};
 use rustc::hir::map::DefPathData;
@@ -39,6 +40,7 @@ use syntax::ast::{Variant, ViewPathGlob, ViewPathList, ViewPathSimple};
 use syntax::ext::base::{MultiItemModifier, Resolver as SyntaxResolver};
 use syntax::ext::hygiene::Mark;
 use syntax::feature_gate::{self, emit_feature_err};
+use syntax::ext::tt::macro_rules;
 use syntax::parse::token::keywords;
 use syntax::visit::{self, Visitor};
 
@@ -77,7 +79,7 @@ impl<'b> Resolver<'b> {
     }
 
     /// Constructs the reduced graph for one item.
-    fn build_reduced_graph_for_item(&mut self, item: &Item) {
+    fn build_reduced_graph_for_item(&mut self, item: &Item, expansion: Mark) {
         let parent = self.current_module;
         let name = item.ident.name;
         let sp = item.span;
@@ -188,10 +190,29 @@ impl<'b> Resolver<'b> {
                 // We need to error on `#[macro_use] extern crate` when it isn't at the
                 // crate root, because `$crate` won't work properly.
                 let is_crate_root = self.current_module.parent.is_none();
-                for def in self.crate_loader.load_macros(item, is_crate_root) {
-                    match def {
-                        LoadedMacro::Def(def) => self.add_macro(Mark::root(), def),
-                        LoadedMacro::CustomDerive(name, ext) => {
+                for loaded_macro in self.crate_loader.load_macros(item, is_crate_root) {
+                    match loaded_macro.kind {
+                        LoadedMacroKind::Def(mut def) => {
+                            let name = def.ident.name;
+                            if def.use_locally {
+                                let ext = macro_rules::compile(&self.session.parse_sess, &def);
+                                let shadowing =
+                                    self.resolve_macro_name(Mark::root(), name, false).is_some();
+                                self.expansion_data[&Mark::root()].module.macros.borrow_mut()
+                                    .insert(name, macros::NameBinding {
+                                        ext: Rc::new(ext),
+                                        expansion: expansion,
+                                        shadowing: shadowing,
+                                        span: loaded_macro.import_site,
+                                    });
+                                self.macro_names.insert(name);
+                            }
+                            if def.export {
+                                def.id = self.next_node_id();
+                                self.exported_macros.push(def);
+                            }
+                        }
+                        LoadedMacroKind::CustomDerive(name, ext) => {
                             self.insert_custom_derive(&name, ext, item.span);
                         }
                     }
@@ -527,11 +548,12 @@ impl<'b> Resolver<'b> {
 
 pub struct BuildReducedGraphVisitor<'a, 'b: 'a> {
     pub resolver: &'a mut Resolver<'b>,
+    pub expansion: Mark,
 }
 
 impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
     fn visit_invoc(&mut self, id: ast::NodeId) {
-        self.resolver.expansion_data.get_mut(&id.as_u32()).unwrap().module =
+        self.resolver.expansion_data.get_mut(&Mark::from_placeholder_id(id)).unwrap().module =
             self.resolver.current_module;
     }
 }
@@ -562,7 +584,7 @@ impl<'a, 'b> Visitor for BuildReducedGraphVisitor<'a, 'b> {
         }
 
         let parent = self.resolver.current_module;
-        self.resolver.build_reduced_graph_for_item(item);
+        self.resolver.build_reduced_graph_for_item(item, self.expansion);
         visit::walk_item(self, item);
         self.resolver.current_module = parent;
     }

src/librustc_resolve/lib.rs

@@ -57,7 +57,6 @@ use syntax::ext::base::MultiItemModifier;
 use syntax::ext::hygiene::Mark;
 use syntax::ast::{self, FloatTy};
 use syntax::ast::{CRATE_NODE_ID, Name, NodeId, IntTy, UintTy};
-use syntax::ext::base::SyntaxExtension;
 use syntax::parse::token::{self, keywords};
 use syntax::util::lev_distance::find_best_match_for_name;
 
@@ -793,7 +792,7 @@ pub struct ModuleS<'a> {
     // `populate_module_if_necessary` call.
     populated: Cell<bool>,
 
-    macros: RefCell<FnvHashMap<Name, Rc<SyntaxExtension>>>,
+    macros: RefCell<FnvHashMap<Name, macros::NameBinding>>,
     macros_escape: bool,
 }
 
@@ -1074,6 +1073,7 @@ pub struct Resolver<'a> {
 
     privacy_errors: Vec<PrivacyError<'a>>,
     ambiguity_errors: Vec<AmbiguityError<'a>>,
+    macro_shadowing_errors: FnvHashSet<Span>,
 
     arenas: &'a ResolverArenas<'a>,
     dummy_binding: &'a NameBinding<'a>,
@@ -1085,7 +1085,7 @@ pub struct Resolver<'a> {
     macro_names: FnvHashSet<Name>,
 
     // Maps the `Mark` of an expansion to its containing module or block.
-    expansion_data: FnvHashMap<u32, macros::ExpansionData<'a>>,
+    expansion_data: FnvHashMap<Mark, macros::ExpansionData<'a>>,
 }
 
 pub struct ResolverArenas<'a> {
@@ -1203,7 +1203,7 @@ impl<'a> Resolver<'a> {
         DefCollector::new(&mut definitions).collect_root();
 
         let mut expansion_data = FnvHashMap();
-        expansion_data.insert(0, macros::ExpansionData::root(graph_root)); // Crate root expansion
+        expansion_data.insert(Mark::root(), macros::ExpansionData::root(graph_root));
 
         Resolver {
             session: session,
@@ -1249,6 +1249,7 @@ impl<'a> Resolver<'a> {
 
             privacy_errors: Vec::new(),
             ambiguity_errors: Vec::new(),
+            macro_shadowing_errors: FnvHashSet(),
 
             arenas: arenas,
             dummy_binding: arenas.alloc_name_binding(NameBinding {