Replace children and import_resolutions with a single NameResolution-valued map.

Refactor away resolve_name_in_module in resolve_imports.rs

Rewrite and improve the core name resolution procedure in NameResolution::result and Module::resolve_name

Refactor the duplicate checking code into NameResolution::try_define

Author: jseyfried

src/librustc_resolve/lib.rs

@@ -87,7 +87,7 @@ use rustc_front::hir::{TraitRef, Ty, TyBool, TyChar, TyFloat, TyInt};
 use rustc_front::hir::{TyRptr, TyStr, TyUint, TyPath, TyPtr};
 use rustc_front::util::walk_pat;
 
-use std::collections::{hash_map, HashMap, HashSet};
+use std::collections::{HashMap, HashSet};
 use std::cell::{Cell, RefCell};
 use std::fmt;
 use std::mem::replace;
@@ -342,11 +342,8 @@ fn resolve_struct_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>,
             if let Some(sp) = resolver.ast_map.span_if_local(did) {
                 err.span_note(sp, "constant defined here");
             }
-            if let Some(directive) = resolver.current_module
-                                             .import_resolutions
-                                             .borrow()
-                                             .get(&(name, ValueNS)) {
-                if let Some(binding) = directive.binding {
+            if let Success(binding) = resolver.current_module.resolve_name(name, ValueNS, true) {
+                if binding.is_import() {
                     err.span_note(binding.span.unwrap(), "constant imported here");
                 }
             }
@@ -653,10 +650,10 @@ impl<'a, 'v, 'tcx> Visitor<'v> for Resolver<'a, 'tcx> {
     }
 }
 
-type ErrorMessage = Option<(Span, String)>;
+pub type ErrorMessage = Option<(Span, String)>;
 
 #[derive(Clone, PartialEq, Eq)]
-enum ResolveResult<T> {
+pub enum ResolveResult<T> {
     Failed(ErrorMessage), // Failed to resolve the name, optional helpful error message.
     Indeterminate, // Couldn't determine due to unresolved globs.
     Success(T), // Successfully resolved the import.
@@ -802,7 +799,7 @@ pub struct ModuleS<'a> {
     is_public: bool,
     is_extern_crate: bool,
 
-    children: RefCell<HashMap<(Name, Namespace), &'a NameBinding<'a>>>,
+    children: RefCell<HashMap<(Name, Namespace), NameResolution<'a>>>,
     imports: RefCell<Vec<ImportDirective>>,
 
     // The anonymous children of this node. Anonymous children are pseudo-
@@ -821,9 +818,6 @@ pub struct ModuleS<'a> {
     // entry block for `f`.
     anonymous_children: RefCell<NodeMap<Module<'a>>>,
 
-    // The status of resolving each import in this module.
-    import_resolutions: RefCell<HashMap<(Name, Namespace), NameResolution<'a>>>,
-
     // The number of unresolved globs that this module exports.
     glob_count: Cell<usize>,
 
@@ -854,7 +848,6 @@ impl<'a> ModuleS<'a> {
             children: RefCell::new(HashMap::new()),
             imports: RefCell::new(Vec::new()),
             anonymous_children: RefCell::new(NodeMap()),
-            import_resolutions: RefCell::new(HashMap::new()),
             glob_count: Cell::new(0),
             pub_count: Cell::new(0),
             pub_glob_count: Cell::new(0),
@@ -863,39 +856,49 @@ impl<'a> ModuleS<'a> {
         }
     }
 
-    fn get_child(&self, name: Name, ns: Namespace) -> Option<&'a NameBinding<'a>> {
-        self.children.borrow().get(&(name, ns)).cloned()
+    fn resolve_name(&self, name: Name, ns: Namespace, allow_private_imports: bool)
+                    -> ResolveResult<&'a NameBinding<'a>> {
+        let glob_count =
+            if allow_private_imports { self.glob_count.get() } else { self.pub_glob_count.get() };
+
+        self.children.borrow().get(&(name, ns)).cloned().unwrap_or_default().result(glob_count)
+            .and_then(|binding| {
+                let allowed = allow_private_imports || !binding.is_import() || binding.is_public();
+                if allowed { Success(binding) } else { Failed(None) }
+            })
     }
 
-    // If the name is not yet defined, define the name and return None.
-    // Otherwise, return the existing definition.
+    // Define the name or return the existing binding if there is a collision.
     fn try_define_child(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>)
                         -> Result<(), &'a NameBinding<'a>> {
-        match self.children.borrow_mut().entry((name, ns)) {
-            hash_map::Entry::Vacant(entry) => { entry.insert(binding); Ok(()) }
-            hash_map::Entry::Occupied(entry) => { Err(entry.get()) },
-        }
+        self.children.borrow_mut().entry((name, ns)).or_insert_with(Default::default)
+                                                    .try_define(binding)
     }
 
     fn increment_outstanding_references_for(&self, name: Name, ns: Namespace) {
-        let mut resolutions = self.import_resolutions.borrow_mut();
-        resolutions.entry((name, ns)).or_insert_with(Default::default).outstanding_references += 1;
+        let mut children = self.children.borrow_mut();
+        children.entry((name, ns)).or_insert_with(Default::default).outstanding_references += 1;
     }
 
     fn decrement_outstanding_references_for(&self, name: Name, ns: Namespace) {
-        match self.import_resolutions.borrow_mut().get_mut(&(name, ns)).unwrap()
-                                                                       .outstanding_references {
+        match self.children.borrow_mut().get_mut(&(name, ns)).unwrap().outstanding_references {
             0 => panic!("No more outstanding references!"),
             ref mut outstanding_references => { *outstanding_references -= 1; }
         }
     }
 
+    fn for_each_child<F: FnMut(Name, Namespace, &'a NameBinding<'a>)>(&self, mut f: F) {
+        for (&(name, ns), name_resolution) in self.children.borrow().iter() {
+            name_resolution.binding.map(|binding| f(name, ns, binding));
+        }
+    }
+
     fn for_each_local_child<F: FnMut(Name, Namespace, &'a NameBinding<'a>)>(&self, mut f: F) {
-        for (&(name, ns), name_binding) in self.children.borrow().iter() {
-            if !name_binding.is_extern_crate() {
+        self.for_each_child(|name, ns, name_binding| {
+            if !name_binding.is_import() && !name_binding.is_extern_crate() {
                 f(name, ns, name_binding)
             }
-        }
+        })
     }
 
     fn def_id(&self) -> Option<DefId> {
@@ -1240,7 +1243,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
     }
 
     #[inline]
-    fn record_import_use(&mut self, name: Name, ns: Namespace, binding: &NameBinding<'a>) {
+    fn record_use(&mut self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) {
+        // track extern crates for unused_extern_crate lint
+        if let Some(DefId { krate, .. }) = binding.module().and_then(ModuleS::def_id) {
+            self.used_crates.insert(krate);
+        }
+
         let import_id = match binding.kind {
             NameBindingKind::Import { id, .. } => id,
             _ => return,
@@ -1278,8 +1286,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                      lp: LastPrivate)
                                      -> ResolveResult<(Module<'a>, LastPrivate)> {
         fn search_parent_externals<'a>(needle: Name, module: Module<'a>) -> Option<Module<'a>> {
-            match module.get_child(needle, TypeNS) {
-                Some(binding) if binding.is_extern_crate() => Some(module),
+            match module.resolve_name(needle, TypeNS, false) {
+                Success(binding) if binding.is_extern_crate() => Some(module),
                 _ => match module.parent_link {
                     ModuleParentLink(ref parent, _) => {
                         search_parent_externals(needle, parent)
@@ -1591,53 +1599,21 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
     /// given namespace. If successful, returns the binding corresponding to
     /// the name.
     fn resolve_name_in_module(&mut self,
-                              module_: Module<'a>,
+                              module: Module<'a>,
                               name: Name,
                               namespace: Namespace,
                               allow_private_imports: bool,
                               record_used: bool)
                               -> ResolveResult<&'a NameBinding<'a>> {
-        debug!("(resolving name in module) resolving `{}` in `{}`",
-               name,
-               module_to_string(&*module_));
-
-        // First, check the direct children of the module.
-        build_reduced_graph::populate_module_if_necessary(self, module_);
-
-        if let Some(binding) = module_.get_child(name, namespace) {
-            debug!("(resolving name in module) found node as child");
-            if binding.is_extern_crate() {
-                // track the extern crate as used.
-                if let Some(DefId { krate, .. }) = binding.module().unwrap().def_id() {
-                    self.used_crates.insert(krate);
-                }
-            }
-            return Success(binding);
-        }
-
-        // Check the list of resolved imports.
-        match module_.import_resolutions.borrow().get(&(name, namespace)) {
-            Some(import_resolution) => {
-                if let Some(binding) = import_resolution.binding {
-                    if !allow_private_imports && binding.is_public() { return Failed(None) }
-                    if binding.is_public() && import_resolution.outstanding_references != 0 {
-                        debug!("(resolving name in module) import unresolved; bailing out");
-                        return Indeterminate;
-                    }
+        debug!("(resolving name in module) resolving `{}` in `{}`", name, module_to_string(module));
 
-                    debug!("(resolving name in module) resolved to import");
-                    if record_used {
-                        self.record_import_use(name, namespace, binding);
-                    }
-                    return Success(binding);
-                }
+        build_reduced_graph::populate_module_if_necessary(self, module);
+        module.resolve_name(name, namespace, allow_private_imports).and_then(|binding| {
+            if record_used {
+                self.record_use(name, namespace, binding);
             }
-            None => {}
-        }
-
-        // We're out of luck.
-        debug!("(resolving name in module) failed to resolve `{}`", name);
-        return Failed(None);
+            Success(binding)
+        })
     }
 
     fn report_unresolved_imports(&mut self, module_: Module<'a>) {
@@ -1700,22 +1676,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             Some(name) => {
                 build_reduced_graph::populate_module_if_necessary(self, &orig_module);
 
-                match orig_module.get_child(name, TypeNS) {
-                    None => {
-                        debug!("!!! (with scope) didn't find `{}` in `{}`",
-                               name,
-                               module_to_string(&*orig_module));
-                    }
-                    Some(name_binding) => {
-                        match name_binding.module() {
-                            None => {
-                                debug!("!!! (with scope) didn't find module for `{}` in `{}`",
-                                       name,
-                                       module_to_string(&*orig_module));
-                            }
-                            Some(module_) => {
-                                self.current_module = module_;
-                            }
+                if let Success(name_binding) = orig_module.resolve_name(name, TypeNS, false) {
+                    match name_binding.module() {
+                        None => {
+                            debug!("!!! (with scope) didn't find module for `{}` in `{}`",
+                                   name,
+                                   module_to_string(orig_module));
+                        }
+                        Some(module) => {
+                            self.current_module = module;
                         }
                     }
                 }
@@ -3101,7 +3070,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             if name_path.len() == 1 {
                 match this.primitive_type_table.primitive_types.get(last_name) {
                     Some(_) => None,
-                    None => this.current_module.get_child(*last_name, TypeNS)
+                    None => this.current_module.resolve_name(*last_name, TypeNS, true).success()
                                                .and_then(NameBinding::module)
                 }
             } else {
@@ -3161,7 +3130,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
         // Look for a method in the current self type's impl module.
         if let Some(module) = get_module(self, path.span, &name_path) {
-            if let Some(binding) = module.get_child(name, ValueNS) {
+            if let Success(binding) = module.resolve_name(name, ValueNS, true) {
                 if let Some(Def::Method(did)) = binding.def() {
                     if is_static_method(self, did) {
                         return StaticMethod(path_names_to_string(&path, 0));
@@ -3484,34 +3453,18 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             // Look for trait children.
             build_reduced_graph::populate_module_if_necessary(self, &search_module);
 
-            for (&(_, ns), name_binding) in search_module.children.borrow().iter() {
-                if ns != TypeNS { continue }
+            search_module.for_each_child(|_, ns, name_binding| {
+                if ns != TypeNS { return }
                 let trait_def_id = match name_binding.def() {
                     Some(Def::Trait(trait_def_id)) => trait_def_id,
-                    Some(..) | None => continue,
+                    Some(..) | None => return,
                 };
                 if self.trait_item_map.contains_key(&(name, trait_def_id)) {
                     add_trait_info(&mut found_traits, trait_def_id, name);
+                    let trait_name = self.get_trait_name(trait_def_id);
+                    self.record_use(trait_name, TypeNS, name_binding);
                 }
-            }
-
-            // Look for imports.
-            for (&(_, ns), import) in search_module.import_resolutions.borrow().iter() {
-                if ns != TypeNS { continue }
-                let binding = match import.binding {
-                    Some(binding) => binding,
-                    None => continue,
-                };
-                let did = match binding.def() {
-                    Some(Def::Trait(trait_def_id)) => trait_def_id,
-                    Some(..) | None => continue,
-                };
-                if self.trait_item_map.contains_key(&(name, did)) {
-                    add_trait_info(&mut found_traits, did, name);
-                    let trait_name = self.get_trait_name(did);
-                    self.record_import_use(trait_name, TypeNS, binding);
-                }
-            }
+            });
 
             match search_module.parent_link {
                 NoParentLink | ModuleParentLink(..) => break,