Auto merge of #31317 - jseyfried:remove_external_module_children, r=nrc

This PR refactors away `Module`'s `external_module_children` and instead puts `extern crate` declarations in `children` like other items, simplifying duplicate checking and name resolution.

This PR also allows values to share a name with extern crates, which are only defined in the type namespace. Other than that, it is a pure refactoring.

r? @nrc

src/librustc_resolve/build_reduced_graph.rs

@@ -107,20 +107,37 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
     /// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined;
     /// otherwise, reports an error.
     fn define<T: ToNameBinding<'b>>(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) {
-        let name_binding = def.to_name_binding();
+        let binding = def.to_name_binding();
-        let span = name_binding.span.unwrap_or(DUMMY_SP);
+        let old_binding = match parent.try_define_child(name, ns, binding.clone()) {
-        self.check_for_conflicts_between_external_crates_and_items(&parent, name, span);
+            Some(old_binding) => old_binding,
-        if !parent.try_define_child(name, ns, name_binding) {
+            None => return,
+        };
+
+        let span = binding.span.unwrap_or(DUMMY_SP);
+        if !old_binding.is_extern_crate() && !binding.is_extern_crate() {
             // Record an error here by looking up the namespace that had the duplicate
             let ns_str = match ns { TypeNS => "type or module", ValueNS => "value" };
             let resolution_error = ResolutionError::DuplicateDefinition(ns_str, name);
             let mut err = resolve_struct_error(self, span, resolution_error);
 
-            if let Some(sp) = parent.children.borrow().get(&(name, ns)).unwrap().span {
+            if let Some(sp) = old_binding.span {
                 let note = format!("first definition of {} `{}` here", ns_str, name);
                 err.span_note(sp, &note);
             }
             err.emit();
+        } else if old_binding.is_extern_crate() && binding.is_extern_crate() {
+            span_err!(self.session,
+                      span,
+                      E0259,
+                      "an external crate named `{}` has already been imported into this module",
+                      name);
+        } else {
+            span_err!(self.session,
+                      span,
+                      E0260,
+                      "the name `{}` conflicts with an external crate \
+                      that has been imported into this module",
+                      name);
         }
     }
 
@@ -289,14 +306,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
                     self.external_exports.insert(def_id);
                     let parent_link = ModuleParentLink(parent, name);
                     let def = Def::Mod(def_id);
-                    let external_module = self.new_module(parent_link, Some(def), false, true);
+                    let external_module = self.new_extern_crate_module(parent_link, def);
+                    self.define(parent, name, TypeNS, (external_module, sp));
 
-                    debug!("(build reduced graph for item) found extern `{}`",
-                           module_to_string(&*external_module));
-                    self.check_for_conflicts_for_external_crate(parent, name, sp);
-                    parent.external_module_children
-                          .borrow_mut()
-                          .insert(name, external_module);
                     self.build_reduced_graph_for_external_crate(&external_module);
                 }
                 parent

src/librustc_resolve/lib.rs

@@ -120,8 +120,6 @@ enum SuggestionType {
 }
 
 pub enum ResolutionError<'a> {
-    /// error E0260: name conflicts with an extern crate
-    NameConflictsWithExternCrate(Name),
     /// error E0401: can't use type parameters from outer function
     TypeParametersFromOuterFunction,
     /// error E0402: cannot use an outer type parameter in this context
