From b7889ef23598524f310b8bb863271299bab628a6 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 11 Feb 2016 06:17:01 +0000 Subject: [PATCH 1/3] Report privacy errors at most once per import (fixes #31402) --- src/librustc_resolve/resolve_imports.rs | 34 ++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index f21ffb9b9a1..13b595e7234 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -403,6 +403,23 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { module_.increment_outstanding_references_for(target, TypeNS); } + match (&value_result, &type_result) { + (&Indeterminate, _) | (_, &Indeterminate) => return Indeterminate, + (&Failed(_), &Failed(_)) => { + let children = target_module.children.borrow(); + let names = children.keys().map(|&(ref name, _)| name); + let lev_suggestion = match find_best_match_for_name(names, &source.as_str(), None) { + Some(name) => format!(". Did you mean to use `{}`?", name), + None => "".to_owned(), + }; + let msg = format!("There is no `{}` in `{}`{}", + source, + module_to_string(target_module), lev_suggestion); + return Failed(Some((directive.span, msg))); + } + _ => (), + } + match (&value_result, &type_result) { (&Success(name_binding), _) if !name_binding.is_import() && directive.is_public && @@ -437,23 +454,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { _ => {} } - match (&value_result, &type_result) { - (&Indeterminate, _) | (_, &Indeterminate) => return Indeterminate, - (&Failed(_), &Failed(_)) => { - let children = target_module.children.borrow(); - let names = children.keys().map(|&(ref name, _)| name); - let lev_suggestion = match find_best_match_for_name(names, &source.as_str(), None) { - Some(name) => format!(". Did you mean to use `{}`?", name), - None => "".to_owned(), - }; - let msg = format!("There is no `{}` in `{}`{}", - source, - module_to_string(target_module), lev_suggestion); - return Failed(Some((directive.span, msg))); - } - _ => (), - } - for &(ns, result) in &[(ValueNS, &value_result), (TypeNS, &type_result)] { if let Success(binding) = *result { if !binding.defined_with(DefModifiers::IMPORTABLE) { From 4af85643b13f9eae3de6fe15f06825c4d197752d Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 14 Feb 2016 08:46:54 +0000 Subject: [PATCH 2/3] Rename Module field children to resolutions --- src/librustc_resolve/lib.rs | 14 +++++++------- src/librustc_resolve/resolve_imports.rs | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index f0e4d7578e3..82856b7a4c7 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -798,7 +798,7 @@ pub struct ModuleS<'a> { is_public: bool, is_extern_crate: bool, - children: RefCell>>, + resolutions: RefCell>>, imports: RefCell>, // The anonymous children of this node. Anonymous children are pseudo- @@ -846,7 +846,7 @@ impl<'a> ModuleS<'a> { def: def, is_public: is_public, is_extern_crate: false, - children: RefCell::new(HashMap::new()), + resolutions: RefCell::new(HashMap::new()), imports: RefCell::new(Vec::new()), anonymous_children: RefCell::new(NodeMap()), shadowed_traits: RefCell::new(Vec::new()), @@ -863,7 +863,7 @@ impl<'a> ModuleS<'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) + self.resolutions.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) } @@ -873,7 +873,7 @@ impl<'a> ModuleS<'a> { // 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>> { - let mut children = self.children.borrow_mut(); + let mut children = self.resolutions.borrow_mut(); let resolution = children.entry((name, ns)).or_insert_with(Default::default); // FIXME #31379: We can use methods from imported traits shadowed by non-import items @@ -889,19 +889,19 @@ impl<'a> ModuleS<'a> { } fn increment_outstanding_references_for(&self, name: Name, ns: Namespace) { - let mut children = self.children.borrow_mut(); + let mut children = self.resolutions.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.children.borrow_mut().get_mut(&(name, ns)).unwrap().outstanding_references { + match self.resolutions.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)>(&self, mut f: F) { - for (&(name, ns), name_resolution) in self.children.borrow().iter() { + for (&(name, ns), name_resolution) in self.resolutions.borrow().iter() { name_resolution.binding.map(|binding| f(name, ns, binding)); } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 13b595e7234..1ad9eaeba1e 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -406,7 +406,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match (&value_result, &type_result) { (&Indeterminate, _) | (_, &Indeterminate) => return Indeterminate, (&Failed(_), &Failed(_)) => { - let children = target_module.children.borrow(); + let children = target_module.resolutions.borrow(); let names = children.keys().map(|&(ref name, _)| name); let lev_suggestion = match find_best_match_for_name(names, &source.as_str(), None) { Some(name) => format!(". Did you mean to use `{}`?", name), From 81d5d02c3733772f80fc23d834189bde248d68d7 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 13 Feb 2016 23:39:51 +0000 Subject: [PATCH 3/3] Rename Module field anonymous_children to module_children, expand it to include both named an anonymous modules, and fix #31644 --- src/librustc_resolve/build_reduced_graph.rs | 3 +- src/librustc_resolve/lib.rs | 61 ++++--------------- src/librustc_resolve/resolve_imports.rs | 14 +---- .../enum-and-module-in-same-scope.rs | 11 ++-- 4 files changed, 20 insertions(+), 69 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 775ed34ab07..5c94c6e4369 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -306,6 +306,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let def = Def::Mod(self.ast_map.local_def_id(item.id)); let module = self.new_module(parent_link, Some(def), false, is_public); self.define(parent, name, TypeNS, (module, sp)); + parent.module_children.borrow_mut().insert(item.id, module); module } @@ -474,7 +475,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let parent_link = BlockParentLink(parent, block_id); let new_module = self.new_module(parent_link, None, false, false); - parent.anonymous_children.borrow_mut().insert(block_id, new_module); + parent.module_children.borrow_mut().insert(block_id, new_module); new_module } else { parent diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 82856b7a4c7..3244d2f1d96 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -801,9 +801,9 @@ pub struct ModuleS<'a> { resolutions: RefCell>>, imports: RefCell>, - // The anonymous children of this node. Anonymous children are pseudo- - // modules that are implicitly created around items contained within - // blocks. + // The module children of this node, including normal modules and anonymous modules. + // Anonymous children are pseudo-modules that are implicitly created around items + // contained within blocks. // // For example, if we have this: // @@ -815,7 +815,7 @@ pub struct ModuleS<'a> { // // There will be an anonymous module created around `g` with the ID of the // entry block for `f`. - anonymous_children: RefCell>>, + module_children: RefCell>>, shadowed_traits: RefCell>>, @@ -848,7 +848,7 @@ impl<'a> ModuleS<'a> { is_extern_crate: false, resolutions: RefCell::new(HashMap::new()), imports: RefCell::new(Vec::new()), - anonymous_children: RefCell::new(NodeMap()), + module_children: RefCell::new(NodeMap()), shadowed_traits: RefCell::new(Vec::new()), glob_count: Cell::new(0), pub_count: Cell::new(0), @@ -906,14 +906,6 @@ impl<'a> ModuleS<'a> { } } - fn for_each_local_child)>(&self, mut f: F) { - 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 { self.def.as_ref().map(Def::def_id) } @@ -1640,20 +1632,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } // Descend into children and anonymous children. - build_reduced_graph::populate_module_if_necessary(self, module_); - - module_.for_each_local_child(|_, _, child_node| { - match child_node.module() { - None => { - // Continue. - } - Some(child_module) => { - self.report_unresolved_imports(child_module); - } - } - }); - - for (_, module_) in module_.anonymous_children.borrow().iter() { + for (_, module_) in module_.module_children.borrow().iter() { self.report_unresolved_imports(module_); } } @@ -1676,32 +1655,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // generate a fake "implementation scope" containing all the // implementations thus found, for compatibility with old resolve pass. - fn with_scope(&mut self, name: Option, f: F) + fn with_scope(&mut self, id: NodeId, f: F) where F: FnOnce(&mut Resolver) { let orig_module = self.current_module; // Move down in the graph. - match name { - None => { - // Nothing to do. - } - Some(name) => { - build_reduced_graph::populate_module_if_necessary(self, &orig_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; - } - } - } - } + if let Some(module) = orig_module.module_children.borrow().get(&id) { + self.current_module = module; } f(self); @@ -1825,7 +1786,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } ItemMod(_) | ItemForeignMod(_) => { - self.with_scope(Some(name), |this| { + self.with_scope(item.id, |this| { intravisit::walk_item(this, item); }); } @@ -2261,7 +2222,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Move down in the graph, if there's an anonymous module rooted here. let orig_module = self.current_module; let anonymous_module = - orig_module.anonymous_children.borrow().get(&block.id).map(|module| *module); + orig_module.module_children.borrow().get(&block.id).map(|module| *module); if let Some(anonymous_module) = anonymous_module { debug!("(resolving block) found anonymous module, moving down"); diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 1ad9eaeba1e..fa9b54a91ea 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -243,19 +243,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { errors.extend(self.resolve_imports_for_module(module_)); self.resolver.current_module = orig_module; - build_reduced_graph::populate_module_if_necessary(self.resolver, module_); - module_.for_each_local_child(|_, _, child_node| { - match child_node.module() { - None => { - // Nothing to do. - } - Some(child_module) => { - errors.extend(self.resolve_imports_for_module_subtree(child_module)); - } - } - }); - - for (_, child_module) in module_.anonymous_children.borrow().iter() { + for (_, child_module) in module_.module_children.borrow().iter() { errors.extend(self.resolve_imports_for_module_subtree(child_module)); } diff --git a/src/test/compile-fail/enum-and-module-in-same-scope.rs b/src/test/compile-fail/enum-and-module-in-same-scope.rs index 67969616ca3..28e969b2149 100644 --- a/src/test/compile-fail/enum-and-module-in-same-scope.rs +++ b/src/test/compile-fail/enum-and-module-in-same-scope.rs @@ -8,12 +8,13 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -mod Foo { - pub static X: isize = 42; -} - -enum Foo { //~ ERROR duplicate definition of type or module `Foo` +enum Foo { X } +mod Foo { //~ ERROR duplicate definition of type or module `Foo` + pub static X: isize = 42; + fn f() { f() } // Check that this does not result in a resolution error +} + fn main() {}