From 48a435a90fb227f0da20b610438618dfc2c49a4e Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 22 Aug 2016 19:20:08 +0000 Subject: [PATCH 01/18] Fix test `compile-fail/task-rng-isnt-sendable.rs`. --- src/test/compile-fail/task-rng-isnt-sendable.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/compile-fail/task-rng-isnt-sendable.rs b/src/test/compile-fail/task-rng-isnt-sendable.rs index 9c0a2267d7cf8..c987d9f2f4e1b 100644 --- a/src/test/compile-fail/task-rng-isnt-sendable.rs +++ b/src/test/compile-fail/task-rng-isnt-sendable.rs @@ -10,10 +10,10 @@ // ensure that the ThreadRng isn't/doesn't become accidentally sendable. -use std::rand; //~ ERROR: module `rand` is private +use std::__rand::ThreadRng; fn test_send() {} pub fn main() { - test_send::(); + test_send::(); //~ ERROR std::marker::Send` is not satisfied } From 5dc1196191c2f1edba8baaf841fdc07f9f8eea0b Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 19 Aug 2016 21:46:28 +0000 Subject: [PATCH 02/18] Refactor away `binding.is_pseudo_public()`. --- src/librustc_resolve/lib.rs | 4 ---- src/librustc_resolve/resolve_imports.rs | 11 +++++------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 54efc4ae30a60..e27936b912960 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -885,10 +885,6 @@ impl<'a> NameBinding<'a> { } } - fn is_pseudo_public(&self) -> bool { - self.pseudo_vis() == ty::Visibility::Public - } - // We sometimes need to treat variants as `pub` for backwards compatibility fn pseudo_vis(&self) -> ty::Visibility { if self.is_variant() { ty::Visibility::Public } else { self.vis } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 8c6d89c29bde1..eedbccbb039ba 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -168,7 +168,7 @@ impl<'a> Resolver<'a> { }; let is_disallowed_private_import = |binding: &NameBinding| { - !allow_private_imports && !binding.is_pseudo_public() && binding.is_import() + !allow_private_imports && binding.vis != ty::Visibility::Public && binding.is_import() }; if let Some(span) = record_used { @@ -338,7 +338,7 @@ impl<'a> Resolver<'a> { }; // Define `new_binding` in `module`s glob importers. - if new_binding.is_importable() && new_binding.is_pseudo_public() { + if new_binding.vis == ty::Visibility::Public { for directive in module.glob_importers.borrow_mut().iter() { let imported_binding = self.import(new_binding, directive); let _ = self.try_define(directive.parent, name, ns, imported_binding); @@ -656,9 +656,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { if let Some(Def::Trait(_)) = module.def { self.session.span_err(directive.span, "items in traits are not importable."); - } - - if module.def_id() == directive.parent.def_id() { + return; + } else if module.def_id() == directive.parent.def_id() { return; } else if let GlobImport { is_prelude: true } = directive.subclass { self.prelude = Some(module); @@ -674,7 +673,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { resolution.borrow().binding().map(|binding| (*name, binding)) }).collect::>(); for ((name, ns), binding) in bindings { - if binding.is_importable() && binding.is_pseudo_public() { + if binding.pseudo_vis() == ty::Visibility::Public { let imported_binding = self.import(binding, directive); let _ = self.try_define(directive.parent, name, ns, imported_binding); } From 691d10c3c947761ea00df87bd1b1fd532da248e0 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 07:22:32 +0000 Subject: [PATCH 03/18] Rename `new_binding` -> `binding`. --- src/librustc_resolve/resolve_imports.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index eedbccbb039ba..81f99e2240b6f 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -324,7 +324,7 @@ impl<'a> Resolver<'a> { { // Ensure that `resolution` isn't borrowed when defining in the module's glob importers, // during which the resolution might end up getting re-defined via a glob cycle. - let (new_binding, t) = { + let (binding, t) = { let mut resolution = &mut *self.resolution(module, name, ns).borrow_mut(); let was_known = resolution.binding().is_some(); @@ -337,10 +337,10 @@ impl<'a> Resolver<'a> { } }; - // Define `new_binding` in `module`s glob importers. - if new_binding.vis == ty::Visibility::Public { + // Define `binding` in `module`s glob importers. + if binding.vis == ty::Visibility::Public { for directive in module.glob_importers.borrow_mut().iter() { - let imported_binding = self.import(new_binding, directive); + let imported_binding = self.import(binding, directive); let _ = self.try_define(directive.parent, name, ns, imported_binding); } } From 87ae68c1d6e55a62e1faf4ceccb5e884aa6a95d5 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 07:26:26 +0000 Subject: [PATCH 04/18] Refactor `binding.def()` to return a `Def` instead of an `Option`. --- src/librustc_resolve/lib.rs | 34 ++++++++++++------------- src/librustc_resolve/resolve_imports.rs | 8 +++--- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index e27936b912960..8a7a22988cb5f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -459,7 +459,7 @@ fn resolve_struct_error<'b, 'a: 'b, 'c>(resolver: &'b Resolver<'a>, err } ResolutionError::BindingShadowsSomethingUnacceptable(what_binding, name, binding) => { - let shadows_what = PathResolution::new(binding.def().unwrap()).kind_name(); + let shadows_what = PathResolution::new(binding.def()).kind_name(); let mut err = struct_span_err!(resolver.session, span, E0530, @@ -739,7 +739,7 @@ impl<'a> LexicalScopeBinding<'a> { fn local_def(self) -> LocalDef { match self { LexicalScopeBinding::LocalDef(local_def) => local_def, - LexicalScopeBinding::Item(binding) => LocalDef::from_def(binding.def().unwrap()), + LexicalScopeBinding::Item(binding) => LocalDef::from_def(binding.def()), } } @@ -877,10 +877,10 @@ impl<'a> NameBinding<'a> { } } - fn def(&self) -> Option { + fn def(&self) -> Def { match self.kind { - NameBindingKind::Def(def) => Some(def), - NameBindingKind::Module(module) => module.def, + NameBindingKind::Def(def) => def, + NameBindingKind::Module(module) => module.def.unwrap(), NameBindingKind::Import { binding, .. } => binding.def(), } } @@ -916,7 +916,7 @@ impl<'a> NameBinding<'a> { } fn is_importable(&self) -> bool { - match self.def().unwrap() { + match self.def() { Def::AssociatedConst(..) | Def::Method(..) | Def::AssociatedTy(..) => false, _ => true, } @@ -1097,7 +1097,7 @@ impl<'a> hir::lowering::Resolver for Resolver<'a> { fn resolve_generated_global_path(&mut self, path: &hir::Path, is_value: bool) -> Def { let namespace = if is_value { ValueNS } else { TypeNS }; match self.resolve_crate_relative_path(path.span, &path.segments, namespace) { - Ok(binding) => binding.def().unwrap(), + Ok(binding) => binding.def(), Err(true) => Def::Err, Err(false) => { let path_name = &format!("{}", path); @@ -1693,7 +1693,7 @@ impl<'a> Resolver<'a> { &prefix.segments, TypeNS) { Ok(binding) => { - let def = binding.def().unwrap(); + let def = binding.def(); self.record_def(item.id, PathResolution::new(def)); } Err(true) => self.record_def(item.id, err_path_resolution()), @@ -2309,7 +2309,7 @@ impl<'a> Resolver<'a> { // entity, then fall back to a fresh binding. let binding = self.resolve_ident_in_lexical_scope(ident.node, ValueNS, None) .and_then(LexicalScopeBinding::item); - let resolution = binding.and_then(NameBinding::def).and_then(|def| { + let resolution = binding.map(NameBinding::def).and_then(|def| { let always_binding = !pat_src.is_refutable() || opt_pat.is_some() || bmode != BindingMode::ByValue(Mutability::Immutable); match def { @@ -2443,7 +2443,7 @@ impl<'a> Resolver<'a> { if path.global { let binding = self.resolve_crate_relative_path(span, segments, namespace); - return binding.map(|binding| mk_res(binding.def().unwrap())); + return binding.map(|binding| mk_res(binding.def())); } // Try to find a path to an item in a module. @@ -2481,7 +2481,7 @@ impl<'a> Resolver<'a> { let unqualified_def = resolve_identifier_with_fallback(self, None); let qualified_binding = self.resolve_module_relative_path(span, segments, namespace); match (qualified_binding, unqualified_def) { - (Ok(binding), Some(ref ud)) if binding.def().unwrap() == ud.def => { + (Ok(binding), Some(ref ud)) if binding.def() == ud.def => { self.session .add_lint(lint::builtin::UNUSED_QUALIFICATIONS, id, @@ -2491,7 +2491,7 @@ impl<'a> Resolver<'a> { _ => {} } - qualified_binding.map(|binding| mk_res(binding.def().unwrap())) + qualified_binding.map(|binding| mk_res(binding.def())) } // Resolve a single identifier @@ -3114,7 +3114,7 @@ impl<'a> Resolver<'a> { let mut collected_traits = Vec::new(); module.for_each_child(|name, ns, binding| { if ns != TypeNS { return } - if let Some(Def::Trait(_)) = binding.def() { + if let Def::Trait(_) = binding.def() { collected_traits.push((name, binding)); } }); @@ -3122,7 +3122,7 @@ impl<'a> Resolver<'a> { } for &(trait_name, binding) in traits.as_ref().unwrap().iter() { - let trait_def_id = binding.def().unwrap().def_id(); + let trait_def_id = binding.def().def_id(); if this.trait_item_map.contains_key(&(name, trait_def_id)) { let mut import_id = None; if let NameBindingKind::Import { directive, .. } = binding.kind { @@ -3181,8 +3181,8 @@ impl<'a> Resolver<'a> { if name_binding.is_import() { return; } // collect results based on the filter function - if let Some(def) = name_binding.def() { - if name == lookup_name && ns == namespace && filter_fn(def) { + if name == lookup_name && ns == namespace { + if filter_fn(name_binding.def()) { // create the path let ident = ast::Ident::with_empty_ctxt(name); let params = PathParameters::none(); @@ -3302,7 +3302,7 @@ impl<'a> Resolver<'a> { let msg = format!("extern crate `{}` is private", name); self.session.add_lint(lint::builtin::INACCESSIBLE_EXTERN_CRATE, node_id, span, msg); } else { - let def = binding.def().unwrap(); + let def = binding.def(); self.session.span_err(span, &format!("{} `{}` is private", def.kind_name(), name)); } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 81f99e2240b6f..e00c736138d3b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -639,9 +639,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { // Record what this import resolves to for later uses in documentation, // this may resolve to either a value or a type, but for documentation // purposes it's good enough to just favor one over the other. - let def = match type_result.ok().and_then(NameBinding::def) { + let def = match type_result.ok().map(NameBinding::def) { Some(def) => def, - None => value_result.ok().and_then(NameBinding::def).unwrap(), + None => value_result.ok().map(NameBinding::def).unwrap(), }; let path_resolution = PathResolution::new(def); self.def_map.insert(directive.id, path_resolution); @@ -714,9 +714,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { if binding.vis == ty::Visibility::Public && (binding.is_import() || binding.is_extern_crate()) { - if let Some(def) = binding.def() { - reexports.push(Export { name: name, def_id: def.def_id() }); - } + reexports.push(Export { name: name, def_id: binding.def().def_id() }); } if let NameBindingKind::Import { binding: orig_binding, directive, .. } = binding.kind { From 1e4c8173e182d6254c7faafb3d1e1020eac194c8 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 07:59:47 +0000 Subject: [PATCH 05/18] Improve diagnostics and remove dead code. --- src/librustc_resolve/lib.rs | 46 +++++++++++------------------ src/test/compile-fail/bad-module.rs | 8 +++-- 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 8a7a22988cb5f..6cf53f877fb60 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1287,7 +1287,7 @@ impl<'a> Resolver<'a> { while index < module_path_len { let name = module_path[index]; match self.resolve_name_in_module(search_module, name, TypeNS, false, span) { - Failed(None) => { + Failed(_) => { let segment_name = name.as_str(); let module_name = module_to_string(search_module); let msg = if "???" == &module_name { @@ -1314,7 +1314,6 @@ impl<'a> Resolver<'a> { return Failed(span.map(|span| (span, msg))); } - Failed(err) => return Failed(err), Indeterminate => { debug!("(resolving module path for import) module resolution is \ indeterminate: {}", @@ -1383,7 +1382,11 @@ impl<'a> Resolver<'a> { let ident = ast::Ident::with_empty_ctxt(module_path[0]); match self.resolve_ident_in_lexical_scope(ident, TypeNS, span) .and_then(LexicalScopeBinding::module) { - None => return Failed(None), + None => { + let msg = + format!("Use of undeclared type or module `{}`", ident.name); + return Failed(span.map(|span| (span, msg))); + } Some(containing_module) => { search_module = containing_module; start_index = 1; @@ -2614,16 +2617,9 @@ impl<'a> Resolver<'a> { let containing_module; match self.resolve_module_path(&module_path, UseLexicalScope, Some(span)) { Failed(err) => { - let (span, msg) = match err { - Some((span, msg)) => (span, msg), - None => { - let msg = format!("Use of undeclared type or module `{}`", - names_to_string(&module_path)); - (span, msg) - } - }; - - resolve_error(self, span, ResolutionError::FailedToResolve(&msg)); + if let Some((span, msg)) = err { + resolve_error(self, span, ResolutionError::FailedToResolve(&msg)); + } return Err(true); } Indeterminate => return Err(false), @@ -2651,16 +2647,9 @@ impl<'a> Resolver<'a> { let containing_module; match self.resolve_module_path_from_root(root_module, &module_path, 0, Some(span)) { Failed(err) => { - let (span, msg) = match err { - Some((span, msg)) => (span, msg), - None => { - let msg = format!("Use of undeclared module `::{}`", - names_to_string(&module_path)); - (span, msg) - } - }; - - resolve_error(self, span, ResolutionError::FailedToResolve(&msg)); + if let Some((span, msg)) = err { + resolve_error(self, span, ResolutionError::FailedToResolve(&msg)); + } return Err(true); } @@ -3270,12 +3259,11 @@ impl<'a> Resolver<'a> { path_resolution = PathResolution::new(def); ty::Visibility::Restricted(self.definitions.as_local_node_id(def.def_id()).unwrap()) } - Failed(Some((span, msg))) => { - self.session.span_err(span, &format!("failed to resolve module path. {}", msg)); - ty::Visibility::Public - } - _ => { - self.session.span_err(path.span, "unresolved module path"); + Indeterminate => unreachable!(), + Failed(err) => { + if let Some((span, msg)) = err { + self.session.span_err(span, &format!("failed to resolve module path. {}", msg)); + } ty::Visibility::Public } }; diff --git a/src/test/compile-fail/bad-module.rs b/src/test/compile-fail/bad-module.rs index 0cd3a8853185f..6987d06ef12c3 100644 --- a/src/test/compile-fail/bad-module.rs +++ b/src/test/compile-fail/bad-module.rs @@ -8,6 +8,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// error-pattern: failed to resolve. Use of undeclared type or module `thing` +fn main() { + let foo = thing::len(Vec::new()); + //~^ ERROR failed to resolve. Use of undeclared type or module `thing` -fn main() { let foo = thing::len(Vec::new()); } + let foo = foo::bar::baz(); + //~^ ERROR failed to resolve. Use of undeclared type or module `foo` +} From 95528d1a9839066a29cc1cb50b097d5f84633148 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 22 Aug 2016 00:24:11 +0000 Subject: [PATCH 06/18] Refactor away `resolver.current_vis` and add `module.normal_ancestor_id`. --- src/librustc_resolve/build_reduced_graph.rs | 18 ++-- src/librustc_resolve/lib.rs | 106 +++++++------------- src/librustc_resolve/resolve_imports.rs | 12 +-- 3 files changed, 47 insertions(+), 89 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 71b00218e7cc1..83d35095f351b 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -30,7 +30,7 @@ use syntax::ast::Name; use syntax::attr; use syntax::parse::token; -use syntax::ast::{Block, Crate}; +use syntax::ast::{Block, Crate, DUMMY_NODE_ID}; use syntax::ast::{ForeignItem, ForeignItemKind, Item, ItemKind}; use syntax::ast::{Mutability, StmtKind, TraitItemKind}; use syntax::ast::{Variant, ViewPathGlob, ViewPathList, ViewPathSimple}; @@ -81,7 +81,6 @@ impl<'b> Resolver<'b> { /// Constructs the reduced graph for one item. fn build_reduced_graph_for_item(&mut self, item: &Item) { let parent = self.current_module; - let parent_vis = self.current_vis; let name = item.ident.name; let sp = item.span; let vis = self.resolve_visibility(&item.vis); @@ -204,7 +203,7 @@ impl<'b> Resolver<'b> { ItemKind::Mod(..) => { let parent_link = ModuleParentLink(parent, name); let def = Def::Mod(self.definitions.local_def_id(item.id)); - let module = self.new_module(parent_link, Some(def), false); + let module = self.new_module(parent_link, Some(def), item.id); module.no_implicit_prelude.set({ parent.no_implicit_prelude.get() || attr::contains_name(&item.attrs, "no_implicit_prelude") @@ -214,7 +213,6 @@ impl<'b> Resolver<'b> { // Descend into the module. self.current_module = module; - self.current_vis = ty::Visibility::Restricted(item.id); } ItemKind::ForeignMod(..) => {} @@ -243,7 +241,7 @@ impl<'b> Resolver<'b> { ItemKind::Enum(ref enum_definition, _) => { let parent_link = ModuleParentLink(parent, name); let def = Def::Enum(self.definitions.local_def_id(item.id)); - let module = self.new_module(parent_link, Some(def), false); + let module = self.new_module(parent_link, Some(def), parent.normal_ancestor_id); self.define(parent, name, TypeNS, (module, sp, vis)); for variant in &(*enum_definition).variants { @@ -285,7 +283,8 @@ impl<'b> Resolver<'b> { // Add all the items within to a new module. let parent_link = ModuleParentLink(parent, name); let def = Def::Trait(def_id); - let module_parent = self.new_module(parent_link, Some(def), false); + let module_parent = + self.new_module(parent_link, Some(def), parent.normal_ancestor_id); self.define(parent, name, TypeNS, (module_parent, sp, vis)); // Add the names of all the items to the trait info. @@ -312,7 +311,6 @@ impl<'b> Resolver<'b> { visit::walk_item(&mut BuildReducedGraphVisitor { resolver: self }, item); self.current_module = parent; - self.current_vis = parent_vis; } // Constructs the reduced graph for one variant. Variants exist in the @@ -363,7 +361,7 @@ impl<'b> Resolver<'b> { block_id); let parent_link = BlockParentLink(parent, block_id); - let new_module = self.new_module(parent_link, None, false); + let new_module = self.new_module(parent_link, None, parent.normal_ancestor_id); self.module_map.insert(block_id, new_module); self.current_module = new_module; // Descend into the block. } @@ -395,7 +393,7 @@ impl<'b> Resolver<'b> { debug!("(building reduced graph for external crate) building module {} {:?}", name, vis); let parent_link = ModuleParentLink(parent, name); - let module = self.new_module(parent_link, Some(def), true); + let module = self.new_module(parent_link, Some(def), DUMMY_NODE_ID); let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); } Def::Variant(_, variant_id) => { @@ -437,7 +435,7 @@ impl<'b> Resolver<'b> { } let parent_link = ModuleParentLink(parent, name); - let module = self.new_module(parent_link, Some(def), true); + let module = self.new_module(parent_link, Some(def), DUMMY_NODE_ID); let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); } Def::TyAlias(..) | Def::AssociatedTy(..) => { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 6cf53f877fb60..b17687e17575c 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -54,7 +54,7 @@ use rustc::util::nodemap::{NodeMap, NodeSet, FnvHashMap, FnvHashSet}; use syntax::ext::hygiene::Mark; use syntax::ast::{self, FloatTy}; -use syntax::ast::{CRATE_NODE_ID, Name, NodeId, CrateNum, IntTy, UintTy}; +use syntax::ast::{CRATE_NODE_ID, DUMMY_NODE_ID, Name, NodeId, CrateNum, IntTy, UintTy}; use syntax::parse::token::{self, keywords}; use syntax::util::lev_distance::find_best_match_for_name; @@ -768,6 +768,9 @@ pub struct ModuleS<'a> { parent_link: ParentLink<'a>, def: Option, + // The node id of the closest normal module (`mod`) ancestor (including this module). + normal_ancestor_id: NodeId, + // If the module is an extern crate, `def` is root of the external crate and `extern_crate_id` // is the NodeId of the local `extern crate` item (otherwise, `extern_crate_id` is None). extern_crate_id: Option, @@ -791,17 +794,18 @@ pub struct ModuleS<'a> { pub type Module<'a> = &'a ModuleS<'a>; impl<'a> ModuleS<'a> { - fn new(parent_link: ParentLink<'a>, def: Option, external: bool) -> Self { + fn new(parent_link: ParentLink<'a>, def: Option, normal_ancestor_id: NodeId) -> Self { ModuleS { parent_link: parent_link, def: def, + normal_ancestor_id: normal_ancestor_id, extern_crate_id: None, resolutions: RefCell::new(FnvHashMap()), no_implicit_prelude: Cell::new(false), glob_importers: RefCell::new(Vec::new()), globs: RefCell::new((Vec::new())), traits: RefCell::new(None), - populated: Cell::new(!external), + populated: Cell::new(normal_ancestor_id != DUMMY_NODE_ID), } } @@ -829,6 +833,13 @@ impl<'a> ModuleS<'a> { _ => false, } } + + fn parent(&self) -> Option<&'a Self> { + match self.parent_link { + ModuleParentLink(parent, _) | BlockParentLink(parent, _) => Some(parent), + NoParentLink => None, + } + } } impl<'a> fmt::Debug for ModuleS<'a> { @@ -983,10 +994,6 @@ pub struct Resolver<'a> { // The module that represents the current item scope. current_module: Module<'a>, - // The visibility of `pub(self)` items in the current scope. - // Equivalently, the visibility required for an item to be accessible from the current scope. - current_vis: ty::Visibility, - // The current set of local scopes, for values. // FIXME #4948: Reuse ribs to avoid allocation. value_ribs: Vec>, @@ -1079,15 +1086,12 @@ impl<'a> ResolverArenas<'a> { } impl<'a> ty::NodeIdTree for Resolver<'a> { - fn is_descendant_of(&self, node: NodeId, ancestor: NodeId) -> bool { - let ancestor = self.definitions.local_def_id(ancestor); - let mut module = *self.module_map.get(&node).unwrap(); - while module.def_id() != Some(ancestor) { - let module_parent = match self.get_nearest_normal_module_parent(module) { - Some(parent) => parent, + fn is_descendant_of(&self, mut node: NodeId, ancestor: NodeId) -> bool { + while node != ancestor { + node = match self.module_map[&node].parent() { + Some(parent) => parent.normal_ancestor_id, None => return false, - }; - module = module_parent; + } } true } @@ -1149,8 +1153,7 @@ impl<'a> Resolver<'a> { pub fn new(session: &'a Session, make_glob_map: MakeGlobMap, arenas: &'a ResolverArenas<'a>) -> Resolver<'a> { let root_def_id = DefId::local(CRATE_DEF_INDEX); - let graph_root = - ModuleS::new(NoParentLink, Some(Def::Mod(root_def_id)), false); + let graph_root = ModuleS::new(NoParentLink, Some(Def::Mod(root_def_id)), CRATE_NODE_ID); let graph_root = arenas.alloc_module(graph_root); let mut module_map = NodeMap(); module_map.insert(CRATE_NODE_ID, graph_root); @@ -1173,7 +1176,6 @@ impl<'a> Resolver<'a> { indeterminate_imports: Vec::new(), current_module: graph_root, - current_vis: ty::Visibility::Restricted(ast::CRATE_NODE_ID), value_ribs: vec![Rib::new(ModuleRibKind(graph_root))], type_ribs: vec![Rib::new(ModuleRibKind(graph_root))], label_ribs: Vec::new(), @@ -1217,21 +1219,20 @@ impl<'a> Resolver<'a> { /// Entry point to crate resolution. pub fn resolve_crate(&mut self, krate: &Crate) { self.current_module = self.graph_root; - self.current_vis = ty::Visibility::Restricted(ast::CRATE_NODE_ID); visit::walk_crate(self, krate); check_unused::check_crate(self, krate); self.report_privacy_errors(); } - fn new_module(&self, parent_link: ParentLink<'a>, def: Option, external: bool) + fn new_module(&self, parent_link: ParentLink<'a>, def: Option, normal_ancestor_id: NodeId) -> Module<'a> { - self.arenas.alloc_module(ModuleS::new(parent_link, def, external)) + self.arenas.alloc_module(ModuleS::new(parent_link, def, normal_ancestor_id)) } fn new_extern_crate_module(&self, parent_link: ParentLink<'a>, def: Def, local_node_id: NodeId) -> Module<'a> { - let mut module = ModuleS::new(parent_link, Some(def), false); + let mut module = ModuleS::new(parent_link, Some(def), local_node_id); module.extern_crate_id = Some(local_node_id); self.arenas.modules.alloc(module) } @@ -1473,35 +1474,6 @@ impl<'a> Resolver<'a> { None } - /// Returns the nearest normal module parent of the given module. - fn get_nearest_normal_module_parent(&self, mut module: Module<'a>) -> Option> { - loop { - match module.parent_link { - NoParentLink => return None, - ModuleParentLink(new_module, _) | - BlockParentLink(new_module, _) => { - let new_module = new_module; - if new_module.is_normal() { - return Some(new_module); - } - module = new_module; - } - } - } - } - - /// Returns the nearest normal module parent of the given module, or the - /// module itself if it is a normal module. - fn get_nearest_normal_module_parent_or_self(&self, module: Module<'a>) -> Module<'a> { - if module.is_normal() { - return module; - } - match self.get_nearest_normal_module_parent(module) { - None => module, - Some(new_module) => new_module, - } - } - /// Resolves a "module prefix". A module prefix is one or both of (a) `self::`; /// (b) some chain of `super::`. /// grammar: (SELF MOD_SEP ) ? (SUPER MOD_SEP) * @@ -1514,22 +1486,19 @@ impl<'a> Resolver<'a> { "super" => 0, _ => return Success(NoPrefixFound), }; - let mut containing_module = - self.get_nearest_normal_module_parent_or_self(self.current_module); + + let mut containing_module = self.module_map[&self.current_module.normal_ancestor_id]; // Now loop through all the `super`s we find. while i < module_path.len() && "super" == module_path[i].as_str() { debug!("(resolving module prefix) resolving `super` at {}", module_to_string(&containing_module)); - match self.get_nearest_normal_module_parent(containing_module) { - None => { - let msg = "There are too many initial `super`s.".into(); - return Failed(span.map(|span| (span, msg))); - } - Some(new_module) => { - containing_module = new_module; - i += 1; - } + if let Some(parent) = containing_module.parent() { + containing_module = self.module_map[&parent.normal_ancestor_id]; + i += 1; + } else { + let msg = "There are too many initial `super`s.".into(); + return Failed(span.map(|span| (span, msg))); } } @@ -1564,14 +1533,12 @@ impl<'a> Resolver<'a> { if let Some(module) = module { // Move down in the graph. let orig_module = replace(&mut self.current_module, module); - let orig_vis = replace(&mut self.current_vis, ty::Visibility::Restricted(id)); self.value_ribs.push(Rib::new(ModuleRibKind(module))); self.type_ribs.push(Rib::new(ModuleRibKind(module))); f(self); self.current_module = orig_module; - self.current_vis = orig_vis; self.value_ribs.pop(); self.type_ribs.pop(); } else { @@ -3248,16 +3215,17 @@ impl<'a> Resolver<'a> { ast::Visibility::Public => return ty::Visibility::Public, ast::Visibility::Crate(_) => return ty::Visibility::Restricted(ast::CRATE_NODE_ID), ast::Visibility::Restricted { ref path, id } => (path, id), - ast::Visibility::Inherited => return self.current_vis, + ast::Visibility::Inherited => { + return ty::Visibility::Restricted(self.current_module.normal_ancestor_id); + } }; let segments: Vec<_> = path.segments.iter().map(|seg| seg.identifier.name).collect(); let mut path_resolution = err_path_resolution(); let vis = match self.resolve_module_path(&segments, DontUseLexicalScope, Some(path.span)) { Success(module) => { - let def = module.def.unwrap(); - path_resolution = PathResolution::new(def); - ty::Visibility::Restricted(self.definitions.as_local_node_id(def.def_id()).unwrap()) + path_resolution = PathResolution::new(module.def.unwrap()); + ty::Visibility::Restricted(module.normal_ancestor_id) } Indeterminate => unreachable!(), Failed(err) => { @@ -3276,7 +3244,7 @@ impl<'a> Resolver<'a> { } fn is_accessible(&self, vis: ty::Visibility) -> bool { - vis.is_at_least(self.current_vis, self) + vis.is_accessible_from(self.current_module.normal_ancestor_id, self) } fn report_privacy_errors(&self) { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index e00c736138d3b..a0aab53c58f09 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -381,14 +381,6 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { // remain or unsuccessfully when no forward progress in resolving imports // is made. - fn set_current_module(&mut self, module: Module<'b>) { - self.current_module = module; - self.current_vis = ty::Visibility::Restricted({ - let normal_module = self.get_nearest_normal_module_parent_or_self(module); - self.definitions.as_local_node_id(normal_module.def_id().unwrap()).unwrap() - }); - } - /// Resolves all imports for the crate. This method performs the fixed- /// point iteration. fn resolve_imports(&mut self) { @@ -472,7 +464,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { names_to_string(&directive.module_path), module_to_string(self.current_module)); - self.set_current_module(directive.parent); + self.current_module = directive.parent; let module = if let Some(module) = directive.imported_module.get() { module @@ -548,7 +540,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } fn finalize_import(&mut self, directive: &'b ImportDirective<'b>) -> ResolveResult<()> { - self.set_current_module(directive.parent); + self.current_module = directive.parent; let ImportDirective { ref module_path, span, .. } = *directive; let module_result = self.resolve_module_path(&module_path, DontUseLexicalScope, Some(span)); From 513e955a1891d12819ba642331eb436d00861f3d Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 22 Aug 2016 04:05:49 +0000 Subject: [PATCH 07/18] Add field `dummy_binding` to `Resolver`. --- src/librustc_resolve/lib.rs | 8 +++++++- src/librustc_resolve/resolve_imports.rs | 9 ++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index b17687e17575c..124a748be326f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -65,7 +65,7 @@ use syntax::ast::{Item, ItemKind, ImplItem, ImplItemKind}; use syntax::ast::{Local, Mutability, Pat, PatKind, Path}; use syntax::ast::{PathSegment, PathParameters, QSelf, TraitItemKind, TraitRef, Ty, TyKind}; -use syntax_pos::Span; +use syntax_pos::{Span, DUMMY_SP}; use errors::DiagnosticBuilder; use std::cell::{Cell, RefCell}; @@ -1052,6 +1052,7 @@ pub struct Resolver<'a> { privacy_errors: Vec>, arenas: &'a ResolverArenas<'a>, + dummy_binding: &'a NameBinding<'a>, } pub struct ResolverArenas<'a> { @@ -1203,6 +1204,11 @@ impl<'a> Resolver<'a> { privacy_errors: Vec::new(), arenas: arenas, + dummy_binding: arenas.alloc_name_binding(NameBinding { + kind: NameBindingKind::Def(Def::Err), + span: DUMMY_SP, + vis: ty::Visibility::Public, + }), } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index a0aab53c58f09..f02e9b048dea7 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -27,7 +27,7 @@ use rustc::hir::def::*; use syntax::ast::{NodeId, Name}; use syntax::util::lev_distance::find_best_match_for_name; -use syntax_pos::{Span, DUMMY_SP}; +use syntax_pos::Span; use std::cell::{Cell, RefCell}; @@ -442,13 +442,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { // failed resolution fn import_dummy_binding(&mut self, directive: &'b ImportDirective<'b>) { if let SingleImport { target, .. } = directive.subclass { - let dummy_binding = self.arenas.alloc_name_binding(NameBinding { - kind: NameBindingKind::Def(Def::Err), - span: DUMMY_SP, - vis: ty::Visibility::Public, - }); + let dummy_binding = self.dummy_binding; let dummy_binding = self.import(dummy_binding, directive); - let _ = self.try_define(directive.parent, target, ValueNS, dummy_binding.clone()); let _ = self.try_define(directive.parent, target, TypeNS, dummy_binding); } From 5ba22c0ed68ed4b46c9db2ceac2e5cc96728411a Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Tue, 16 Aug 2016 06:03:36 +0000 Subject: [PATCH 08/18] Add `item_like_imports` feature. --- src/librustc_resolve/lib.rs | 2 ++ src/libsyntax/feature_gate.rs | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 124a748be326f..8428507e686df 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1053,6 +1053,7 @@ pub struct Resolver<'a> { arenas: &'a ResolverArenas<'a>, dummy_binding: &'a NameBinding<'a>, + new_import_semantics: bool, // true if `#![feature(item_like_imports)]` } pub struct ResolverArenas<'a> { @@ -1209,6 +1210,7 @@ impl<'a> Resolver<'a> { span: DUMMY_SP, vis: ty::Visibility::Public, }), + new_import_semantics: session.features.borrow().item_like_imports, } } diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index e224e30b1a2a4..02c44c3a56d7e 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -285,7 +285,10 @@ declare_features! ( // Allows the sysV64 ABI to be specified on all platforms // instead of just the platforms on which it is the C ABI - (active, abi_sysv64, "1.13.0", Some(36167)) + (active, abi_sysv64, "1.13.0", Some(36167)), + + // Use the import semantics from RFC 1560. + (active, item_like_imports, "1.13.0", Some(35120)) ); declare_features! ( From efc0bea687a6c412b7d11acab12ada8e33050089 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 00:57:19 +0000 Subject: [PATCH 09/18] item_like_imports: Treat private imports like private items. --- src/librustc_resolve/resolve_imports.rs | 4 +++- src/test/run-pass/imports.rs | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 src/test/run-pass/imports.rs diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index f02e9b048dea7..d6aae23947f2e 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -167,8 +167,10 @@ impl<'a> Resolver<'a> { _ => return Failed(None), // This happens when there is a cycle of imports }; + let new_import_semantics = self.new_import_semantics; let is_disallowed_private_import = |binding: &NameBinding| { - !allow_private_imports && binding.vis != ty::Visibility::Public && binding.is_import() + !new_import_semantics && !allow_private_imports && // disallowed + binding.vis != ty::Visibility::Public && binding.is_import() // non-`pub` import }; if let Some(span) = record_used { diff --git a/src/test/run-pass/imports.rs b/src/test/run-pass/imports.rs new file mode 100644 index 0000000000000..900e0b69ebba5 --- /dev/null +++ b/src/test/run-pass/imports.rs @@ -0,0 +1,24 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(item_like_imports)] +#![allow(unused)] + +// Like other items, private imports can be imported and used non-lexically in paths. +mod a { + use a as foo; + use self::foo::foo as bar; + + mod b { + use super::bar; + } +} + +fn main() {} From aad1f3cbf3340afd0685b79981a59f0ba3e83116 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 18 Aug 2016 20:33:24 +0000 Subject: [PATCH 10/18] item_like_imports: Allow glob imports to be shadowed by items and single imports. --- src/librustc_resolve/resolve_imports.rs | 20 +++++++----- src/test/run-pass/imports.rs | 42 +++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index d6aae23947f2e..c7cb3a351784d 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -690,15 +690,21 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { }; // Report conflicts - for duplicate_glob in resolution.duplicate_globs.iter() { - // FIXME #31337: We currently allow items to shadow glob-imported re-exports. - if !binding.is_import() { - if let NameBindingKind::Import { binding, .. } = duplicate_glob.kind { - if binding.is_import() { continue } + if !self.new_import_semantics { + for duplicate_glob in resolution.duplicate_globs.iter() { + // FIXME #31337: We currently allow items to shadow glob-imported re-exports. + if !binding.is_import() { + if let NameBindingKind::Import { binding, .. } = duplicate_glob.kind { + if binding.is_import() { continue } + } } - } - self.report_conflict(module, name, ns, duplicate_glob, binding); + self.report_conflict(module, name, ns, duplicate_glob, binding); + } + } else if binding.is_glob_import() { + for duplicate_glob in resolution.duplicate_globs.iter() { + self.report_conflict(module, name, ns, duplicate_glob, binding); + } } if binding.vis == ty::Visibility::Public && diff --git a/src/test/run-pass/imports.rs b/src/test/run-pass/imports.rs index 900e0b69ebba5..df4961c074ae1 100644 --- a/src/test/run-pass/imports.rs +++ b/src/test/run-pass/imports.rs @@ -21,4 +21,46 @@ mod a { } } +mod foo { pub fn f() {} } +mod bar { pub fn f() {} } + +pub fn f() -> bool { true } + +// Items and explicit imports shadow globs. +fn g() { + use foo::*; + use bar::*; + fn f() -> bool { true } + let _: bool = f(); +} + +fn h() { + use foo::*; + use bar::*; + use f; + let _: bool = f(); +} + +// Here, there appears to be shadowing but isn't because of namespaces. +mod b { + use foo::*; // This imports `f` in the value namespace. + use super::b as f; // This imports `f` only in the type namespace, + fn test() { self::f(); } // so the glob isn't shadowed. +} + +// Here, there is shadowing in one namespace, but not the other. +mod c { + mod test { + pub fn f() {} + pub mod f {} + } + use self::test::*; // This glob-imports `f` in both namespaces. + mod f { pub fn f() {} } // This shadows the glob only in the value namespace. + + fn test() { + self::f(); // Check that the glob-imported value isn't shadowed. + self::f::f(); // Check that the glob-imported module is shadowed. + } +} + fn main() {} From c56a5afd4d1c4717770efa693e69eead13abee34 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 19 Aug 2016 22:52:26 +0000 Subject: [PATCH 11/18] item_like_imports: Allow single imports with a given visibility to reexport some (but not all) namespaces with less visibility. --- src/librustc_resolve/resolve_imports.rs | 51 +++++++++++++++++----- src/test/compile-fail/imports/reexports.rs | 37 ++++++++++++++++ 2 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 src/test/compile-fail/imports/reexports.rs diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index c7cb3a351784d..7084aa685aec5 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -285,13 +285,20 @@ impl<'a> Resolver<'a> { // return the corresponding binding defined by the import directive. fn import(&mut self, binding: &'a NameBinding<'a>, directive: &'a ImportDirective<'a>) -> NameBinding<'a> { + let vis = if binding.pseudo_vis().is_at_least(directive.vis.get(), self) || + !directive.is_glob() && binding.is_extern_crate() { // c.f. `PRIVATE_IN_PUBLIC` + directive.vis.get() + } else { + binding.pseudo_vis() + }; + NameBinding { kind: NameBindingKind::Import { binding: binding, directive: directive, }, span: directive.span, - vis: directive.vis.get(), + vis: vis, } } @@ -597,22 +604,44 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } } + let session = self.session; + let reexport_error = || { + let msg = format!("`{}` is private, and cannot be reexported", name); + let note_msg = + format!("consider marking `{}` as `pub` in the imported module", name); + struct_span_err!(session, directive.span, E0364, "{}", &msg) + .span_note(directive.span, ¬e_msg) + .emit(); + }; + + let extern_crate_lint = || { + let msg = format!("extern crate `{}` is private, and cannot be reexported \ + (error E0364), consider declaring with `pub`", + name); + session.add_lint(PRIVATE_IN_PUBLIC, directive.id, directive.span, msg); + }; + match (value_result, type_result) { + // With `#![feature(item_like_imports)]`, all namespaces + // must be re-exported with extra visibility for an error to occur. + (Ok(value_binding), Ok(type_binding)) if self.new_import_semantics => { + let vis = directive.vis.get(); + if !value_binding.pseudo_vis().is_at_least(vis, self) && + !type_binding.pseudo_vis().is_at_least(vis, self) { + reexport_error(); + } else if type_binding.is_extern_crate() && + !type_binding.vis.is_at_least(vis, self) { + extern_crate_lint(); + } + } + (Ok(binding), _) if !binding.pseudo_vis().is_at_least(directive.vis.get(), self) => { - let msg = format!("`{}` is private, and cannot be reexported", name); - let note_msg = - format!("consider marking `{}` as `pub` in the imported module", name); - struct_span_err!(self.session, directive.span, E0364, "{}", &msg) - .span_note(directive.span, ¬e_msg) - .emit(); + reexport_error(); } (_, Ok(binding)) if !binding.pseudo_vis().is_at_least(directive.vis.get(), self) => { if binding.is_extern_crate() { - let msg = format!("extern crate `{}` is private, and cannot be reexported \ - (error E0364), consider declaring with `pub`", - name); - self.session.add_lint(PRIVATE_IN_PUBLIC, directive.id, directive.span, msg); + extern_crate_lint(); } else { struct_span_err!(self.session, directive.span, E0365, "`{}` is private, and cannot be reexported", name) diff --git a/src/test/compile-fail/imports/reexports.rs b/src/test/compile-fail/imports/reexports.rs new file mode 100644 index 0000000000000..f8dbb4d444886 --- /dev/null +++ b/src/test/compile-fail/imports/reexports.rs @@ -0,0 +1,37 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(item_like_imports)] + +mod a { + fn foo() {} + mod foo {} + + mod a { + pub use super::foo; //~ ERROR cannot be reexported + } +} + +mod b { + pub fn foo() {} + mod foo { pub struct S; } + + pub mod a { + pub use super::foo; // This is OK since the value `foo` is visible enough. + fn f(_: foo::S) {} // `foo` is imported in the type namespace (but not `pub` reexported). + } +} + +mod c { + // Test that `foo` is not reexported. + use b::a::foo::S; //~ ERROR `foo` +} + +fn main() {} From 097b6d62fc7431b322b46b3a0e9f36134c13dd82 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 00:23:32 +0000 Subject: [PATCH 12/18] item_like_imports: Allow glob imports with a given visibility to reexport some (but not all) names with less visibility. --- src/librustc_resolve/build_reduced_graph.rs | 7 +++++- src/librustc_resolve/resolve_imports.rs | 25 +++++++++++++++++---- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 83d35095f351b..0c7c970371877 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -26,6 +26,8 @@ use rustc::hir::def::*; use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId}; use rustc::ty::{self, VariantKind}; +use std::cell::Cell; + use syntax::ast::Name; use syntax::attr; use syntax::parse::token; @@ -176,7 +178,10 @@ impl<'b> Resolver<'b> { } } ViewPathGlob(_) => { - let subclass = GlobImport { is_prelude: is_prelude }; + let subclass = GlobImport { + is_prelude: is_prelude, + max_vis: Cell::new(ty::Visibility::PrivateExternal), + }; let span = view_path.span; self.add_import_directive(module_path, subclass, span, item.id, vis); } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 7084aa685aec5..189253348b0aa 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -52,7 +52,10 @@ pub enum ImportDirectiveSubclass<'a> { value_result: Cell, Determinacy>>, type_result: Cell, Determinacy>>, }, - GlobImport { is_prelude: bool }, + GlobImport { + is_prelude: bool, + max_vis: Cell, // The visibility of the greatest reexport. + }, } impl<'a> ImportDirectiveSubclass<'a> { @@ -276,7 +279,7 @@ impl<'a> Resolver<'a> { } // We don't add prelude imports to the globs since they only affect lexical scopes, // which are not relevant to import resolution. - GlobImport { is_prelude: true } => {} + GlobImport { is_prelude: true, .. } => {} GlobImport { .. } => self.current_module.globs.borrow_mut().push(directive), } } @@ -292,6 +295,12 @@ impl<'a> Resolver<'a> { binding.pseudo_vis() }; + if let GlobImport { ref max_vis, .. } = directive.subclass { + if vis == directive.vis.get() || vis.is_at_least(max_vis.get(), self) { + max_vis.set(vis) + } + } + NameBinding { kind: NameBindingKind::Import { binding: binding, @@ -562,7 +571,15 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { let msg = "Cannot glob-import a module into itself.".into(); return Failed(Some((directive.span, msg))); } - GlobImport { .. } => return Success(()), + GlobImport { is_prelude, ref max_vis } => { + if !is_prelude && + max_vis.get() != ty::Visibility::PrivateExternal && // Allow empty globs. + !max_vis.get().is_at_least(directive.vis.get(), self) { + let msg = "A non-empty glob must import something with the glob's visibility"; + self.session.span_err(directive.span, msg); + } + return Success(()); + } }; for &(ns, result) in &[(ValueNS, value_result), (TypeNS, type_result)] { @@ -677,7 +694,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { return; } else if module.def_id() == directive.parent.def_id() { return; - } else if let GlobImport { is_prelude: true } = directive.subclass { + } else if let GlobImport { is_prelude: true, .. } = directive.subclass { self.prelude = Some(module); return; } From 245a0c5530f8d4d251bcef2d7b8de2fa19f442bf Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 00:33:06 +0000 Subject: [PATCH 13/18] item_like_imports: Make all visible items glob importable. --- src/librustc_resolve/lib.rs | 4 ++++ src/librustc_resolve/resolve_imports.rs | 10 +++++++--- src/test/compile-fail/imports/reexports.rs | 7 +++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 8428507e686df..12b708fa1a163 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -3255,6 +3255,10 @@ impl<'a> Resolver<'a> { vis.is_accessible_from(self.current_module.normal_ancestor_id, self) } + fn is_accessible_from(&self, vis: ty::Visibility, module: Module<'a>) -> bool { + vis.is_accessible_from(module.normal_ancestor_id, self) + } + fn report_privacy_errors(&self) { if self.privacy_errors.len() == 0 { return } let mut reported_spans = FnvHashSet(); diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 189253348b0aa..4ab4ec4789d01 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -356,8 +356,11 @@ impl<'a> Resolver<'a> { }; // Define `binding` in `module`s glob importers. - if binding.vis == ty::Visibility::Public { - for directive in module.glob_importers.borrow_mut().iter() { + for directive in module.glob_importers.borrow_mut().iter() { + if match self.new_import_semantics { + true => self.is_accessible_from(binding.vis, directive.parent), + false => binding.vis == ty::Visibility::Public, + } { let imported_binding = self.import(binding, directive); let _ = self.try_define(directive.parent, name, ns, imported_binding); } @@ -708,7 +711,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { resolution.borrow().binding().map(|binding| (*name, binding)) }).collect::>(); for ((name, ns), binding) in bindings { - if binding.pseudo_vis() == ty::Visibility::Public { + if binding.pseudo_vis() == ty::Visibility::Public || + self.new_import_semantics && self.is_accessible(binding.vis) { let imported_binding = self.import(binding, directive); let _ = self.try_define(directive.parent, name, ns, imported_binding); } diff --git a/src/test/compile-fail/imports/reexports.rs b/src/test/compile-fail/imports/reexports.rs index f8dbb4d444886..fc46b23351adf 100644 --- a/src/test/compile-fail/imports/reexports.rs +++ b/src/test/compile-fail/imports/reexports.rs @@ -16,6 +16,7 @@ mod a { mod a { pub use super::foo; //~ ERROR cannot be reexported + pub use super::*; //~ ERROR must import something with the glob's visibility } } @@ -27,11 +28,17 @@ mod b { pub use super::foo; // This is OK since the value `foo` is visible enough. fn f(_: foo::S) {} // `foo` is imported in the type namespace (but not `pub` reexported). } + + pub mod b { + pub use super::*; // This is also OK since the value `foo` is visible enough. + fn f(_: foo::S) {} // Again, the module `foo` is imported (but not `pub` reexported). + } } mod c { // Test that `foo` is not reexported. use b::a::foo::S; //~ ERROR `foo` + use b::b::foo::S as T; //~ ERROR `foo` } fn main() {} From f582fa327e210ea5bf91d6b8f174f35fc9006054 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 03:28:35 +0000 Subject: [PATCH 14/18] item_like_imports: Allow multiple glob imports of the same item. --- src/librustc_resolve/resolve_imports.rs | 20 +++++++++--- src/test/compile-fail/imports/duplicate.rs | 38 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 src/test/compile-fail/imports/duplicate.rs diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 4ab4ec4789d01..495bdcb7dfd2a 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -317,10 +317,17 @@ impl<'a> Resolver<'a> { where T: ToNameBinding<'a> { let binding = self.arenas.alloc_name_binding(binding.to_name_binding()); - self.update_resolution(module, name, ns, |_, resolution| { + self.update_resolution(module, name, ns, |this, resolution| { if let Some(old_binding) = resolution.binding { if binding.is_glob_import() { - resolution.duplicate_globs.push(binding); + if !this.new_import_semantics || !old_binding.is_glob_import() { + resolution.duplicate_globs.push(binding); + } else if binding.def() != old_binding.def() { + resolution.duplicate_globs.push(binding); + } else if !old_binding.vis.is_at_least(binding.vis, this) { + // We are glob-importing the same item but with greater visibility. + resolution.binding = Some(binding); + } } else if old_binding.is_glob_import() { resolution.duplicate_globs.push(old_binding); resolution.binding = Some(binding); @@ -344,14 +351,17 @@ impl<'a> Resolver<'a> { // during which the resolution might end up getting re-defined via a glob cycle. let (binding, t) = { let mut resolution = &mut *self.resolution(module, name, ns).borrow_mut(); - let was_known = resolution.binding().is_some(); + let old_binding = resolution.binding(); let t = f(self, resolution); - if was_known { return t; } match resolution.binding() { - Some(binding) => (binding, t), + _ if !self.new_import_semantics && old_binding.is_some() => return t, None => return t, + Some(binding) => match old_binding { + Some(old_binding) if old_binding as *const _ == binding as *const _ => return t, + _ => (binding, t), + } } }; diff --git a/src/test/compile-fail/imports/duplicate.rs b/src/test/compile-fail/imports/duplicate.rs new file mode 100644 index 0000000000000..0b5963dd89315 --- /dev/null +++ b/src/test/compile-fail/imports/duplicate.rs @@ -0,0 +1,38 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(item_like_imports)] + +mod a { + pub fn foo() {} +} + +mod b { + pub fn foo() {} +} + +mod c { + pub use a::foo; +} + +mod d { + use a::foo; //~ NOTE previous import + use a::foo; //~ ERROR `foo` has already been imported + //~| NOTE already imported +} + +mod e { + pub use a::*; + pub use c::*; // ok +} + +fn main() { + e::foo(); +} From 681a14f29b5e8d8745bda4fc7ba4d4ccb634ddb9 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 20 Aug 2016 05:23:19 +0000 Subject: [PATCH 15/18] item_like_imports: Allow unused ambiguous glob imports. --- src/librustc_resolve/lib.rs | 34 ++++++++++++++++++++-- src/librustc_resolve/resolve_imports.rs | 31 +++++++++++++++----- src/test/compile-fail/imports/duplicate.rs | 14 +++++++++ src/test/run-pass/imports.rs | 10 +++++++ 4 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 12b708fa1a163..d77258f44eb9a 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -874,6 +874,10 @@ enum NameBindingKind<'a> { binding: &'a NameBinding<'a>, directive: &'a ImportDirective<'a>, }, + Ambiguity { + b1: &'a NameBinding<'a>, + b2: &'a NameBinding<'a>, + } } #[derive(Clone, Debug)] @@ -885,6 +889,7 @@ impl<'a> NameBinding<'a> { NameBindingKind::Module(module) => Some(module), NameBindingKind::Def(_) => None, NameBindingKind::Import { binding, .. } => binding.module(), + NameBindingKind::Ambiguity { .. } => None, } } @@ -893,6 +898,7 @@ impl<'a> NameBinding<'a> { NameBindingKind::Def(def) => def, NameBindingKind::Module(module) => module.def.unwrap(), NameBindingKind::Import { binding, .. } => binding.def(), + NameBindingKind::Ambiguity { .. } => Def::Err, } } @@ -922,6 +928,7 @@ impl<'a> NameBinding<'a> { fn is_glob_import(&self) -> bool { match self.kind { NameBindingKind::Import { directive, .. } => directive.is_glob(), + NameBindingKind::Ambiguity { .. } => true, _ => false, } } @@ -932,6 +939,14 @@ impl<'a> NameBinding<'a> { _ => true, } } + + fn ambiguity(&self) -> Option<(&'a NameBinding<'a>, &'a NameBinding<'a>)> { + match self.kind { + NameBindingKind::Ambiguity { b1, b2 } => Some((b1, b2)), + NameBindingKind::Import { binding, .. } => binding.ambiguity(), + _ => None, + } + } } /// Interns the names of the primitive types. @@ -1249,7 +1264,8 @@ impl<'a> Resolver<'a> { match ns { ValueNS => &mut self.value_ribs, TypeNS => &mut self.type_ribs } } - fn record_use(&mut self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) { + fn record_use(&mut self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>, span: Span) + -> bool /* true if an error was reported */ { // 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); @@ -1259,6 +1275,19 @@ impl<'a> Resolver<'a> { self.used_imports.insert((directive.id, ns)); self.add_to_glob_map(directive.id, name); } + + if let Some((b1, b2)) = binding.ambiguity() { + let msg1 = format!("`{}` could resolve to the name imported here", name); + let msg2 = format!("`{}` could also resolve to the name imported here", name); + self.session.struct_span_err(span, &format!("`{}` is ambiguous", name)) + .span_note(b1.span, &msg1) + .span_note(b2.span, &msg2) + .note(&format!("Consider adding an explicit import of `{}` to disambiguate", name)) + .emit(); + return true; + } + + false } fn add_to_glob_map(&mut self, id: NodeId, name: Name) { @@ -2294,7 +2323,8 @@ impl<'a> Resolver<'a> { Def::Struct(..) | Def::Variant(..) | Def::Const(..) | Def::AssociatedConst(..) if !always_binding => { // A constant, unit variant, etc pattern. - self.record_use(ident.node.name, ValueNS, binding.unwrap()); + let name = ident.node.name; + self.record_use(name, ValueNS, binding.unwrap(), ident.span); Some(PathResolution::new(def)) } Def::Struct(..) | Def::Variant(..) | diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 495bdcb7dfd2a..cb89231fc0551 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -181,7 +181,9 @@ impl<'a> Resolver<'a> { if is_disallowed_private_import(binding) { return Failed(None); } - self.record_use(name, ns, binding); + if self.record_use(name, ns, binding, span) { + return Success(self.dummy_binding); + } if !self.is_accessible(binding.vis) { self.privacy_errors.push(PrivacyError(span, name, binding)); } @@ -323,7 +325,18 @@ impl<'a> Resolver<'a> { if !this.new_import_semantics || !old_binding.is_glob_import() { resolution.duplicate_globs.push(binding); } else if binding.def() != old_binding.def() { - resolution.duplicate_globs.push(binding); + resolution.binding = Some(this.arenas.alloc_name_binding(NameBinding { + kind: NameBindingKind::Ambiguity { + b1: old_binding, + b2: binding, + }, + vis: if old_binding.vis.is_at_least(binding.vis, this) { + old_binding.vis + } else { + binding.vis + }, + span: old_binding.span, + })); } else if !old_binding.vis.is_at_least(binding.vis, this) { // We are glob-importing the same item but with greater visibility. resolution.binding = Some(binding); @@ -597,7 +610,10 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { for &(ns, result) in &[(ValueNS, value_result), (TypeNS, type_result)] { if let Ok(binding) = result { - self.record_use(name, ns, binding); + if self.record_use(name, ns, binding, directive.span) { + self.resolution(module, name, ns).borrow_mut().binding = + Some(self.dummy_binding); + } } } @@ -759,17 +775,16 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } } - self.report_conflict(module, name, ns, duplicate_glob, binding); - } - } else if binding.is_glob_import() { - for duplicate_glob in resolution.duplicate_globs.iter() { self.report_conflict(module, name, ns, duplicate_glob, binding); } } if binding.vis == ty::Visibility::Public && (binding.is_import() || binding.is_extern_crate()) { - reexports.push(Export { name: name, def_id: binding.def().def_id() }); + let def = binding.def(); + if def != Def::Err { + reexports.push(Export { name: name, def_id: def.def_id() }); + } } if let NameBindingKind::Import { binding: orig_binding, directive, .. } = binding.kind { diff --git a/src/test/compile-fail/imports/duplicate.rs b/src/test/compile-fail/imports/duplicate.rs index 0b5963dd89315..05c0d9cd38e83 100644 --- a/src/test/compile-fail/imports/duplicate.rs +++ b/src/test/compile-fail/imports/duplicate.rs @@ -33,6 +33,20 @@ mod e { pub use c::*; // ok } +mod f { + pub use a::*; //~ NOTE `foo` could resolve to the name imported here + pub use b::*; //~ NOTE `foo` could also resolve to the name imported here +} + +mod g { + pub use a::*; //~ NOTE `foo` could resolve to the name imported here + pub use f::*; //~ NOTE `foo` could also resolve to the name imported here +} + fn main() { e::foo(); + f::foo(); //~ ERROR `foo` is ambiguous + //~| NOTE Consider adding an explicit import of `foo` to disambiguate + g::foo(); //~ ERROR `foo` is ambiguous + //~| NOTE Consider adding an explicit import of `foo` to disambiguate } diff --git a/src/test/run-pass/imports.rs b/src/test/run-pass/imports.rs index df4961c074ae1..195b99c9788e8 100644 --- a/src/test/run-pass/imports.rs +++ b/src/test/run-pass/imports.rs @@ -63,4 +63,14 @@ mod c { } } +// Unused names can be ambiguous. +mod d { + pub use foo::*; // This imports `f` in the value namespace. + pub use bar::*; // This also imports `f` in the value namespace. +} + +mod e { + pub use d::*; // n.b. Since `e::f` is not used, this is not considered to be a use of `d::f`. +} + fn main() {} From 32a0cfeb485f2c1bbfe52eaa9e119e974e38f21f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 22 Aug 2016 08:30:07 +0000 Subject: [PATCH 16/18] Avoid reporting multiple ambiguity errors for a single use of a name. --- src/librustc_resolve/lib.rs | 30 ++++++++++++++-------- src/test/compile-fail/imports/duplicate.rs | 13 ++++++++++ 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index d77258f44eb9a..a881feaa4d353 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1065,6 +1065,7 @@ pub struct Resolver<'a> { pub maybe_unused_trait_imports: NodeSet, privacy_errors: Vec>, + ambiguity_errors: Vec<(Span, Name, &'a NameBinding<'a>)>, arenas: &'a ResolverArenas<'a>, dummy_binding: &'a NameBinding<'a>, @@ -1218,6 +1219,7 @@ impl<'a> Resolver<'a> { maybe_unused_trait_imports: NodeSet(), privacy_errors: Vec::new(), + ambiguity_errors: Vec::new(), arenas: arenas, dummy_binding: arenas.alloc_name_binding(NameBinding { @@ -1245,7 +1247,7 @@ impl<'a> Resolver<'a> { visit::walk_crate(self, krate); check_unused::check_crate(self, krate); - self.report_privacy_errors(); + self.report_errors(); } fn new_module(&self, parent_link: ParentLink<'a>, def: Option, normal_ancestor_id: NodeId) @@ -1276,14 +1278,8 @@ impl<'a> Resolver<'a> { self.add_to_glob_map(directive.id, name); } - if let Some((b1, b2)) = binding.ambiguity() { - let msg1 = format!("`{}` could resolve to the name imported here", name); - let msg2 = format!("`{}` could also resolve to the name imported here", name); - self.session.struct_span_err(span, &format!("`{}` is ambiguous", name)) - .span_note(b1.span, &msg1) - .span_note(b2.span, &msg2) - .note(&format!("Consider adding an explicit import of `{}` to disambiguate", name)) - .emit(); + if binding.ambiguity().is_some() { + self.ambiguity_errors.push((span, name, binding)); return true; } @@ -3289,9 +3285,21 @@ impl<'a> Resolver<'a> { vis.is_accessible_from(module.normal_ancestor_id, self) } - fn report_privacy_errors(&self) { - if self.privacy_errors.len() == 0 { return } + fn report_errors(&self) { let mut reported_spans = FnvHashSet(); + + for &(span, name, binding) in &self.ambiguity_errors { + if !reported_spans.insert(span) { continue } + let (b1, b2) = binding.ambiguity().unwrap(); + let msg1 = format!("`{}` could resolve to the name imported here", name); + let msg2 = format!("`{}` could also resolve to the name imported here", name); + self.session.struct_span_err(span, &format!("`{}` is ambiguous", name)) + .span_note(b1.span, &msg1) + .span_note(b2.span, &msg2) + .note(&format!("Consider adding an explicit import of `{}` to disambiguate", name)) + .emit(); + } + for &PrivacyError(span, name, binding) in &self.privacy_errors { if !reported_spans.insert(span) { continue } if binding.is_extern_crate() { diff --git a/src/test/compile-fail/imports/duplicate.rs b/src/test/compile-fail/imports/duplicate.rs index 05c0d9cd38e83..70936b2544646 100644 --- a/src/test/compile-fail/imports/duplicate.rs +++ b/src/test/compile-fail/imports/duplicate.rs @@ -50,3 +50,16 @@ fn main() { g::foo(); //~ ERROR `foo` is ambiguous //~| NOTE Consider adding an explicit import of `foo` to disambiguate } + +mod ambiguous_module_errors { + pub mod m1 { pub use super::m1 as foo; } + pub mod m2 { pub use super::m2 as foo; } + + use self::m1::*; //~ NOTE + use self::m2::*; //~ NOTE + + fn f() { + foo::bar(); //~ ERROR `foo` is ambiguous + //~| NOTE + } +} From 4f5616e3c43f866f4758a21f67d98da52b89ee20 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 22 Aug 2016 07:12:13 +0000 Subject: [PATCH 17/18] Avoid cascading name resolution errors caused by an ambiguous module. --- src/librustc_resolve/lib.rs | 68 +++++++++++++--------- src/librustc_resolve/resolve_imports.rs | 2 +- src/test/compile-fail/imports/duplicate.rs | 5 ++ 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index a881feaa4d353..0fe7f9ed21547 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -749,10 +749,6 @@ impl<'a> LexicalScopeBinding<'a> { _ => None, } } - - fn module(self) -> Option> { - self.item().and_then(NameBinding::module) - } } /// The link from a module up to its nearest parent node. @@ -884,12 +880,13 @@ enum NameBindingKind<'a> { struct PrivacyError<'a>(Span, Name, &'a NameBinding<'a>); impl<'a> NameBinding<'a> { - fn module(&self) -> Option> { + fn module(&self) -> Result, bool /* true if an error has already been reported */> { match self.kind { - NameBindingKind::Module(module) => Some(module), - NameBindingKind::Def(_) => None, + NameBindingKind::Module(module) => Ok(module), NameBindingKind::Import { binding, .. } => binding.module(), - NameBindingKind::Ambiguity { .. } => None, + NameBindingKind::Def(Def::Err) => Err(true), + NameBindingKind::Def(_) => Err(false), + NameBindingKind::Ambiguity { .. } => Err(false), } } @@ -915,7 +912,7 @@ impl<'a> NameBinding<'a> { } fn is_extern_crate(&self) -> bool { - self.module().and_then(|module| module.extern_crate_id).is_some() + self.module().ok().and_then(|module| module.extern_crate_id).is_some() } fn is_import(&self) -> bool { @@ -1269,7 +1266,7 @@ impl<'a> Resolver<'a> { fn record_use(&mut self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>, span: Span) -> bool /* true if an error was reported */ { // track extern crates for unused_extern_crate lint - if let Some(DefId { krate, .. }) = binding.module().and_then(ModuleS::def_id) { + if let Some(DefId { krate, .. }) = binding.module().ok().and_then(ModuleS::def_id) { self.used_crates.insert(krate); } @@ -1292,6 +1289,18 @@ impl<'a> Resolver<'a> { } } + fn expect_module(&mut self, name: Name, binding: &'a NameBinding<'a>, span: Option) + -> ResolveResult> { + match binding.module() { + Ok(module) => Success(module), + Err(true) => Failed(None), + Err(false) => { + let msg = format!("Not a module `{}`", name); + Failed(span.map(|span| (span, msg))) + } + } + } + /// Resolves the given module path from the given root `search_module`. fn resolve_module_path_from_root(&mut self, mut search_module: Module<'a>, @@ -1357,11 +1366,9 @@ impl<'a> Resolver<'a> { Success(binding) => { // Check to see whether there are type bindings, and, if // so, whether there is a module within. - if let Some(module_def) = binding.module() { - search_module = module_def; - } else { - let msg = format!("Not a module `{}`", name); - return Failed(span.map(|span| (span, msg))); + match self.expect_module(name, binding, span) { + Success(module) => search_module = module, + result @ _ => return result, } } } @@ -1414,17 +1421,20 @@ impl<'a> Resolver<'a> { // first component of the path in the current lexical // scope and then proceed to resolve below that. let ident = ast::Ident::with_empty_ctxt(module_path[0]); - match self.resolve_ident_in_lexical_scope(ident, TypeNS, span) - .and_then(LexicalScopeBinding::module) { - None => { - let msg = - format!("Use of undeclared type or module `{}`", ident.name); - return Failed(span.map(|span| (span, msg))); - } - Some(containing_module) => { - search_module = containing_module; - start_index = 1; + let lexical_binding = + self.resolve_ident_in_lexical_scope(ident, TypeNS, span); + if let Some(binding) = lexical_binding.and_then(LexicalScopeBinding::item) { + match self.expect_module(ident.name, binding, span) { + Success(containing_module) => { + search_module = containing_module; + start_index = 1; + } + result @ _ => return result, } + } else { + let msg = + format!("Use of undeclared type or module `{}`", ident.name); + return Failed(span.map(|span| (span, msg))); } } } @@ -3202,7 +3212,7 @@ impl<'a> Resolver<'a> { } // collect submodules to explore - if let Some(module) = name_binding.module() { + if let Ok(module) = name_binding.module() { // form the path let path_segments = match module.parent_link { NoParentLink => path_segments.clone(), @@ -3341,9 +3351,9 @@ impl<'a> Resolver<'a> { let msg = { let kind = match (ns, old_binding.module()) { (ValueNS, _) => "a value", - (TypeNS, Some(module)) if module.extern_crate_id.is_some() => "an extern crate", - (TypeNS, Some(module)) if module.is_normal() => "a module", - (TypeNS, Some(module)) if module.is_trait() => "a trait", + (TypeNS, Ok(module)) if module.extern_crate_id.is_some() => "an extern crate", + (TypeNS, Ok(module)) if module.is_normal() => "a module", + (TypeNS, Ok(module)) if module.is_trait() => "a trait", (TypeNS, _) => "a type", }; format!("{} named `{}` has already been {} in this {}", diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index cb89231fc0551..c8982d95d4e00 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -460,7 +460,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { errors = true; let (span, help) = match err { Some((span, msg)) => (span, msg), - None => (import.span, String::new()), + None => continue, }; // If the error is a single failed import then create a "fake" import diff --git a/src/test/compile-fail/imports/duplicate.rs b/src/test/compile-fail/imports/duplicate.rs index 70936b2544646..fb61bb8e489be 100644 --- a/src/test/compile-fail/imports/duplicate.rs +++ b/src/test/compile-fail/imports/duplicate.rs @@ -56,7 +56,12 @@ mod ambiguous_module_errors { pub mod m2 { pub use super::m2 as foo; } use self::m1::*; //~ NOTE + //~| NOTE use self::m2::*; //~ NOTE + //~| NOTE + + use self::foo::bar; //~ ERROR `foo` is ambiguous + //~| NOTE fn f() { foo::bar(); //~ ERROR `foo` is ambiguous From 90ce504c1c3dc014ca8e0aa91e21c46569a9d4ab Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 29 Aug 2016 05:29:01 +0000 Subject: [PATCH 18/18] Address comments. --- src/librustc_resolve/build_reduced_graph.rs | 8 ++--- src/librustc_resolve/lib.rs | 34 ++++++++++++--------- src/librustc_resolve/resolve_imports.rs | 1 + src/test/run-pass/imports.rs | 2 ++ 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 0c7c970371877..3e9b37f0a95a7 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -32,7 +32,7 @@ use syntax::ast::Name; use syntax::attr; use syntax::parse::token; -use syntax::ast::{Block, Crate, DUMMY_NODE_ID}; +use syntax::ast::{Block, Crate}; use syntax::ast::{ForeignItem, ForeignItemKind, Item, ItemKind}; use syntax::ast::{Mutability, StmtKind, TraitItemKind}; use syntax::ast::{Variant, ViewPathGlob, ViewPathList, ViewPathSimple}; @@ -208,7 +208,7 @@ impl<'b> Resolver<'b> { ItemKind::Mod(..) => { let parent_link = ModuleParentLink(parent, name); let def = Def::Mod(self.definitions.local_def_id(item.id)); - let module = self.new_module(parent_link, Some(def), item.id); + let module = self.new_module(parent_link, Some(def), Some(item.id)); module.no_implicit_prelude.set({ parent.no_implicit_prelude.get() || attr::contains_name(&item.attrs, "no_implicit_prelude") @@ -398,7 +398,7 @@ impl<'b> Resolver<'b> { debug!("(building reduced graph for external crate) building module {} {:?}", name, vis); let parent_link = ModuleParentLink(parent, name); - let module = self.new_module(parent_link, Some(def), DUMMY_NODE_ID); + let module = self.new_module(parent_link, Some(def), None); let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); } Def::Variant(_, variant_id) => { @@ -440,7 +440,7 @@ impl<'b> Resolver<'b> { } let parent_link = ModuleParentLink(parent, name); - let module = self.new_module(parent_link, Some(def), DUMMY_NODE_ID); + let module = self.new_module(parent_link, Some(def), None); let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); } Def::TyAlias(..) | Def::AssociatedTy(..) => { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 0fe7f9ed21547..1224c694a4e6e 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -54,7 +54,7 @@ use rustc::util::nodemap::{NodeMap, NodeSet, FnvHashMap, FnvHashSet}; use syntax::ext::hygiene::Mark; use syntax::ast::{self, FloatTy}; -use syntax::ast::{CRATE_NODE_ID, DUMMY_NODE_ID, Name, NodeId, CrateNum, IntTy, UintTy}; +use syntax::ast::{CRATE_NODE_ID, Name, NodeId, CrateNum, IntTy, UintTy}; use syntax::parse::token::{self, keywords}; use syntax::util::lev_distance::find_best_match_for_name; @@ -765,7 +765,7 @@ pub struct ModuleS<'a> { def: Option, // The node id of the closest normal module (`mod`) ancestor (including this module). - normal_ancestor_id: NodeId, + normal_ancestor_id: Option, // If the module is an extern crate, `def` is root of the external crate and `extern_crate_id` // is the NodeId of the local `extern crate` item (otherwise, `extern_crate_id` is None). @@ -790,7 +790,8 @@ pub struct ModuleS<'a> { pub type Module<'a> = &'a ModuleS<'a>; impl<'a> ModuleS<'a> { - fn new(parent_link: ParentLink<'a>, def: Option, normal_ancestor_id: NodeId) -> Self { + fn new(parent_link: ParentLink<'a>, def: Option, normal_ancestor_id: Option) + -> Self { ModuleS { parent_link: parent_link, def: def, @@ -801,7 +802,7 @@ impl<'a> ModuleS<'a> { glob_importers: RefCell::new(Vec::new()), globs: RefCell::new((Vec::new())), traits: RefCell::new(None), - populated: Cell::new(normal_ancestor_id != DUMMY_NODE_ID), + populated: Cell::new(normal_ancestor_id.is_some()), } } @@ -1104,7 +1105,7 @@ impl<'a> ty::NodeIdTree for Resolver<'a> { fn is_descendant_of(&self, mut node: NodeId, ancestor: NodeId) -> bool { while node != ancestor { node = match self.module_map[&node].parent() { - Some(parent) => parent.normal_ancestor_id, + Some(parent) => parent.normal_ancestor_id.unwrap(), None => return false, } } @@ -1168,7 +1169,8 @@ impl<'a> Resolver<'a> { pub fn new(session: &'a Session, make_glob_map: MakeGlobMap, arenas: &'a ResolverArenas<'a>) -> Resolver<'a> { let root_def_id = DefId::local(CRATE_DEF_INDEX); - let graph_root = ModuleS::new(NoParentLink, Some(Def::Mod(root_def_id)), CRATE_NODE_ID); + let graph_root = + ModuleS::new(NoParentLink, Some(Def::Mod(root_def_id)), Some(CRATE_NODE_ID)); let graph_root = arenas.alloc_module(graph_root); let mut module_map = NodeMap(); module_map.insert(CRATE_NODE_ID, graph_root); @@ -1247,14 +1249,17 @@ impl<'a> Resolver<'a> { self.report_errors(); } - fn new_module(&self, parent_link: ParentLink<'a>, def: Option, normal_ancestor_id: NodeId) + fn new_module(&self, + parent_link: ParentLink<'a>, + def: Option, + normal_ancestor_id: Option) -> Module<'a> { self.arenas.alloc_module(ModuleS::new(parent_link, def, normal_ancestor_id)) } fn new_extern_crate_module(&self, parent_link: ParentLink<'a>, def: Def, local_node_id: NodeId) -> Module<'a> { - let mut module = ModuleS::new(parent_link, Some(def), local_node_id); + let mut module = ModuleS::new(parent_link, Some(def), Some(local_node_id)); module.extern_crate_id = Some(local_node_id); self.arenas.modules.alloc(module) } @@ -1530,14 +1535,15 @@ impl<'a> Resolver<'a> { _ => return Success(NoPrefixFound), }; - let mut containing_module = self.module_map[&self.current_module.normal_ancestor_id]; + let mut containing_module = + self.module_map[&self.current_module.normal_ancestor_id.unwrap()]; // Now loop through all the `super`s we find. while i < module_path.len() && "super" == module_path[i].as_str() { debug!("(resolving module prefix) resolving `super` at {}", module_to_string(&containing_module)); if let Some(parent) = containing_module.parent() { - containing_module = self.module_map[&parent.normal_ancestor_id]; + containing_module = self.module_map[&parent.normal_ancestor_id.unwrap()]; i += 1; } else { let msg = "There are too many initial `super`s.".into(); @@ -3260,7 +3266,7 @@ impl<'a> Resolver<'a> { ast::Visibility::Crate(_) => return ty::Visibility::Restricted(ast::CRATE_NODE_ID), ast::Visibility::Restricted { ref path, id } => (path, id), ast::Visibility::Inherited => { - return ty::Visibility::Restricted(self.current_module.normal_ancestor_id); + return ty::Visibility::Restricted(self.current_module.normal_ancestor_id.unwrap()); } }; @@ -3269,7 +3275,7 @@ impl<'a> Resolver<'a> { let vis = match self.resolve_module_path(&segments, DontUseLexicalScope, Some(path.span)) { Success(module) => { path_resolution = PathResolution::new(module.def.unwrap()); - ty::Visibility::Restricted(module.normal_ancestor_id) + ty::Visibility::Restricted(module.normal_ancestor_id.unwrap()) } Indeterminate => unreachable!(), Failed(err) => { @@ -3288,11 +3294,11 @@ impl<'a> Resolver<'a> { } fn is_accessible(&self, vis: ty::Visibility) -> bool { - vis.is_accessible_from(self.current_module.normal_ancestor_id, self) + vis.is_accessible_from(self.current_module.normal_ancestor_id.unwrap(), self) } fn is_accessible_from(&self, vis: ty::Visibility, module: Module<'a>) -> bool { - vis.is_accessible_from(module.normal_ancestor_id, self) + vis.is_accessible_from(module.normal_ancestor_id.unwrap(), self) } fn report_errors(&self) { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index c8982d95d4e00..875d6745f6b2e 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -55,6 +55,7 @@ pub enum ImportDirectiveSubclass<'a> { GlobImport { is_prelude: bool, max_vis: Cell, // The visibility of the greatest reexport. + // n.b. `max_vis` is only used in `finalize_import` to check for reexport errors. }, } diff --git a/src/test/run-pass/imports.rs b/src/test/run-pass/imports.rs index 195b99c9788e8..9851dfe0262f8 100644 --- a/src/test/run-pass/imports.rs +++ b/src/test/run-pass/imports.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// ignore-pretty : (#23623) problems when ending with // comments + #![feature(item_like_imports)] #![allow(unused)]