@@ -228,14 +226,6 @@ fn resolve_struct_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>,
     }
 
     match resolution_error {
-        ResolutionError::NameConflictsWithExternCrate(name) => {
-            struct_span_err!(resolver.session,
-                             span,
-                             E0260,
-                             "the name `{}` conflicts with an external crate \
-                             that has been imported into this module",
-                             name)
-        }
         ResolutionError::TypeParametersFromOuterFunction => {
             struct_span_err!(resolver.session,
                              span,
@@ -801,14 +791,11 @@ pub struct ModuleS<'a> {
     parent_link: ParentLink<'a>,
     def: Cell<Option<Def>>,
     is_public: bool,
+    is_extern_crate: bool,
 
     children: RefCell<HashMap<(Name, Namespace), NameBinding<'a>>>,
     imports: RefCell<Vec<ImportDirective>>,
 
-    // The external module children of this node that were declared with
-    // `extern crate`.
-    external_module_children: RefCell<HashMap<Name, Module<'a>>>,
-
     // The anonymous children of this node. Anonymous children are pseudo-
     // modules that are implicitly created around items contained within
     // blocks.
@@ -854,9 +841,9 @@ impl<'a> ModuleS<'a> {
             parent_link: parent_link,
             def: Cell::new(def),
             is_public: is_public,
+            is_extern_crate: false,
             children: RefCell::new(HashMap::new()),
             imports: RefCell::new(Vec::new()),
-            external_module_children: RefCell::new(HashMap::new()),
             anonymous_children: RefCell::new(NodeMap()),
             import_resolutions: RefCell::new(HashMap::new()),
             glob_count: Cell::new(0),
@@ -871,10 +858,21 @@ impl<'a> ModuleS<'a> {
         self.children.borrow().get(&(name, ns)).cloned()
     }
 
-    fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>) -> bool {
+    // If the name is not yet defined, define the name and return None.
+    // Otherwise, return the existing definition.
+    fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>)
+                        -> Option<NameBinding<'a>> {
         match self.children.borrow_mut().entry((name, ns)) {
-            hash_map::Entry::Vacant(entry) => { entry.insert(binding); true }
+            hash_map::Entry::Vacant(entry) => { entry.insert(binding); None }
-            hash_map::Entry::Occupied(_) => false,
+            hash_map::Entry::Occupied(entry) => { Some(entry.get().clone()) },
+        }
+    }
+
+    fn for_each_local_child<F: FnMut(Name, Namespace, &NameBinding<'a>)>(&self, mut f: F) {
+        for (&(name, ns), name_binding) in self.children.borrow().iter() {
+            if !name_binding.is_extern_crate() {
+                f(name, ns, name_binding)
+            }
         }
     }
 
@@ -1005,6 +1003,10 @@ impl<'a> NameBinding<'a> {
         let def = self.def().unwrap();
         (def, LastMod(if self.is_public() { AllPublic } else { DependsOn(def.def_id()) }))
     }
+
+    fn is_extern_crate(&self) -> bool {
+        self.module().map(|module| module.is_extern_crate).unwrap_or(false)
+    }
 }
 
 /// Interns the names of the primitive types.
@@ -1184,6 +1186,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         self.arenas.modules.alloc(ModuleS::new(parent_link, def, external, is_public))
     }
 
+    fn new_extern_crate_module(&self, parent_link: ParentLink<'a>, def: Def) -> Module<'a> {
+        let mut module = ModuleS::new(parent_link, Some(def), false, true);
+        module.is_extern_crate = true;
+        self.arenas.modules.alloc(module)
+    }
+
     fn get_ribs<'b>(&'b mut self, ns: Namespace) -> &'b mut Vec<Rib<'a>> {
         match ns { ValueNS => &mut self.value_ribs, TypeNS => &mut self.type_ribs }
     }
@@ -1211,32 +1219,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         }
     }
 
