From fc06ea5f9c904df1325b9e886353e561ea2ba385 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 17 Mar 2016 01:13:31 +0000 Subject: [PATCH 01/10] Add a type parameter to ImportDirective --- src/librustc_resolve/lib.rs | 11 ++++++----- src/librustc_resolve/resolve_imports.rs | 17 ++++++++--------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 2f6d5e1c36e0a..be01880e2a915 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -828,7 +828,7 @@ pub struct ModuleS<'a> { extern_crate_id: Option, resolutions: RefCell>>, - unresolved_imports: RefCell>, + unresolved_imports: RefCell>>, // The module children of this node, including normal modules and anonymous modules. // Anonymous children are pseudo-modules that are implicitly created around items @@ -848,7 +848,7 @@ pub struct ModuleS<'a> { prelude: RefCell>>, - glob_importers: RefCell, &'a ImportDirective)>>, + glob_importers: RefCell, &'a ImportDirective<'a>)>>, resolved_globs: RefCell<(Vec> /* public */, Vec> /* private */)>, // The number of public glob imports in this module. @@ -891,7 +891,7 @@ impl<'a> ModuleS<'a> { } } - fn add_import_directive(&self, import_directive: ImportDirective) { + fn add_import_directive(&self, import_directive: ImportDirective<'a>) { let import_directive = self.arenas.alloc_import_directive(import_directive); self.unresolved_imports.borrow_mut().push(import_directive); } @@ -1134,7 +1134,7 @@ pub struct Resolver<'a, 'tcx: 'a> { struct ResolverArenas<'a> { modules: arena::TypedArena>, name_bindings: arena::TypedArena>, - import_directives: arena::TypedArena, + import_directives: arena::TypedArena>, } impl<'a> ResolverArenas<'a> { @@ -1144,7 +1144,8 @@ impl<'a> ResolverArenas<'a> { fn alloc_name_binding(&'a self, name_binding: NameBinding<'a>) -> &'a NameBinding<'a> { self.name_bindings.alloc(name_binding) } - fn alloc_import_directive(&'a self, import_directive: ImportDirective) -> &'a ImportDirective { + fn alloc_import_directive(&'a self, import_directive: ImportDirective<'a>) + -> &'a ImportDirective { self.import_directives.alloc(import_directive) } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 7c5d131dbc540..b446fa6430e8b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -57,7 +57,7 @@ impl ImportDirectiveSubclass { /// One import directive. #[derive(Debug,Clone)] -pub struct ImportDirective { +pub struct ImportDirective<'a> { module_path: Vec, subclass: ImportDirectiveSubclass, span: Span, @@ -66,14 +66,14 @@ pub struct ImportDirective { is_prelude: bool, } -impl ImportDirective { +impl<'a> ImportDirective<'a> { pub fn new(module_path: Vec, subclass: ImportDirectiveSubclass, span: Span, id: NodeId, is_public: bool, is_prelude: bool) - -> ImportDirective { + -> Self { ImportDirective { module_path: module_path, subclass: subclass, @@ -86,9 +86,8 @@ impl ImportDirective { // Given the binding to which this directive resolves in a particular namespace, // this returns the binding for the name this directive defines in that namespace. - fn import<'a>(&self, - binding: &'a NameBinding<'a>, - privacy_error: Option>>) -> NameBinding<'a> { + fn import(&self, binding: &'a NameBinding<'a>, privacy_error: Option>>) + -> NameBinding<'a> { let mut modifiers = match self.is_public { true => DefModifiers::PUBLIC | DefModifiers::IMPORTABLE, false => DefModifiers::empty(), @@ -292,7 +291,7 @@ impl<'a> ::ModuleS<'a> { struct ImportResolvingError<'a> { /// Module where the error happened source_module: Module<'a>, - import_directive: &'a ImportDirective, + import_directive: &'a ImportDirective<'a>, span: Span, help: String, } @@ -424,7 +423,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// don't know whether the name exists at the moment due to other /// currently-unresolved imports, or success if we know the name exists. /// If successful, the resolved bindings are written into the module. - fn resolve_import(&mut self, directive: &'b ImportDirective) -> ResolveResult<()> { + fn resolve_import(&mut self, directive: &'b ImportDirective<'b>) -> ResolveResult<()> { debug!("(resolving import for module) resolving import `{}::...` in `{}`", names_to_string(&directive.module_path), module_to_string(self.resolver.current_module)); @@ -579,7 +578,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // succeeds or bails out (as importing * from an empty module or a module // that exports nothing is valid). target_module is the module we are // actually importing, i.e., `foo` in `use foo::*`. - fn resolve_glob_import(&mut self, target_module: Module<'b>, directive: &'b ImportDirective) + fn resolve_glob_import(&mut self, target_module: Module<'b>, directive: &'b ImportDirective<'b>) -> ResolveResult<()> { if let Some(Def::Trait(_)) = target_module.def { self.resolver.session.span_err(directive.span, "items in traits are not importable."); From b83739af4e4687469638f760bba60c9ad362ec34 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 17 Mar 2016 01:16:33 +0000 Subject: [PATCH 02/10] Add a field `target_module: Cell>` to `ImportDirective` --- src/librustc_resolve/resolve_imports.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index b446fa6430e8b..1e0048d12b5cf 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -59,6 +59,7 @@ impl ImportDirectiveSubclass { #[derive(Debug,Clone)] pub struct ImportDirective<'a> { module_path: Vec, + target_module: Cell>>, // the resolution of `module_path` subclass: ImportDirectiveSubclass, span: Span, id: NodeId, @@ -76,6 +77,7 @@ impl<'a> ImportDirective<'a> { -> Self { ImportDirective { module_path: module_path, + target_module: Cell::new(None), subclass: subclass, span: span, id: id, @@ -435,6 +437,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Indeterminate => return Indeterminate, Failed(err) => return Failed(err), }; + directive.target_module.set(Some(target_module)); let (source, target, value_determined, type_determined) = match directive.subclass { SingleImport { source, target, ref value_determined, ref type_determined } => From 0d1bc02da84db560729e5f6276759001b234bb6c Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 18 Mar 2016 00:57:05 +0000 Subject: [PATCH 03/10] Avoid recomputing the target module for an import directive. --- src/librustc_resolve/resolve_imports.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 1e0048d12b5cf..6e527545ece04 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -430,15 +430,18 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { names_to_string(&directive.module_path), module_to_string(self.resolver.current_module)); - let target_module = match self.resolver.resolve_module_path(&directive.module_path, - DontUseLexicalScope, - directive.span) { - Success(module) => module, - Indeterminate => return Indeterminate, - Failed(err) => return Failed(err), + let target_module = match directive.target_module.get() { + Some(module) => module, + _ => match self.resolver.resolve_module_path(&directive.module_path, + DontUseLexicalScope, + directive.span) { + Success(module) => module, + Indeterminate => return Indeterminate, + Failed(err) => return Failed(err), + }, }; - directive.target_module.set(Some(target_module)); + directive.target_module.set(Some(target_module)); let (source, target, value_determined, type_determined) = match directive.subclass { SingleImport { source, target, ref value_determined, ref type_determined } => (source, target, value_determined, type_determined), From 5ff21f138a1a807205180caf7921256e6dc16790 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 17 Mar 2016 11:05:09 +0000 Subject: [PATCH 04/10] Refactor ModuleS fields `public_glob_count`, `private_glob_count`, and `resolved_globs` into a single field `globs: RefCell>`. --- src/librustc_resolve/build_reduced_graph.rs | 7 --- src/librustc_resolve/lib.rs | 22 +-------- src/librustc_resolve/resolve_imports.rs | 53 +++++++++------------ 3 files changed, 25 insertions(+), 57 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 211bff352feef..2480fd02bea1d 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -538,13 +538,6 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { module_.increment_outstanding_references_for(target, ValueNS, is_public); module_.increment_outstanding_references_for(target, TypeNS, is_public); } - GlobImport if !is_prelude => { - // Set the glob flag. This tells us that we don't know the - // module's exports ahead of time. - module_.inc_glob_count(is_public) - } - // Prelude imports are not included in the glob counts since they do not get added to - // `resolved_globs` -- they are handled separately in `resolve_imports`. GlobImport => {} } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index be01880e2a915..cc229f1f16136 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -849,13 +849,7 @@ pub struct ModuleS<'a> { prelude: RefCell>>, glob_importers: RefCell, &'a ImportDirective<'a>)>>, - resolved_globs: RefCell<(Vec> /* public */, Vec> /* private */)>, - - // The number of public glob imports in this module. - public_glob_count: Cell, - - // The number of private glob imports in this module. - private_glob_count: Cell, + globs: RefCell>>, // Whether this module is populated. If not populated, any attempt to // access the children must be preceded with a @@ -883,19 +877,12 @@ impl<'a> ModuleS<'a> { module_children: RefCell::new(NodeMap()), prelude: RefCell::new(None), glob_importers: RefCell::new(Vec::new()), - resolved_globs: RefCell::new((Vec::new(), Vec::new())), - public_glob_count: Cell::new(0), - private_glob_count: Cell::new(0), + globs: RefCell::new((Vec::new())), populated: Cell::new(!external), arenas: arenas } } - fn add_import_directive(&self, import_directive: ImportDirective<'a>) { - let import_directive = self.arenas.alloc_import_directive(import_directive); - self.unresolved_imports.borrow_mut().push(import_directive); - } - fn for_each_child)>(&self, mut f: F) { for (&(name, ns), name_resolution) in self.resolutions.borrow().iter() { name_resolution.binding.map(|binding| f(name, ns, binding)); @@ -928,11 +915,6 @@ impl<'a> ModuleS<'a> { _ => false, } } - - fn inc_glob_count(&self, is_public: bool) { - let glob_count = if is_public { &self.public_glob_count } else { &self.private_glob_count }; - glob_count.set(glob_count.get() + 1); - } } impl<'a> fmt::Debug for ModuleS<'a> { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 6e527545ece04..86cfa928a9d2b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -212,29 +212,15 @@ impl<'a> ::ModuleS<'a> { }); } - let (ref mut public_globs, ref mut private_globs) = *self.resolved_globs.borrow_mut(); - - // Check if the public globs are determined - if public_globs.len() < self.public_glob_count.get() { - return Indeterminate; - } - for module in public_globs.iter() { - if let Indeterminate = module.resolve_name(name, ns, false) { - return Indeterminate; - } - } - - if !allow_private_imports { - return Failed(None); - } - - // Check if the private globs are determined - if private_globs.len() < self.private_glob_count.get() { - return Indeterminate; - } - for module in private_globs.iter() { - if let Indeterminate = module.resolve_name(name, ns, false) { - return Indeterminate; + // Check if the globs are determined + for directive in self.globs.borrow().iter() { + if !allow_private_imports && !directive.is_public { continue } + match directive.target_module.get() { + None => return Indeterminate, + Some(target_module) => match target_module.resolve_name(name, ns, false) { + Indeterminate => return Indeterminate, + _ => {} + } } } @@ -259,6 +245,18 @@ impl<'a> ::ModuleS<'a> { }) } + pub fn add_import_directive(&self, directive: ImportDirective<'a>) { + let directive = self.arenas.alloc_import_directive(directive); + self.unresolved_imports.borrow_mut().push(directive); + if let GlobImport = directive.subclass { + // We don't add prelude imports to the globs since they only affect lexical scopes, + // which are not relevant to import resolution. + if !directive.is_prelude { + self.globs.borrow_mut().push(directive); + } + } + } + pub fn increment_outstanding_references_for(&self, name: Name, ns: Namespace, is_public: bool) { self.resolutions.borrow_mut().entry((name, ns)).or_insert_with(Default::default) .increment_outstanding_references(is_public); @@ -603,12 +601,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return Success(()); } - // Add to target_module's glob_importers and module_'s resolved_globs + // Add to target_module's glob_importers target_module.glob_importers.borrow_mut().push((module_, directive)); - match *module_.resolved_globs.borrow_mut() { - (ref mut public_globs, _) if directive.is_public => public_globs.push(target_module), - (_, ref mut private_globs) => private_globs.push(target_module), - } for (&(name, ns), resolution) in target_module.resolutions.borrow().iter() { if let Some(Success(binding)) = resolution.try_result(false) { @@ -635,8 +629,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // reporting conflicts, reporting the PRIVATE_IN_PUBLIC lint, and reporting unresolved imports. fn finalize_resolutions(&mut self, module: Module<'b>, report_unresolved_imports: bool) { // Since import resolution is finished, globs will not define any more names. - module.public_glob_count.set(0); module.private_glob_count.set(0); - *module.resolved_globs.borrow_mut() = (Vec::new(), Vec::new()); + *module.globs.borrow_mut() = Vec::new(); let mut reexports = Vec::new(); for (&(name, ns), resolution) in module.resolutions.borrow().iter() { From bfb832e7c886a720e0aa847ffd8500621a3152d5 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 17 Mar 2016 08:43:17 +0000 Subject: [PATCH 05/10] Add `SingleImports` and use it in place of `outstanding_references` and `pub_outstanding_references`. --- src/librustc_resolve/build_reduced_graph.rs | 43 +---- src/librustc_resolve/resolve_imports.rs | 175 ++++++++++++-------- 2 files changed, 114 insertions(+), 104 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 2480fd02bea1d..10a8647906e35 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -14,8 +14,7 @@ //! any imports resolved. use DefModifiers; -use resolve_imports::ImportDirective; -use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport}; +use resolve_imports::ImportDirectiveSubclass::{self, GlobImport}; use Module; use Namespace::{self, TypeNS, ValueNS}; use {NameBinding, NameBindingKind}; @@ -28,7 +27,7 @@ use rustc::middle::def::*; use rustc::middle::def_id::{CRATE_DEF_INDEX, DefId}; use rustc::ty::VariantKind; -use syntax::ast::{Name, NodeId}; +use syntax::ast::Name; use syntax::attr::AttrMetaMethods; use syntax::parse::token::special_idents; use syntax::codemap::{Span, DUMMY_SP}; @@ -152,8 +151,8 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { } let subclass = ImportDirectiveSubclass::single(binding, source_name); - self.build_import_directive(parent, - module_path, + self.unresolved_imports += 1; + parent.add_import_directive(module_path, subclass, view_path.span, item.id, @@ -203,8 +202,8 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { } }; let subclass = ImportDirectiveSubclass::single(rename, name); - self.build_import_directive(parent, - module_path, + self.unresolved_imports += 1; + parent.add_import_directive(module_path, subclass, source_item.span, source_item.node.id(), @@ -213,8 +212,8 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { } } ViewPathGlob(_) => { - self.build_import_directive(parent, - module_path, + self.unresolved_imports += 1; + parent.add_import_directive(module_path, GlobImport, view_path.span, item.id, @@ -521,32 +520,6 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { } } - /// Creates and adds an import directive to the given module. - fn build_import_directive(&mut self, - module_: Module<'b>, - module_path: Vec, - subclass: ImportDirectiveSubclass, - span: Span, - id: NodeId, - is_public: bool, - is_prelude: bool) { - // Bump the reference count on the name. Or, if this is a glob, set - // the appropriate flag. - - match subclass { - SingleImport { target, .. } => { - module_.increment_outstanding_references_for(target, ValueNS, is_public); - module_.increment_outstanding_references_for(target, TypeNS, is_public); - } - GlobImport => {} - } - - let directive = - ImportDirective::new(module_path, subclass, span, id, is_public, is_prelude); - module_.add_import_directive(directive); - self.unresolved_imports += 1; - } - /// Ensures that the reduced graph rooted at the given external module /// is built, building it if it is not. pub fn populate_module_if_necessary(&mut self, module: Module<'b>) { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 86cfa928a9d2b..a13ecc26ad807 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -68,24 +68,6 @@ pub struct ImportDirective<'a> { } impl<'a> ImportDirective<'a> { - pub fn new(module_path: Vec, - subclass: ImportDirectiveSubclass, - span: Span, - id: NodeId, - is_public: bool, - is_prelude: bool) - -> Self { - ImportDirective { - module_path: module_path, - target_module: Cell::new(None), - subclass: subclass, - span: span, - id: id, - is_public: is_public, - is_prelude: is_prelude, - } - } - // Given the binding to which this directive resolves in a particular namespace, // this returns the binding for the name this directive defines in that namespace. fn import(&self, binding: &'a NameBinding<'a>, privacy_error: Option>>) @@ -111,17 +93,52 @@ impl<'a> ImportDirective<'a> { } #[derive(Clone, Default)] -/// Records information about the resolution of a name in a module. +/// Records information about the resolution of a name in a namespace of a module. pub struct NameResolution<'a> { - /// The number of unresolved single imports of any visibility that could define the name. - outstanding_references: u32, - /// The number of unresolved `pub` single imports that could define the name. - pub_outstanding_references: u32, + /// The single imports that define the name in the namespace. + single_imports: SingleImports<'a>, /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>, duplicate_globs: Vec<&'a NameBinding<'a>>, } +#[derive(Clone, Debug)] +enum SingleImports<'a> { + /// No single imports can define the name in the namespace. + None, + /// Only the given single import can define the name in the namespace. + MaybeOne(&'a ImportDirective<'a>), + /// At least one single import will define the name in the namespace. + AtLeastOne, +} + +impl<'a> Default for SingleImports<'a> { + fn default() -> Self { + SingleImports::None + } +} + +impl<'a> SingleImports<'a> { + fn add_directive(&mut self, directive: &'a ImportDirective<'a>) { + match *self { + SingleImports::None => *self = SingleImports::MaybeOne(directive), + // If two single imports can define the name in the namespace, we can assume that at + // least one of them will define it since otherwise both would have to define only one + // namespace, leading to a duplicate error. + SingleImports::MaybeOne(_) => *self = SingleImports::AtLeastOne, + SingleImports::AtLeastOne => {} + }; + } + + fn directive_failed(&mut self) { + match *self { + SingleImports::None => unreachable!(), + SingleImports::MaybeOne(_) => *self = SingleImports::None, + SingleImports::AtLeastOne => {} + } + } +} + impl<'a> NameResolution<'a> { fn try_define(&mut self, binding: &'a NameBinding<'a>) -> Result<(), &'a NameBinding<'a>> { if let Some(old_binding) = self.binding { @@ -140,40 +157,43 @@ impl<'a> NameResolution<'a> { Ok(()) } + // Returns the binding for the name if it is known or None if it not known. + fn binding(&self) -> Option<&'a NameBinding<'a>> { + self.binding.and_then(|binding| match self.single_imports { + SingleImports::None => Some(binding), + _ if !binding.defined_with(DefModifiers::GLOB_IMPORTED) => Some(binding), + _ => None, // The binding could be shadowed by a single import, so it is not known. + }) + } + // Returns Some(the resolution of the name), or None if the resolution depends // on whether more globs can define the name. fn try_result(&self, allow_private_imports: bool) -> Option>> { match self.binding { Some(binding) if !binding.defined_with(DefModifiers::GLOB_IMPORTED) => - Some(Success(binding)), - // If (1) we don't allow private imports, (2) no public single import can define the - // name, and (3) no public glob has defined the name, the resolution depends on globs. - _ if !allow_private_imports && self.pub_outstanding_references == 0 && - !self.binding.map(NameBinding::is_public).unwrap_or(false) => None, - _ if self.outstanding_references > 0 => Some(Indeterminate), - Some(binding) => Some(Success(binding)), - None => None, - } - } - - fn increment_outstanding_references(&mut self, is_public: bool) { - self.outstanding_references += 1; - if is_public { - self.pub_outstanding_references += 1; - } - } - - fn decrement_outstanding_references(&mut self, is_public: bool) { - let decrement_references = |count: &mut _| { - assert!(*count > 0); - *count -= 1; + return Some(Success(binding)), + _ => {} // Items and single imports are not shadowable }; - decrement_references(&mut self.outstanding_references); - if is_public { - decrement_references(&mut self.pub_outstanding_references); + // Check if a single import can still define the name. + match self.single_imports { + SingleImports::None => {}, + SingleImports::AtLeastOne => return Some(Indeterminate), + SingleImports::MaybeOne(directive) => { + // If (1) we don't allow private imports, (2) no public single import can define + // the name, and (3) no public glob has defined the name, the resolution depends + // on whether more globs can define the name. + if !allow_private_imports && !directive.is_public && + !self.binding.map(NameBinding::is_public).unwrap_or(false) { + return None; + } + + return Indeterminate; + } } + + self.binding.map(Success) } fn report_conflicts(&self, mut report: F) { @@ -245,23 +265,39 @@ impl<'a> ::ModuleS<'a> { }) } - pub fn add_import_directive(&self, directive: ImportDirective<'a>) { - let directive = self.arenas.alloc_import_directive(directive); + pub fn add_import_directive(&self, + module_path: Vec, + subclass: ImportDirectiveSubclass, + span: Span, + id: NodeId, + is_public: bool, + is_prelude: bool) { + let directive = self.arenas.alloc_import_directive(ImportDirective { + module_path: module_path, + target_module: Cell::new(None), + subclass: subclass, + span: span, + id: id, + is_public: is_public, + is_prelude: is_prelude, + }); + self.unresolved_imports.borrow_mut().push(directive); - if let GlobImport = directive.subclass { + match directive.subclass { + SingleImport { target, .. } => { + let mut resolutions = self.resolutions.borrow_mut(); + for &ns in &[ValueNS, TypeNS] { + resolutions.entry((target, ns)).or_insert_with(Default::default) + .single_imports.add_directive(directive); + } + } // We don't add prelude imports to the globs since they only affect lexical scopes, // which are not relevant to import resolution. - if !directive.is_prelude { - self.globs.borrow_mut().push(directive); - } + GlobImport if directive.is_prelude => {} + GlobImport => self.globs.borrow_mut().push(directive), } } - pub fn increment_outstanding_references_for(&self, name: Name, ns: Namespace, is_public: bool) { - self.resolutions.borrow_mut().entry((name, ns)).or_insert_with(Default::default) - .increment_outstanding_references(is_public); - } - // Use `update` to mutate the resolution for the name. // If the resolution becomes a success, define it in the module's glob importers. fn update_resolution(&self, name: Name, ns: Namespace, update: F) -> T @@ -269,11 +305,11 @@ impl<'a> ::ModuleS<'a> { { let mut resolutions = self.resolutions.borrow_mut(); let resolution = resolutions.entry((name, ns)).or_insert_with(Default::default); - let was_success = resolution.try_result(false).and_then(ResolveResult::success).is_some(); + let was_known = resolution.binding().is_some(); let t = update(resolution); - if !was_success { - if let Some(Success(binding)) = resolution.try_result(false) { + if !was_known { + if let Some(binding) = resolution.binding() { self.define_in_glob_importers(name, ns, binding); } } @@ -454,12 +490,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // (as opposed to being indeterminate) when it can only be defined by the directive. if !determined { module_.resolutions.borrow_mut().get_mut(&(target, ns)).unwrap() - .decrement_outstanding_references(directive.is_public); + .single_imports.directive_failed(); } let result = self.resolver.resolve_name_in_module(target_module, source, ns, false, true); if !determined { - module_.increment_outstanding_references_for(target, ns, directive.is_public) + module_.resolutions.borrow_mut().get_mut(&(target, ns)).unwrap() + .single_imports.add_directive(directive); } result }; @@ -491,11 +528,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let binding = &directive.import(binding, None); self.resolver.report_conflict(module_, target, ns, binding, old_binding); } + } else { + module_.update_resolution(target, ns, |resolution| { + resolution.single_imports.directive_failed(); + }); } - - module_.update_resolution(target, ns, |resolution| { - resolution.decrement_outstanding_references(directive.is_public); - }) } match (&value_result, &type_result) { @@ -605,7 +642,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { target_module.glob_importers.borrow_mut().push((module_, directive)); for (&(name, ns), resolution) in target_module.resolutions.borrow().iter() { - if let Some(Success(binding)) = resolution.try_result(false) { + if let Some(binding) = resolution.binding() { if binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) { let _ = module_.try_define_child(name, ns, directive.import(binding, None)); } From 85a12095708850031cf9962e2bbd8cb9f4920d9f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 18 Mar 2016 00:05:08 +0000 Subject: [PATCH 06/10] Improve import failure detection --- src/librustc_resolve/resolve_imports.rs | 42 +++++++++++-------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index a13ecc26ad807..ae78f69785105 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -168,7 +168,7 @@ impl<'a> NameResolution<'a> { // Returns Some(the resolution of the name), or None if the resolution depends // on whether more globs can define the name. - fn try_result(&self, allow_private_imports: bool) + fn try_result(&self, ns: Namespace, allow_private_imports: bool) -> Option>> { match self.binding { Some(binding) if !binding.defined_with(DefModifiers::GLOB_IMPORTED) => @@ -189,7 +189,18 @@ impl<'a> NameResolution<'a> { return None; } - return Indeterminate; + let target_module = match directive.target_module.get() { + Some(target_module) => target_module, + None => return Some(Indeterminate), + }; + let name = match directive.subclass { + SingleImport { source, target, .. } if source == target => target, + _ => return Some(Indeterminate), + }; + match target_module.resolve_name(name, ns, false) { + Failed(_) => {} + _ => return Some(Indeterminate), + } } } @@ -224,7 +235,7 @@ impl<'a> ::ModuleS<'a> { }; let resolution = resolutions.get(&(name, ns)).cloned().unwrap_or_default(); - if let Some(result) = resolution.try_result(allow_private_imports) { + if let Some(result) = resolution.try_result(ns, allow_private_imports) { // If the resolution doesn't depend on glob definability, check privacy and return. return result.and_then(|binding| { let allowed = allow_private_imports || !binding.is_import() || binding.is_public(); @@ -483,27 +494,12 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; // We need to resolve both namespaces for this to succeed. - let module_ = self.resolver.current_module; - let (value_result, type_result) = { - let mut resolve_in_ns = |ns, determined: bool| { - // Temporarily count the directive as determined so that the resolution fails - // (as opposed to being indeterminate) when it can only be defined by the directive. - if !determined { - module_.resolutions.borrow_mut().get_mut(&(target, ns)).unwrap() - .single_imports.directive_failed(); - } - let result = - self.resolver.resolve_name_in_module(target_module, source, ns, false, true); - if !determined { - module_.resolutions.borrow_mut().get_mut(&(target, ns)).unwrap() - .single_imports.add_directive(directive); - } - result - }; - (resolve_in_ns(ValueNS, value_determined.get()), - resolve_in_ns(TypeNS, type_determined.get())) - }; + let value_result = + self.resolver.resolve_name_in_module(target_module, source, ValueNS, false, true); + let type_result = + self.resolver.resolve_name_in_module(target_module, source, TypeNS, false, true); + let module_ = self.resolver.current_module; for &(ns, result, determined) in &[(ValueNS, &value_result, value_determined), (TypeNS, &type_result, type_determined)] { if determined.get() { continue } From 62f44957bf74f9127be7cd18c494fd98d9ef6d85 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 18 Mar 2016 00:11:52 +0000 Subject: [PATCH 07/10] Add test for #32119 --- src/test/compile-fail/issue-32119.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 src/test/compile-fail/issue-32119.rs diff --git a/src/test/compile-fail/issue-32119.rs b/src/test/compile-fail/issue-32119.rs new file mode 100644 index 0000000000000..226a3a24c9bd8 --- /dev/null +++ b/src/test/compile-fail/issue-32119.rs @@ -0,0 +1,21 @@ +// 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(rustc_attrs)] + +pub type T = (); +mod foo { pub use super::T; } +mod bar { pub use super::T; } + +pub use foo::*; +pub use bar::*; + +#[rustc_error] +fn main() {} //~ ERROR compilation successful From 26e38d2fffbcf7fc39d733686a1277291b54535d Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 18 Mar 2016 06:06:07 +0000 Subject: [PATCH 08/10] Remove the test for #32089; it is subsumed by the test for #32119 --- src/test/compile-fail/issue-32089.rs | 23 ----------------------- 1 file changed, 23 deletions(-) delete mode 100644 src/test/compile-fail/issue-32089.rs diff --git a/src/test/compile-fail/issue-32089.rs b/src/test/compile-fail/issue-32089.rs deleted file mode 100644 index 5da7b9fff6e9a..0000000000000 --- a/src/test/compile-fail/issue-32089.rs +++ /dev/null @@ -1,23 +0,0 @@ -// 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(rustc_attrs)] -#![allow(unused_imports)] - -pub type Type = i32; - -mod one { use super::Type; } -pub use self::one::*; - -mod two { use super::Type; } -pub use self::two::*; - -#[rustc_error] -fn main() {} //~ ERROR compilation successful From 3d9db595661a96c37f4a5f9cf310c20da08efa5f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 30 Mar 2016 22:21:56 +0000 Subject: [PATCH 09/10] Detect cycles that include renamed imports --- src/librustc_resolve/lib.rs | 9 +++++-- src/librustc_resolve/resolve_imports.rs | 32 ++++++++++++++----------- src/test/compile-fail/issue-32119.rs | 7 ++++++ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index cc229f1f16136..fc13ee11ae1a5 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -827,7 +827,7 @@ pub struct ModuleS<'a> { // is the NodeId of the local `extern crate` item (otherwise, `extern_crate_id` is None). extern_crate_id: Option, - resolutions: RefCell>>, + resolutions: RefCell>>>, unresolved_imports: RefCell>>, // The module children of this node, including normal modules and anonymous modules. @@ -885,7 +885,7 @@ impl<'a> ModuleS<'a> { fn for_each_child)>(&self, mut f: F) { for (&(name, ns), name_resolution) in self.resolutions.borrow().iter() { - name_resolution.binding.map(|binding| f(name, ns, binding)); + name_resolution.borrow().binding.map(|binding| f(name, ns, binding)); } } @@ -1117,6 +1117,7 @@ struct ResolverArenas<'a> { modules: arena::TypedArena>, name_bindings: arena::TypedArena>, import_directives: arena::TypedArena>, + name_resolutions: arena::TypedArena>>, } impl<'a> ResolverArenas<'a> { @@ -1130,6 +1131,9 @@ impl<'a> ResolverArenas<'a> { -> &'a ImportDirective { self.import_directives.alloc(import_directive) } + fn alloc_name_resolution(&'a self) -> &'a RefCell> { + self.name_resolutions.alloc(Default::default()) + } } #[derive(PartialEq)] @@ -1198,6 +1202,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { modules: arena::TypedArena::new(), name_bindings: arena::TypedArena::new(), import_directives: arena::TypedArena::new(), + name_resolutions: arena::TypedArena::new(), } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index ae78f69785105..2f7de8c4aa477 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -30,7 +30,7 @@ use syntax::codemap::Span; use syntax::util::lev_distance::find_best_match_for_name; use std::mem::replace; -use std::cell::Cell; +use std::cell::{Cell, RefCell}; /// Contains data for specific types of import directives. #[derive(Clone, Debug)] @@ -194,8 +194,8 @@ impl<'a> NameResolution<'a> { None => return Some(Indeterminate), }; let name = match directive.subclass { - SingleImport { source, target, .. } if source == target => target, - _ => return Some(Indeterminate), + SingleImport { source, .. } => source, + GlobImport => unreachable!(), }; match target_module.resolve_name(name, ns, false) { Failed(_) => {} @@ -227,14 +227,19 @@ impl<'a> NameResolution<'a> { } impl<'a> ::ModuleS<'a> { + fn resolution(&self, name: Name, ns: Namespace) -> &'a RefCell> { + *self.resolutions.borrow_mut().entry((name, ns)) + .or_insert_with(|| self.arenas.alloc_name_resolution()) + } + pub fn resolve_name(&self, name: Name, ns: Namespace, allow_private_imports: bool) -> ResolveResult<&'a NameBinding<'a>> { - let resolutions = match self.resolutions.borrow_state() { - ::std::cell::BorrowState::Unused => self.resolutions.borrow(), - _ => return Failed(None), // This happens when there is a cycle of glob imports + let resolution = self.resolution(name, ns); + let resolution = match resolution.borrow_state() { + ::std::cell::BorrowState::Unused => resolution.borrow_mut(), + _ => return Failed(None), // This happens when there is a cycle of imports }; - let resolution = resolutions.get(&(name, ns)).cloned().unwrap_or_default(); if let Some(result) = resolution.try_result(ns, allow_private_imports) { // If the resolution doesn't depend on glob definability, check privacy and return. return result.and_then(|binding| { @@ -261,7 +266,7 @@ impl<'a> ::ModuleS<'a> { // Invariant: this may not be called until import resolution is complete. pub fn resolve_name_in_lexical_scope(&self, name: Name, ns: Namespace) -> Option<&'a NameBinding<'a>> { - self.resolutions.borrow().get(&(name, ns)).and_then(|resolution| resolution.binding) + self.resolution(name, ns).borrow().binding .or_else(|| self.prelude.borrow().and_then(|prelude| { prelude.resolve_name(name, ns, false).success() })) @@ -296,10 +301,9 @@ impl<'a> ::ModuleS<'a> { self.unresolved_imports.borrow_mut().push(directive); match directive.subclass { SingleImport { target, .. } => { - let mut resolutions = self.resolutions.borrow_mut(); for &ns in &[ValueNS, TypeNS] { - resolutions.entry((target, ns)).or_insert_with(Default::default) - .single_imports.add_directive(directive); + self.resolution(target, ns).borrow_mut().single_imports + .add_directive(directive); } } // We don't add prelude imports to the globs since they only affect lexical scopes, @@ -314,8 +318,7 @@ impl<'a> ::ModuleS<'a> { fn update_resolution(&self, name: Name, ns: Namespace, update: F) -> T where F: FnOnce(&mut NameResolution<'a>) -> T { - let mut resolutions = self.resolutions.borrow_mut(); - let resolution = resolutions.entry((name, ns)).or_insert_with(Default::default); + let mut resolution = &mut *self.resolution(name, ns).borrow_mut(); let was_known = resolution.binding().is_some(); let t = update(resolution); @@ -638,7 +641,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { target_module.glob_importers.borrow_mut().push((module_, directive)); for (&(name, ns), resolution) in target_module.resolutions.borrow().iter() { - if let Some(binding) = resolution.binding() { + if let Some(binding) = resolution.borrow().binding() { if binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) { let _ = module_.try_define_child(name, ns, directive.import(binding, None)); } @@ -666,6 +669,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let mut reexports = Vec::new(); for (&(name, ns), resolution) in module.resolutions.borrow().iter() { + let resolution = resolution.borrow(); resolution.report_conflicts(|b1, b2| { self.resolver.report_conflict(module, name, ns, b1, b2) }); diff --git a/src/test/compile-fail/issue-32119.rs b/src/test/compile-fail/issue-32119.rs index 226a3a24c9bd8..4743b779ef63e 100644 --- a/src/test/compile-fail/issue-32119.rs +++ b/src/test/compile-fail/issue-32119.rs @@ -17,5 +17,12 @@ mod bar { pub use super::T; } pub use foo::*; pub use bar::*; +mod baz { + pub type T = (); + mod foo { pub use super::T as S; } + mod bar { pub use super::foo::S as T; } + pub use self::bar::*; +} + #[rustc_error] fn main() {} //~ ERROR compilation successful From 6f09deaa32ba8a4f26a46c28ffecbb3efc3d165e Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 31 Mar 2016 04:44:04 +0000 Subject: [PATCH 10/10] Fix suggestions --- src/librustc_resolve/resolve_imports.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 2f7de8c4aa477..2aa8925fb54b5 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -537,8 +537,14 @@ 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.resolutions.borrow(); - let names = children.keys().map(|&(ref name, _)| name); + let resolutions = target_module.resolutions.borrow(); + let names = resolutions.iter().filter_map(|(&(ref name, _), resolution)| { + match *resolution.borrow() { + NameResolution { binding: Some(_), .. } => Some(name), + NameResolution { single_imports: SingleImports::None, .. } => None, + _ => Some(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(),