-    /// Check that an external crate doesn't collide with items or other external crates.
-    fn check_for_conflicts_for_external_crate(&self, module: Module<'a>, name: Name, span: Span) {
-        if module.external_module_children.borrow().contains_key(&name) {
-            span_err!(self.session,
-                      span,
-                      E0259,
-                      "an external crate named `{}` has already been imported into this module",
-                      name);
-        }
-        if let Some(name_binding) = module.get_child(name, TypeNS) {
-            resolve_error(self,
-                          name_binding.span.unwrap_or(codemap::DUMMY_SP),
-                          ResolutionError::NameConflictsWithExternCrate(name));
-        }
-    }
-
-    /// Checks that the names of items don't collide with external crates.
-    fn check_for_conflicts_between_external_crates_and_items(&self,
-                                                             module: Module<'a>,
-                                                             name: Name,
-                                                             span: Span) {
-        if module.external_module_children.borrow().contains_key(&name) {
-            resolve_error(self, span, ResolutionError::NameConflictsWithExternCrate(name));
-        }
-    }
-
     /// Resolves the given module path from the given root `module_`.
     fn resolve_module_path_from_root(&mut self,
                                      module_: Module<'a>,
@@ -1245,11 +1227,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                      span: Span,
                                      lp: LastPrivate)
                                      -> ResolveResult<(Module<'a>, LastPrivate)> {
-        fn search_parent_externals<'a>(needle: Name, module: Module<'a>)
+        fn search_parent_externals<'a>(needle: Name, module: Module<'a>) -> Option<Module<'a>> {
-                                       -> Option<Module<'a>> {
+            match module.get_child(needle, TypeNS) {
-            match module.external_module_children.borrow().get(&needle) {
+                Some(ref binding) if binding.is_extern_crate() => Some(module),
-                Some(_) => Some(module),
+                _ => match module.parent_link {
-                None => match module.parent_link {
                     ModuleParentLink(ref parent, _) => {
                         search_parent_externals(needle, parent)
                     }
@@ -1480,17 +1461,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             }
         }
 
-        // Search for external modules.
-        if namespace == TypeNS {
-            let children = module_.external_module_children.borrow();
-            if let Some(module) = children.get(&name) {
-                let name_binding = NameBinding::create_from_module(module, None);
-                debug!("lower name bindings succeeded");
-                return Success((Target::new(module_, name_binding, Shadowable::Never),
-                                false));
-            }
-        }
-
         // Finally, proceed up the scope chain looking for parent modules.
         let mut search_module = module_;
         loop {
@@ -1684,16 +1654,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             Some(..) | None => {} // Continue.
         }
 
-        // Finally, search through external children.
-        if namespace == TypeNS {
-            let children = module_.external_module_children.borrow();
-            if let Some(module) = children.get(&name) {
-                let name_binding = NameBinding::create_from_module(module, None);
-                return Success((Target::new(module_, name_binding, Shadowable::Never),
-                                false));
-            }
-        }
-
         // We're out of luck.
         debug!("(resolving name in module) failed to resolve `{}`", name);
         return Failed(None);
@@ -1712,7 +1672,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         // Descend into children and anonymous children.
         build_reduced_graph::populate_module_if_necessary(self, &module_);
 
-        for (_, child_node) in module_.children.borrow().iter() {
+        module_.for_each_local_child(|_, _, child_node| {
             match child_node.module() {
                 None => {
                     // Continue.
@@ -1721,7 +1681,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                     self.report_unresolved_imports(child_module);
                 }
             }
-        }
+        });
 
         for (_, module_) in module_.anonymous_children.borrow().iter() {
             self.report_unresolved_imports(module_);

src/librustc_resolve/resolve_imports.rs

@@ -213,7 +213,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         self.resolver.current_module = orig_module;
 
         build_reduced_graph::populate_module_if_necessary(self.resolver, &module_);
-        for (_, child_node) in module_.children.borrow().iter() {
+        module_.for_each_local_child(|_, _, child_node| {
             match child_node.module() {
                 None => {
                     // Nothing to do.
@@ -222,7 +222,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                     errors.extend(self.resolve_imports_for_module_subtree(child_module));
                 }
             }
-        }
+        });
 
         for (_, child_module) in module_.anonymous_children.borrow().iter() {
             errors.extend(self.resolve_imports_for_module_subtree(child_module));
@@ -386,18 +386,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                               -> (ResolveResult<(Module<'b>, NameBinding<'b>)>, bool) {
         build_reduced_graph::populate_module_if_necessary(self.resolver, module);
         if let Some(name_binding) = module.get_child(name, ns) {
-            return (Success((module, name_binding)), false);
+            if name_binding.is_extern_crate() {
-        }
-
-        if ns == TypeNS {
-            if let Some(extern_crate) = module.external_module_children.borrow().get(&name) {
                 // track the extern crate as used.
-                if let Some(DefId{ krate: kid, .. }) = extern_crate.def_id() {
+                if let Some(DefId { krate, .. }) = name_binding.module().unwrap().def_id() {
-                    self.resolver.used_crates.insert(kid);
+                    self.resolver.used_crates.insert(krate);
                 }
-                let name_binding = NameBinding::create_from_module(extern_crate, None);
-                return (Success((module, name_binding)), false);
             }
+            return (Success((module, name_binding)), false)
         }
 
         // If there is an unresolved glob at this point in the containing module, bail out.
@@ -725,13 +720,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         // Add all children from the containing module.
         build_reduced_graph::populate_module_if_necessary(self.resolver, &target_module);
 
-        for (&name, name_binding) in target_module.children.borrow().iter() {
+        target_module.for_each_local_child(|name, ns, name_binding| {
             self.merge_import_resolution(module_,
                                          target_module,
                                          import_directive,
-                                         name,
+                                         (name, ns),
                                          name_binding.clone());
-        }
+        });
 
         // Record the destination of this import
         if let Some(did) = target_module.def_id() {
@@ -878,21 +873,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                                                      import: &ImportResolution<'b>,
                                                      import_span: Span,
                                                      (name, ns): (Name, Namespace)) {
-        // First, check for conflicts between imports and `extern crate`s.
-        if ns == TypeNS {
-            if module.external_module_children.borrow().contains_key(&name) {
-                match import.target {
-                    Some(ref target) if target.shadowable != Shadowable::Always => {
-                        let msg = format!("import `{0}` conflicts with imported crate \
-                                           in this module (maybe you meant `use {0}::*`?)",
-                                          name);
-                        span_err!(self.resolver.session, import_span, E0254, "{}", &msg[..]);
-                    }
-                    Some(_) | None => {}
-                }
-            }
-        }
-
         // Check for item conflicts.
         let name_binding = match module.get_child(name, ns) {
             None => {
@@ -921,6 +901,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         } else {
             match import.target {
                 Some(ref target) if target.shadowable != Shadowable::Always => {
+                    if name_binding.is_extern_crate() {
+                        let msg = format!("import `{0}` conflicts with imported crate \
+                                           in this module (maybe you meant `use {0}::*`?)",
+                                          name);
+                        span_err!(self.resolver.session, import_span, E0254, "{}", &msg[..]);
+                        return;
+                    }
+
                     let (what, note) = match name_binding.module() {
                         Some(ref module) if module.is_normal() =>
                             ("existing submodule", "note conflicting module here"),

src/test/compile-fail/resolve-conflict-item-vs-extern-crate.rs

@@ -8,7 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-fn std() {}    //~ ERROR the name `std` conflicts with an external crate
+fn std() {}
+mod std {}    //~ ERROR the name `std` conflicts with an external crate
 
 fn main() {
 }