From 661b7ce830c1d3313f0fd4456c0f55583711730c Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 19:29:51 +0000 Subject: [PATCH 01/12] Make resolve_name_in_module solely responsible for tracking used crates in lib.rs --- src/librustc_resolve/lib.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 054aa1d5f555b..3fcf14bbbcd7f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1202,10 +1202,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { fn record_import_use(&mut self, name: Name, ns: Namespace, resolution: &ImportResolution<'a>) { let import_id = resolution.id; self.used_imports.insert((import_id, ns)); - match resolution.target.as_ref().and_then(|target| target.target_module.def_id()) { - Some(DefId { krate, .. }) => { self.used_crates.insert(krate); } - _ => {} - }; if !self.make_glob_map { return; @@ -1299,11 +1295,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Check to see whether there are type bindings, and, if // so, whether there is a module within. if let Some(module_def) = target.binding.module() { - // track extern crates for unused_extern_crate lint - if let Some(did) = module_def.def_id() { - self.used_crates.insert(did.krate); - } - search_module = module_def; // Keep track of the closest private module used @@ -1573,6 +1564,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { if let Some(binding) = module_.get_child(name, namespace) { debug!("(resolving name in module) found node as child"); + if binding.is_extern_crate() { + // track the extern crate as used. + if let Some(DefId { krate, .. }) = binding.module().unwrap().def_id() { + self.used_crates.insert(krate); + } + } return Success((Target::new(module_, binding, Shadowable::Never), false)); } @@ -2923,9 +2920,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } _ => return None, }; - if let Some(DefId{krate: kid, ..}) = containing_module.def_id() { - self.used_crates.insert(kid); - } return Some(def); } From 7366d105cb29100c6522619abbabe57777240778 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 21:04:01 +0000 Subject: [PATCH 02/12] Refactor away Target --- src/librustc_resolve/lib.rs | 56 ++++++------ src/librustc_resolve/resolve_imports.rs | 114 ++++++++++-------------- 2 files changed, 75 insertions(+), 95 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 3fcf14bbbcd7f..b2f60ddb8da61 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -92,8 +92,7 @@ use std::cell::{Cell, RefCell}; use std::fmt; use std::mem::replace; -use resolve_imports::{Target, ImportDirective, ImportResolution}; -use resolve_imports::Shadowable; +use resolve_imports::{ImportDirective, ImportResolution}; // NB: This module needs to be declared first so diagnostics are // registered before they are used. @@ -951,6 +950,7 @@ bitflags! { // Variants are considered `PUBLIC`, but some of them live in private enums. // We need to track them to prohibit reexports like `pub use PrivEnum::Variant`. const PRIVATE_VARIANT = 1 << 2, + const PRELUDE = 1 << 3, } } @@ -1291,10 +1291,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { name); return Indeterminate; } - Success((target, used_proxy)) => { + Success((binding, used_proxy)) => { // Check to see whether there are type bindings, and, if // so, whether there is a module within. - if let Some(module_def) = target.binding.module() { + if let Some(module_def) = binding.module() { search_module = module_def; // Keep track of the closest private module used @@ -1390,7 +1390,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving module path for import) indeterminate; bailing"); return Indeterminate; } - Success((target, _)) => match target.binding.module() { + Success((binding, _)) => match binding.module() { Some(containing_module) => { search_module = containing_module; start_index = 1; @@ -1424,7 +1424,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { name: Name, namespace: Namespace, record_used: bool) - -> ResolveResult<(Target<'a>, bool)> { + -> ResolveResult<(NameBinding<'a>, bool)> { debug!("(resolving item in lexical scope) resolving `{}` in namespace {:?} in `{}`", name, namespace, @@ -1446,10 +1446,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving item in lexical scope) indeterminate higher scope; bailing"); return Indeterminate; } - Success((target, used_reexport)) => { + Success((binding, used_reexport)) => { // We found the module. debug!("(resolving item in lexical scope) found name in module, done"); - return Success((target, used_reexport)); + return Success((binding, used_reexport)); } } @@ -1543,7 +1543,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } /// Attempts to resolve the supplied name in the given module for the - /// given namespace. If successful, returns the target corresponding to + /// given namespace. If successful, returns the binding corresponding to /// the name. /// /// The boolean returned on success is an indicator of whether this lookup @@ -1554,7 +1554,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { namespace: Namespace, allow_private_imports: bool, record_used: bool) - -> ResolveResult<(Target<'a>, bool)> { + -> ResolveResult<(NameBinding<'a>, bool)> { debug!("(resolving name in module) resolving `{}` in `{}`", name, module_to_string(&*module_)); @@ -1570,7 +1570,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { self.used_crates.insert(krate); } } - return Success((Target::new(module_, binding, Shadowable::Never), false)); + return Success((binding, false)); } // Check the list of resolved imports. @@ -1580,12 +1580,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving name in module) import unresolved; bailing out"); return Indeterminate; } - if let Some(target) = import_resolution.target.clone() { + if let Some(binding) = import_resolution.binding.clone() { debug!("(resolving name in module) resolved to import"); if record_used { self.record_import_use(name, namespace, &import_resolution); } - return Success((target, true)); + return Success((binding, true)); } } Some(..) | None => {} // Continue. @@ -2616,11 +2616,11 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { -> BareIdentifierPatternResolution { let module = self.current_module; match self.resolve_item_in_lexical_scope(module, name, ValueNS, true) { - Success((target, _)) => { + Success((binding, _)) => { debug!("(resolve bare identifier pattern) succeeded in finding {} at {:?}", name, - &target.binding); - match target.binding.def() { + &binding); + match binding.def() { None => { panic!("resolved name in the value namespace to a set of name bindings \ with no def?!"); @@ -2776,7 +2776,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let module = self.current_module; let name = identifier.unhygienic_name; match self.resolve_item_in_lexical_scope(module, name, namespace, record_used) { - Success((target, _)) => target.binding.def().map(LocalDef::from_def), + Success((binding, _)) => binding.def().map(LocalDef::from_def), Failed(Some((span, msg))) => { resolve_error(self, span, ResolutionError::FailedToResolve(&*msg)); None @@ -2914,7 +2914,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let name = segments.last().unwrap().identifier.name; let result = self.resolve_name_in_module(containing_module, name, namespace, false, true); let def = match result { - Success((Target { binding, .. }, _)) => { + Success((binding, _)) => { let (def, lp) = binding.def_and_lp(); (def, last_private.or(lp)) } @@ -2970,7 +2970,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let name = segments.last().unwrap().identifier.name; match self.resolve_name_in_module(containing_module, name, namespace, false, true) { - Success((Target { binding, .. }, _)) => { + Success((binding, _)) => { let (def, lp) = binding.def_and_lp(); Some((def, last_private.or(lp))) } @@ -3008,12 +3008,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } if let AnonymousModuleRibKind(module) = self.get_ribs(namespace)[i].kind { - if let Success((target, _)) = self.resolve_name_in_module(module, - ident.unhygienic_name, - namespace, - true, - true) { - if let Some(def) = target.binding.def() { + if let Success((binding, _)) = self.resolve_name_in_module(module, + ident.unhygienic_name, + namespace, + true, + true) { + if let Some(def) = binding.def() { return Some(LocalDef::from_def(def)); } } @@ -3455,11 +3455,11 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Look for imports. for (&(_, ns), import) in search_module.import_resolutions.borrow().iter() { if ns != TypeNS { continue } - let target = match import.target { - Some(ref target) => target, + let binding = match import.binding { + Some(ref binding) => binding, None => continue, }; - let did = match target.binding.def() { + let did = match binding.def() { Some(Def::Trait(trait_def_id)) => trait_def_id, Some(..) | None => continue, }; diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 3d2300e44c468..882531efbb716 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -78,24 +78,14 @@ impl ImportDirective { shadowable: shadowable, } } -} - -/// The item that an import resolves to. -#[derive(Clone,Debug)] -pub struct Target<'a> { - pub target_module: Module<'a>, - pub binding: NameBinding<'a>, - pub shadowable: Shadowable, -} -impl<'a> Target<'a> { - pub fn new(target_module: Module<'a>, binding: NameBinding<'a>, shadowable: Shadowable) - -> Self { - Target { - target_module: target_module, - binding: binding, - shadowable: shadowable, + // 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, mut binding: NameBinding<'a>) -> NameBinding<'a> { + if self.shadowable == Shadowable::Always { + binding.modifiers = binding.modifiers | DefModifiers::PRELUDE; } + binding } } @@ -117,7 +107,7 @@ pub struct ImportResolution<'a> { pub is_public: bool, /// Resolution of the name in the namespace - pub target: Option>, + pub binding: Option>, /// The source node of the `use` directive pub id: NodeId, @@ -128,14 +118,16 @@ impl<'a> ImportResolution<'a> { ImportResolution { outstanding_references: 0, id: id, - target: None, + binding: None, is_public: is_public, } } pub fn shadowable(&self) -> Shadowable { - match self.target { - Some(ref target) => target.shadowable, + match self.binding { + Some(ref binding) if binding.defined_with(DefModifiers::PRELUDE) => + Shadowable::Always, + Some(_) => Shadowable::Never, None => Shadowable::Always, } } @@ -217,23 +209,17 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { e.import_directive.is_public) }); - if resolution.target.is_none() { + if resolution.binding.is_none() { debug!("(resolving import error) adding fake target to import resolution of `{}`", target); - let name_binding = NameBinding { + let dummy_binding = NameBinding { modifiers: DefModifiers::IMPORTABLE, def_or_module: DefOrModule::Def(Def::Err), span: None, }; - // Create a fake target pointing to a fake name binding in our - // own module - let target = Target::new(e.source_module, - name_binding, - Shadowable::Always); - - resolution.target = Some(target); + resolution.binding = Some(dummy_binding); } } @@ -374,14 +360,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } /// Resolves the name in the namespace of the module because it is being imported by - /// importing_module. Returns the module in which the name was defined (as opposed to imported), - /// the name bindings defining the name, and whether or not the name was imported into `module`. + /// importing_module. Returns the name bindings defining the name + /// and whether or not the name was imported. fn resolve_name_in_module(&mut self, module: Module<'b>, // Module containing the name name: Name, ns: Namespace, importing_module: Module<'b>) // Module importing the name - -> (ResolveResult<(Module<'b>, NameBinding<'b>)>, bool) { + -> (ResolveResult>, bool) { build_reduced_graph::populate_module_if_necessary(self.resolver, module); if let Some(name_binding) = module.get_child(name, ns) { if name_binding.is_extern_crate() { @@ -390,7 +376,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { self.resolver.used_crates.insert(krate); } } - return (Success((module, name_binding)), false) + return (Success(name_binding), false) } // If there is an unresolved glob at this point in the containing module, bail out. @@ -411,10 +397,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return (Failed(None), false); } - let target = resolution.target.clone(); - if let Some(Target { target_module, binding, shadowable: _ }) = target { + if let Some(binding) = resolution.binding.clone() { self.resolver.record_import_use(name, ns, &resolution); - (Success((target_module, binding)), true) + (Success(binding), true) } else { (Failed(None), false) } @@ -470,9 +455,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { self.resolve_name_in_module(target_module, source, TypeNS, module_); match (&value_result, &type_result) { - (&Success((_, ref name_binding)), _) if !value_used_reexport && - directive.is_public && - !name_binding.is_public() => { + (&Success(ref name_binding), _) if !value_used_reexport && + directive.is_public && + !name_binding.is_public() => { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = format!("Consider marking `{}` as `pub` in the imported module", source); @@ -481,8 +466,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { .emit(); } - (_, &Success((_, ref name_binding))) if !type_used_reexport && - directive.is_public => { + (_, &Success(ref name_binding)) if !type_used_reexport && + directive.is_public => { if !name_binding.is_public() { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = @@ -534,7 +519,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { { let mut check_and_write_import = |namespace, result, used_public: &mut bool| { - let result: &ResolveResult<(Module<'b>, NameBinding)> = result; + let result: &ResolveResult = result; let import_resolution = import_resolutions.get_mut(&(target, namespace)).unwrap(); let namespace_name = match namespace { @@ -543,7 +528,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; match *result { - Success((ref target_module, ref name_binding)) => { + Success(ref name_binding) => { debug!("(resolving single import) found {:?} target: {:?}", namespace_name, name_binding.def()); @@ -556,9 +541,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { directive.span, target); - import_resolution.target = Some(Target::new(target_module, - name_binding.clone(), - directive.shadowable)); + import_resolution.binding = Some(directive.import(name_binding.clone())); import_resolution.id = directive.id; import_resolution.is_public = directive.is_public; @@ -600,8 +583,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // 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. - import_resolution_value.target.as_ref().map(|target| { - let def = target.binding.def().unwrap(); + import_resolution_value.binding.as_ref().map(|binding| { + let def = binding.def().unwrap(); let last_private = if value_used_public { lp } else { DependsOn(def.def_id()) }; (def, last_private) }) @@ -612,8 +595,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { assert!(import_resolution_type.outstanding_references >= 1); import_resolution_type.outstanding_references -= 1; - import_resolution_type.target.as_ref().map(|target| { - let def = target.binding.def().unwrap(); + import_resolution_type.binding.as_ref().map(|binding| { + let def = binding.def().unwrap(); let last_private = if type_used_public { lp } else { DependsOn(def.def_id()) }; (def, last_private) }) @@ -695,15 +678,15 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { import_resolutions.entry((name, ns)) .or_insert_with(|| ImportResolution::new(id, is_public)); - match target_import_resolution.target { - Some(ref target) if target_import_resolution.is_public => { + match target_import_resolution.binding { + Some(ref binding) if target_import_resolution.is_public => { self.check_for_conflicting_import(&dest_import_resolution, import_directive.span, name, ns); dest_import_resolution.id = id; dest_import_resolution.is_public = is_public; - dest_import_resolution.target = Some(target.clone()); + dest_import_resolution.binding = Some(import_directive.import(binding.clone())); self.add_export(module_, name, &dest_import_resolution); } _ => {} @@ -778,10 +761,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { name); span_err!(self.resolver.session, import_directive.span, E0251, "{}", msg); } else { - let target = Target::new(containing_module, - name_binding.clone(), - import_directive.shadowable); - dest_import_resolution.target = Some(target); + dest_import_resolution.binding = Some(import_directive.import(name_binding.clone())); dest_import_resolution.id = id; dest_import_resolution.is_public = is_public; self.add_export(module_, name, &dest_import_resolution); @@ -800,7 +780,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some(def_id) => self.resolver.ast_map.as_local_node_id(def_id).unwrap(), None => return, }; - let export = match resolution.target.as_ref().unwrap().binding.def() { + let export = match resolution.binding.as_ref().unwrap().def() { Some(def) => Export { name: name, def_id: def.def_id() }, None => return, }; @@ -813,16 +793,16 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { import_span: Span, name: Name, namespace: Namespace) { - let target = &import_resolution.target; + let binding = &import_resolution.binding; debug!("check_for_conflicting_import: {}; target exists: {}", name, - target.is_some()); + binding.is_some()); - match *target { - Some(ref target) if target.shadowable != Shadowable::Always => { + match *binding { + Some(ref binding) if !binding.defined_with(DefModifiers::PRELUDE) => { let ns_word = match namespace { TypeNS => { - match target.binding.module() { + match binding.module() { Some(ref module) if module.is_normal() => "module", Some(ref module) if module.is_trait() => "trait", _ => "type", @@ -876,8 +856,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; if ns == ValueNS { - match import.target { - Some(ref target) if target.shadowable != Shadowable::Always => { + match import.binding { + Some(ref binding) if !binding.defined_with(DefModifiers::PRELUDE) => { let mut err = struct_span_err!(self.resolver.session, import_span, E0255, @@ -892,8 +872,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some(_) | None => {} } } else { - match import.target { - Some(ref target) if target.shadowable != Shadowable::Always => { + match import.binding { + Some(ref binding) if !binding.defined_with(DefModifiers::PRELUDE) => { if name_binding.is_extern_crate() { let msg = format!("import `{0}` conflicts with imported crate \ in this module (maybe you meant `use {0}::*`?)", From 22e189ed57b27caa396da44ce4f273b9a0061dda Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 21:23:58 +0000 Subject: [PATCH 03/12] Add and use an arena for `NameBinding`s --- src/librustc_resolve/build_reduced_graph.rs | 6 +-- src/librustc_resolve/lib.rs | 31 +++++++------ src/librustc_resolve/resolve_imports.rs | 51 +++++++++++---------- 3 files changed, 48 insertions(+), 40 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 6bc73194aa98b..baed8bb9919ef 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -101,14 +101,14 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { fn try_define(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) where T: ToNameBinding<'b> { - parent.try_define_child(name, ns, def.to_name_binding()); + parent.try_define_child(name, ns, self.new_name_binding(def.to_name_binding())); } /// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined; /// otherwise, reports an error. fn define>(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) { - let binding = def.to_name_binding(); - let old_binding = match parent.try_define_child(name, ns, binding.clone()) { + let binding = self.new_name_binding(def.to_name_binding()); + let old_binding = match parent.try_define_child(name, ns, binding) { Some(old_binding) => old_binding, None => return, }; diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index b2f60ddb8da61..528209d5854ce 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -794,7 +794,7 @@ pub struct ModuleS<'a> { is_public: bool, is_extern_crate: bool, - children: RefCell>>, + children: RefCell>>, imports: RefCell>, // The anonymous children of this node. Anonymous children are pseudo- @@ -855,21 +855,21 @@ impl<'a> ModuleS<'a> { } } - fn get_child(&self, name: Name, ns: Namespace) -> Option> { + fn get_child(&self, name: Name, ns: Namespace) -> Option<&'a NameBinding<'a>> { self.children.borrow().get(&(name, ns)).cloned() } // If the name is not yet defined, define the name and return None. // Otherwise, return the existing definition. - fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>) - -> Option> { + fn try_define_child(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) + -> Option<&'a NameBinding<'a>> { match self.children.borrow_mut().entry((name, ns)) { hash_map::Entry::Vacant(entry) => { entry.insert(binding); None } - hash_map::Entry::Occupied(entry) => { Some(entry.get().clone()) }, + hash_map::Entry::Occupied(entry) => { Some(entry.get()) }, } } - fn for_each_local_child)>(&self, mut f: F) { + fn for_each_local_child)>(&self, mut f: F) { for (&(name, ns), name_binding) in self.children.borrow().iter() { if !name_binding.is_extern_crate() { f(name, ns, name_binding) @@ -1112,6 +1112,7 @@ pub struct Resolver<'a, 'tcx: 'a> { pub struct ResolverArenas<'a> { modules: arena::TypedArena>, + name_bindings: arena::TypedArena>, } #[derive(PartialEq)] @@ -1177,6 +1178,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { fn arenas() -> ResolverArenas<'a> { ResolverArenas { modules: arena::TypedArena::new(), + name_bindings: arena::TypedArena::new(), } } @@ -1188,6 +1190,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { self.arenas.modules.alloc(ModuleS::new(parent_link, def, external, is_public)) } + fn new_name_binding(&self, name_binding: NameBinding<'a>) -> &'a NameBinding<'a> { + self.arenas.name_bindings.alloc(name_binding) + } + fn new_extern_crate_module(&self, parent_link: ParentLink<'a>, def: Def) -> Module<'a> { let mut module = ModuleS::new(parent_link, Some(def), false, true); module.is_extern_crate = true; @@ -1234,7 +1240,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { -> ResolveResult<(Module<'a>, LastPrivate)> { fn search_parent_externals<'a>(needle: Name, module: Module<'a>) -> Option> { match module.get_child(needle, TypeNS) { - Some(ref binding) if binding.is_extern_crate() => Some(module), + Some(binding) if binding.is_extern_crate() => Some(module), _ => match module.parent_link { ModuleParentLink(ref parent, _) => { search_parent_externals(needle, parent) @@ -1424,7 +1430,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { name: Name, namespace: Namespace, record_used: bool) - -> ResolveResult<(NameBinding<'a>, bool)> { + -> ResolveResult<(&'a NameBinding<'a>, bool)> { debug!("(resolving item in lexical scope) resolving `{}` in namespace {:?} in `{}`", name, namespace, @@ -1554,7 +1560,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { namespace: Namespace, allow_private_imports: bool, record_used: bool) - -> ResolveResult<(NameBinding<'a>, bool)> { + -> ResolveResult<(&'a NameBinding<'a>, bool)> { debug!("(resolving name in module) resolving `{}` in `{}`", name, module_to_string(&*module_)); @@ -1580,7 +1586,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving name in module) import unresolved; bailing out"); return Indeterminate; } - if let Some(binding) = import_resolution.binding.clone() { + if let Some(binding) = import_resolution.binding { debug!("(resolving name in module) resolved to import"); if record_used { self.record_import_use(name, namespace, &import_resolution); @@ -2619,7 +2625,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Success((binding, _)) => { debug!("(resolve bare identifier pattern) succeeded in finding {} at {:?}", name, - &binding); + binding); match binding.def() { None => { panic!("resolved name in the value namespace to a set of name bindings \ @@ -3058,7 +3064,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { match this.primitive_type_table.primitive_types.get(last_name) { Some(_) => None, None => this.current_module.get_child(*last_name, TypeNS) - .as_ref() .and_then(NameBinding::module) } } else { @@ -3456,7 +3461,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { for (&(_, ns), import) in search_module.import_resolutions.borrow().iter() { if ns != TypeNS { continue } let binding = match import.binding { - Some(ref binding) => binding, + Some(binding) => binding, None => continue, }; let did = match binding.def() { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 882531efbb716..015b07a7ebf0d 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -81,7 +81,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, mut binding: NameBinding<'a>) -> NameBinding<'a> { + fn import<'a>(&self, binding: &'a NameBinding<'a>) -> NameBinding<'a> { + let mut binding = binding.clone(); if self.shadowable == Shadowable::Always { binding.modifiers = binding.modifiers | DefModifiers::PRELUDE; } @@ -107,7 +108,7 @@ pub struct ImportResolution<'a> { pub is_public: bool, /// Resolution of the name in the namespace - pub binding: Option>, + pub binding: Option<&'a NameBinding<'a>>, /// The source node of the `use` directive pub id: NodeId, @@ -125,7 +126,7 @@ impl<'a> ImportResolution<'a> { pub fn shadowable(&self) -> Shadowable { match self.binding { - Some(ref binding) if binding.defined_with(DefModifiers::PRELUDE) => + Some(binding) if binding.defined_with(DefModifiers::PRELUDE) => Shadowable::Always, Some(_) => Shadowable::Never, None => Shadowable::Always, @@ -195,7 +196,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// Resolves an `ImportResolvingError` into the correct enum discriminant /// and passes that on to `resolve_error`. - fn import_resolving_error(&self, e: ImportResolvingError) { + fn import_resolving_error(&self, e: ImportResolvingError<'b>) { // If it's a single failed import then create a "fake" import // resolution for it so that later resolve stages won't complain. if let SingleImport(target, _) = e.import_directive.subclass { @@ -213,11 +214,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { debug!("(resolving import error) adding fake target to import resolution of `{}`", target); - let dummy_binding = NameBinding { + let dummy_binding = self.resolver.new_name_binding(NameBinding { modifiers: DefModifiers::IMPORTABLE, def_or_module: DefOrModule::Def(Def::Err), span: None, - }; + }); resolution.binding = Some(dummy_binding); } @@ -367,7 +368,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { name: Name, ns: Namespace, importing_module: Module<'b>) // Module importing the name - -> (ResolveResult>, bool) { + -> (ResolveResult<&'b NameBinding<'b>>, bool) { build_reduced_graph::populate_module_if_necessary(self.resolver, module); if let Some(name_binding) = module.get_child(name, ns) { if name_binding.is_extern_crate() { @@ -397,7 +398,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return (Failed(None), false); } - if let Some(binding) = resolution.binding.clone() { + if let Some(binding) = resolution.binding { self.resolver.record_import_use(name, ns, &resolution); (Success(binding), true) } else { @@ -455,9 +456,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { self.resolve_name_in_module(target_module, source, TypeNS, module_); match (&value_result, &type_result) { - (&Success(ref name_binding), _) if !value_used_reexport && - directive.is_public && - !name_binding.is_public() => { + (&Success(name_binding), _) if !value_used_reexport && + directive.is_public && + !name_binding.is_public() => { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = format!("Consider marking `{}` as `pub` in the imported module", source); @@ -466,8 +467,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { .emit(); } - (_, &Success(ref name_binding)) if !type_used_reexport && - directive.is_public => { + (_, &Success(name_binding)) if !type_used_reexport && directive.is_public => { if !name_binding.is_public() { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = @@ -519,7 +519,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { { let mut check_and_write_import = |namespace, result, used_public: &mut bool| { - let result: &ResolveResult = result; + let result: &ResolveResult<&NameBinding> = result; let import_resolution = import_resolutions.get_mut(&(target, namespace)).unwrap(); let namespace_name = match namespace { @@ -528,7 +528,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; match *result { - Success(ref name_binding) => { + Success(name_binding) => { debug!("(resolving single import) found {:?} target: {:?}", namespace_name, name_binding.def()); @@ -541,7 +541,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { directive.span, target); - import_resolution.binding = Some(directive.import(name_binding.clone())); + import_resolution.binding = + Some(self.resolver.new_name_binding(directive.import(name_binding))); import_resolution.id = directive.id; import_resolution.is_public = directive.is_public; @@ -679,14 +680,15 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { .or_insert_with(|| ImportResolution::new(id, is_public)); match target_import_resolution.binding { - Some(ref binding) if target_import_resolution.is_public => { + Some(binding) if target_import_resolution.is_public => { self.check_for_conflicting_import(&dest_import_resolution, import_directive.span, name, ns); dest_import_resolution.id = id; dest_import_resolution.is_public = is_public; - dest_import_resolution.binding = Some(import_directive.import(binding.clone())); + dest_import_resolution.binding = + Some(self.resolver.new_name_binding(import_directive.import(binding))); self.add_export(module_, name, &dest_import_resolution); } _ => {} @@ -701,7 +703,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { target_module, import_directive, (name, ns), - name_binding.clone()); + name_binding); }); // Record the destination of this import @@ -723,7 +725,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { containing_module: Module<'b>, import_directive: &ImportDirective, (name, ns): (Name, Namespace), - name_binding: NameBinding<'b>) { + name_binding: &'b NameBinding<'b>) { let id = import_directive.id; let is_public = import_directive.is_public; @@ -761,7 +763,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { name); span_err!(self.resolver.session, import_directive.span, E0251, "{}", msg); } else { - dest_import_resolution.binding = Some(import_directive.import(name_binding.clone())); + dest_import_resolution.binding = + Some(self.resolver.new_name_binding(import_directive.import(name_binding))); dest_import_resolution.id = id; dest_import_resolution.is_public = is_public; self.add_export(module_, name, &dest_import_resolution); @@ -799,7 +802,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { binding.is_some()); match *binding { - Some(ref binding) if !binding.defined_with(DefModifiers::PRELUDE) => { + Some(binding) if !binding.defined_with(DefModifiers::PRELUDE) => { let ns_word = match namespace { TypeNS => { match binding.module() { @@ -857,7 +860,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { if ns == ValueNS { match import.binding { - Some(ref binding) if !binding.defined_with(DefModifiers::PRELUDE) => { + Some(binding) if !binding.defined_with(DefModifiers::PRELUDE) => { let mut err = struct_span_err!(self.resolver.session, import_span, E0255, @@ -873,7 +876,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } } else { match import.binding { - Some(ref binding) if !binding.defined_with(DefModifiers::PRELUDE) => { + Some(binding) if !binding.defined_with(DefModifiers::PRELUDE) => { if name_binding.is_extern_crate() { let msg = format!("import `{0}` conflicts with imported crate \ in this module (maybe you meant `use {0}::*`?)", From 2e24c7410f47d5a69ab9bd69b06e4999e0e6bea3 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 21:34:23 +0000 Subject: [PATCH 04/12] Expand NameBinding to better represent bindings from imports --- src/librustc_resolve/build_reduced_graph.rs | 6 ++-- src/librustc_resolve/lib.rs | 38 ++++++++++++++------- src/librustc_resolve/resolve_imports.rs | 22 ++++++++---- 3 files changed, 45 insertions(+), 21 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index baed8bb9919ef..8654fa19c28a5 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -19,7 +19,7 @@ use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport}; use resolve_imports::ImportResolution; use Module; use Namespace::{self, TypeNS, ValueNS}; -use {NameBinding, DefOrModule}; +use {NameBinding, NameBindingKind}; use {names_to_string, module_to_string}; use ParentLink::{ModuleParentLink, BlockParentLink}; use Resolver; @@ -82,8 +82,8 @@ impl<'a> ToNameBinding<'a> for (Module<'a>, Span) { impl<'a> ToNameBinding<'a> for (Def, Span, DefModifiers) { fn to_name_binding(self) -> NameBinding<'a> { - let def = DefOrModule::Def(self.0); - NameBinding { modifiers: self.2, def_or_module: def, span: Some(self.1) } + let kind = NameBindingKind::Def(self.0); + NameBinding { modifiers: self.2, kind: kind, span: Some(self.1) } } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 528209d5854ce..37cedb75ec529 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -951,21 +951,26 @@ bitflags! { // We need to track them to prohibit reexports like `pub use PrivEnum::Variant`. const PRIVATE_VARIANT = 1 << 2, const PRELUDE = 1 << 3, + const GLOB_IMPORTED = 1 << 4, } } // Records a possibly-private value, type, or module definition. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct NameBinding<'a> { - modifiers: DefModifiers, // see note in ImportResolution about how to use this - def_or_module: DefOrModule<'a>, + modifiers: DefModifiers, + kind: NameBindingKind<'a>, span: Option, } -#[derive(Clone, Debug)] -enum DefOrModule<'a> { +#[derive(Debug)] +enum NameBindingKind<'a> { Def(Def), Module(Module<'a>), + Import { + binding: &'a NameBinding<'a>, + id: NodeId, + }, } impl<'a> NameBinding<'a> { @@ -976,20 +981,22 @@ impl<'a> NameBinding<'a> { DefModifiers::empty() } | DefModifiers::IMPORTABLE; - NameBinding { modifiers: modifiers, def_or_module: DefOrModule::Module(module), span: span } + NameBinding { modifiers: modifiers, kind: NameBindingKind::Module(module), span: span } } fn module(&self) -> Option> { - match self.def_or_module { - DefOrModule::Module(ref module) => Some(module), - DefOrModule::Def(_) => None, + match self.kind { + NameBindingKind::Module(module) => Some(module), + NameBindingKind::Def(_) => None, + NameBindingKind::Import { binding, .. } => binding.module(), } } fn def(&self) -> Option { - match self.def_or_module { - DefOrModule::Def(def) => Some(def), - DefOrModule::Module(ref module) => module.def, + match self.kind { + NameBindingKind::Def(def) => Some(def), + NameBindingKind::Module(module) => module.def, + NameBindingKind::Import { binding, .. } => binding.def(), } } @@ -1009,6 +1016,13 @@ impl<'a> NameBinding<'a> { fn is_extern_crate(&self) -> bool { self.module().map(|module| module.is_extern_crate).unwrap_or(false) } + + fn is_import(&self) -> bool { + match self.kind { + NameBindingKind::Import { .. } => true, + _ => false, + } + } } /// Interns the names of the primitive types. diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 015b07a7ebf0d..9f0e9e1075213 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -11,10 +11,9 @@ use self::ImportDirectiveSubclass::*; use DefModifiers; -use DefOrModule; use Module; use Namespace::{self, TypeNS, ValueNS}; -use NameBinding; +use {NameBinding, NameBindingKind}; use ResolveResult; use ResolveResult::*; use Resolver; @@ -82,11 +81,22 @@ 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>) -> NameBinding<'a> { - let mut binding = binding.clone(); + let mut modifiers = match self.is_public { + true => DefModifiers::PUBLIC | DefModifiers::IMPORTABLE, + false => DefModifiers::empty(), + }; + if let GlobImport = self.subclass { + modifiers = modifiers | DefModifiers::GLOB_IMPORTED; + } if self.shadowable == Shadowable::Always { - binding.modifiers = binding.modifiers | DefModifiers::PRELUDE; + modifiers = modifiers | DefModifiers::PRELUDE; + } + + NameBinding { + kind: NameBindingKind::Import { binding: binding, id: self.id }, + span: Some(self.span), + modifiers: modifiers, } - binding } } @@ -216,7 +226,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let dummy_binding = self.resolver.new_name_binding(NameBinding { modifiers: DefModifiers::IMPORTABLE, - def_or_module: DefOrModule::Def(Def::Err), + kind: NameBindingKind::Def(Def::Err), span: None, }); From 4428b1cfdf724bc4b43173195192da57a5901e4f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 21:48:24 +0000 Subject: [PATCH 05/12] Refactor away separate tracking of used_public and used_reexport. NameBinding now encodes these directly with binding.is_public() and (binding.is_public() && binding.is_import()) (respectively) --- src/librustc_resolve/lib.rs | 39 ++++++++++----------- src/librustc_resolve/resolve_imports.rs | 46 ++++++++++--------------- 2 files changed, 37 insertions(+), 48 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 37cedb75ec529..20663a1e12cea 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1311,7 +1311,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { name); return Indeterminate; } - Success((binding, used_proxy)) => { + 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() { @@ -1319,7 +1319,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Keep track of the closest private module used // when resolving this import chain. - if !used_proxy && !search_module.is_public { + if !binding.is_public() { if let Some(did) = search_module.def_id() { closest_private = LastMod(DependsOn(did)); } @@ -1410,7 +1410,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving module path for import) indeterminate; bailing"); return Indeterminate; } - Success((binding, _)) => match binding.module() { + Success(binding) => match binding.module() { Some(containing_module) => { search_module = containing_module; start_index = 1; @@ -1444,7 +1444,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { name: Name, namespace: Namespace, record_used: bool) - -> ResolveResult<(&'a NameBinding<'a>, bool)> { + -> ResolveResult<&'a NameBinding<'a>> { debug!("(resolving item in lexical scope) resolving `{}` in namespace {:?} in `{}`", name, namespace, @@ -1466,10 +1466,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving item in lexical scope) indeterminate higher scope; bailing"); return Indeterminate; } - Success((binding, used_reexport)) => { + Success(binding) => { // We found the module. debug!("(resolving item in lexical scope) found name in module, done"); - return Success((binding, used_reexport)); + return Success(binding); } } @@ -1565,16 +1565,13 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { /// Attempts to resolve the supplied name in the given module for the /// given namespace. If successful, returns the binding corresponding to /// the name. - /// - /// The boolean returned on success is an indicator of whether this lookup - /// passed through a public re-export proxy. fn resolve_name_in_module(&mut self, module_: Module<'a>, name: Name, namespace: Namespace, allow_private_imports: bool, record_used: bool) - -> ResolveResult<(&'a NameBinding<'a>, bool)> { + -> ResolveResult<&'a NameBinding<'a>> { debug!("(resolving name in module) resolving `{}` in `{}`", name, module_to_string(&*module_)); @@ -1590,7 +1587,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { self.used_crates.insert(krate); } } - return Success((binding, false)); + return Success(binding); } // Check the list of resolved imports. @@ -1605,7 +1602,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { if record_used { self.record_import_use(name, namespace, &import_resolution); } - return Success((binding, true)); + return Success(binding); } } Some(..) | None => {} // Continue. @@ -2636,7 +2633,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { -> BareIdentifierPatternResolution { let module = self.current_module; match self.resolve_item_in_lexical_scope(module, name, ValueNS, true) { - Success((binding, _)) => { + Success(binding) => { debug!("(resolve bare identifier pattern) succeeded in finding {} at {:?}", name, binding); @@ -2796,7 +2793,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let module = self.current_module; let name = identifier.unhygienic_name; match self.resolve_item_in_lexical_scope(module, name, namespace, record_used) { - Success((binding, _)) => binding.def().map(LocalDef::from_def), + Success(binding) => binding.def().map(LocalDef::from_def), Failed(Some((span, msg))) => { resolve_error(self, span, ResolutionError::FailedToResolve(&*msg)); None @@ -2934,7 +2931,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let name = segments.last().unwrap().identifier.name; let result = self.resolve_name_in_module(containing_module, name, namespace, false, true); let def = match result { - Success((binding, _)) => { + Success(binding) => { let (def, lp) = binding.def_and_lp(); (def, last_private.or(lp)) } @@ -2990,7 +2987,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let name = segments.last().unwrap().identifier.name; match self.resolve_name_in_module(containing_module, name, namespace, false, true) { - Success((binding, _)) => { + Success(binding) => { let (def, lp) = binding.def_and_lp(); Some((def, last_private.or(lp))) } @@ -3028,11 +3025,11 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } if let AnonymousModuleRibKind(module) = self.get_ribs(namespace)[i].kind { - if let Success((binding, _)) = self.resolve_name_in_module(module, - ident.unhygienic_name, - namespace, - true, - true) { + if let Success(binding) = self.resolve_name_in_module(module, + ident.unhygienic_name, + namespace, + true, + true) { if let Some(def) = binding.def() { return Some(LocalDef::from_def(def)); } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 9f0e9e1075213..54dcd0001c4a5 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -371,14 +371,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } /// Resolves the name in the namespace of the module because it is being imported by - /// importing_module. Returns the name bindings defining the name - /// and whether or not the name was imported. + /// importing_module. Returns the name bindings defining the name. fn resolve_name_in_module(&mut self, module: Module<'b>, // Module containing the name name: Name, ns: Namespace, importing_module: Module<'b>) // Module importing the name - -> (ResolveResult<&'b NameBinding<'b>>, bool) { + -> ResolveResult<&'b NameBinding<'b>> { build_reduced_graph::populate_module_if_necessary(self.resolver, module); if let Some(name_binding) = module.get_child(name, ns) { if name_binding.is_extern_crate() { @@ -387,32 +386,32 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { self.resolver.used_crates.insert(krate); } } - return (Success(name_binding), false) + return Success(name_binding); } // If there is an unresolved glob at this point in the containing module, bail out. // We don't know enough to be able to resolve the name. if module.pub_glob_count.get() > 0 { - return (Indeterminate, false); + return Indeterminate; } match module.import_resolutions.borrow().get(&(name, ns)) { // The containing module definitely doesn't have an exported import with the // name in question. We can therefore accurately report that names are unbound. - None => (Failed(None), false), + None => Failed(None), // The name is an import which has been fully resolved, so we just follow it. Some(resolution) if resolution.outstanding_references == 0 => { // Import resolutions must be declared with "pub" in order to be exported. if !resolution.is_public { - return (Failed(None), false); + return Failed(None); } if let Some(binding) = resolution.binding { self.resolver.record_import_use(name, ns, &resolution); - (Success(binding), true) + Success(binding) } else { - (Failed(None), false) + Failed(None) } } @@ -427,8 +426,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // In this case we continue as if we resolved the import and let // check_for_conflicts_between_imports_and_items handle the conflict Some(_) => match (importing_module.def_id(), module.def_id()) { - (Some(id1), Some(id2)) if id1 == id2 => (Failed(None), false), - _ => (Indeterminate, false) + (Some(id1), Some(id2)) if id1 == id2 => Failed(None), + _ => Indeterminate }, } } @@ -460,13 +459,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; // We need to resolve both namespaces for this to succeed. - let (value_result, value_used_reexport) = + let value_result = self.resolve_name_in_module(target_module, source, ValueNS, module_); - let (type_result, type_used_reexport) = + let type_result = self.resolve_name_in_module(target_module, source, TypeNS, module_); match (&value_result, &type_result) { - (&Success(name_binding), _) if !value_used_reexport && + (&Success(name_binding), _) if !name_binding.is_import() && directive.is_public && !name_binding.is_public() => { let msg = format!("`{}` is private, and cannot be reexported", source); @@ -477,7 +476,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { .emit(); } - (_, &Success(name_binding)) if !type_used_reexport && directive.is_public => { + (_, &Success(name_binding)) if !name_binding.is_import() && directive.is_public => { if !name_binding.is_public() { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = @@ -521,14 +520,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { _ => (), } - let mut value_used_public = false; - let mut type_used_public = false; - // We've successfully resolved the import. Write the results in. let mut import_resolutions = module_.import_resolutions.borrow_mut(); { - let mut check_and_write_import = |namespace, result, used_public: &mut bool| { + let mut check_and_write_import = |namespace, result| { let result: &ResolveResult<&NameBinding> = result; let import_resolution = import_resolutions.get_mut(&(target, namespace)).unwrap(); @@ -557,7 +553,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { import_resolution.is_public = directive.is_public; self.add_export(module_, target, &import_resolution); - *used_public = name_binding.is_public(); } Failed(_) => { // Continue. @@ -572,8 +567,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { directive.span, (target, namespace)); }; - check_and_write_import(ValueNS, &value_result, &mut value_used_public); - check_and_write_import(TypeNS, &type_result, &mut type_used_public); + check_and_write_import(ValueNS, &value_result); + check_and_write_import(TypeNS, &type_result); } if let (&Failed(_), &Failed(_)) = (&value_result, &type_result) { @@ -583,9 +578,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return Failed(Some((directive.span, msg))); } - let value_used_public = value_used_reexport || value_used_public; - let type_used_public = type_used_reexport || type_used_public; - let value_def_and_priv = { let import_resolution_value = import_resolutions.get_mut(&(target, ValueNS)).unwrap(); assert!(import_resolution_value.outstanding_references >= 1); @@ -596,7 +588,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // purposes it's good enough to just favor one over the other. import_resolution_value.binding.as_ref().map(|binding| { let def = binding.def().unwrap(); - let last_private = if value_used_public { lp } else { DependsOn(def.def_id()) }; + let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; (def, last_private) }) }; @@ -608,7 +600,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { import_resolution_type.binding.as_ref().map(|binding| { let def = binding.def().unwrap(); - let last_private = if type_used_public { lp } else { DependsOn(def.def_id()) }; + let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; (def, last_private) }) }; From 96b4dc4b87c6a6afd9116fe8be8f5bf055878314 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 22:28:54 +0000 Subject: [PATCH 06/12] Refactor away the fields id and is_public of ImportResolution and rename ImportResolution to NameResolution --- src/librustc_resolve/build_reduced_graph.rs | 7 +- src/librustc_resolve/lib.rs | 35 ++++++---- src/librustc_resolve/resolve_imports.rs | 72 ++++++++------------- 3 files changed, 49 insertions(+), 65 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 8654fa19c28a5..0c7563f986467 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -16,7 +16,7 @@ use DefModifiers; use resolve_imports::ImportDirective; use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport}; -use resolve_imports::ImportResolution; +use resolve_imports::NameResolution; use Module; use Namespace::{self, TypeNS, ValueNS}; use {NameBinding, NameBindingKind}; @@ -703,13 +703,10 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let mut import_resolutions = module_.import_resolutions.borrow_mut(); for &ns in [TypeNS, ValueNS].iter() { let mut resolution = import_resolutions.entry((target, ns)).or_insert( - ImportResolution::new(id, is_public) + NameResolution::default() ); resolution.outstanding_references += 1; - // the source of this name is different now - resolution.id = id; - resolution.is_public = is_public; } } GlobImport => { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 20663a1e12cea..5a78315d79cf1 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -92,7 +92,7 @@ use std::cell::{Cell, RefCell}; use std::fmt; use std::mem::replace; -use resolve_imports::{ImportDirective, ImportResolution}; +use resolve_imports::{ImportDirective, NameResolution}; // NB: This module needs to be declared first so diagnostics are // registered before they are used. @@ -346,8 +346,9 @@ fn resolve_struct_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>, .import_resolutions .borrow() .get(&(name, ValueNS)) { - let item = resolver.ast_map.expect_item(directive.id); - err.span_note(item.span, "constant imported here"); + if let Some(binding) = directive.binding { + err.span_note(binding.span.unwrap(), "constant imported here"); + } } err } @@ -814,7 +815,7 @@ pub struct ModuleS<'a> { anonymous_children: RefCell>>, // The status of resolving each import in this module. - import_resolutions: RefCell>>, + import_resolutions: RefCell>>, // The number of unresolved globs that this module exports. glob_count: Cell, @@ -1219,8 +1220,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } #[inline] - fn record_import_use(&mut self, name: Name, ns: Namespace, resolution: &ImportResolution<'a>) { - let import_id = resolution.id; + fn record_import_use(&mut self, name: Name, ns: Namespace, binding: &NameBinding<'a>) { + let import_id = match binding.kind { + NameBindingKind::Import { id, .. } => id, + _ => return, + }; + self.used_imports.insert((import_id, ns)); if !self.make_glob_map { @@ -1592,20 +1597,22 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Check the list of resolved imports. match module_.import_resolutions.borrow().get(&(name, namespace)) { - Some(import_resolution) if allow_private_imports || import_resolution.is_public => { - if import_resolution.is_public && import_resolution.outstanding_references != 0 { - debug!("(resolving name in module) import unresolved; bailing out"); - return Indeterminate; - } + Some(import_resolution) => { if let Some(binding) = import_resolution.binding { + if !allow_private_imports && binding.is_public() { return Failed(None) } + if binding.is_public() && import_resolution.outstanding_references != 0 { + debug!("(resolving name in module) import unresolved; bailing out"); + return Indeterminate; + } + debug!("(resolving name in module) resolved to import"); if record_used { - self.record_import_use(name, namespace, &import_resolution); + self.record_import_use(name, namespace, binding); } return Success(binding); } } - Some(..) | None => {} // Continue. + None => {} } // We're out of luck. @@ -3482,7 +3489,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { if self.trait_item_map.contains_key(&(name, did)) { add_trait_info(&mut found_traits, did, name); let trait_name = self.get_trait_name(did); - self.record_import_use(trait_name, TypeNS, &import); + self.record_import_use(trait_name, TypeNS, binding); } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 54dcd0001c4a5..b67de2d170dbb 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -101,12 +101,12 @@ impl ImportDirective { } #[derive(Debug)] -/// An ImportResolution records what we know about an imported name in a given namespace. +/// An NameResolution records what we know about an imported name in a given namespace. /// More specifically, it records the number of unresolved `use` directives that import the name, /// the `use` directive importing the name in the namespace, and the `NameBinding` to which the /// name in the namespace resolves (if applicable). /// Different `use` directives may import the same name in different namespaces. -pub struct ImportResolution<'a> { +pub struct NameResolution<'a> { // When outstanding_references reaches zero, outside modules can count on the targets being // correct. Before then, all bets are off; future `use` directives could override the name. // Since shadowing is forbidden, the only way outstanding_references > 1 in a legal program @@ -114,26 +114,17 @@ pub struct ImportResolution<'a> { // value and the other of which resolves to a type. pub outstanding_references: usize, - /// Whether this resolution came from a `use` or a `pub use`. - pub is_public: bool, - /// Resolution of the name in the namespace pub binding: Option<&'a NameBinding<'a>>, - - /// The source node of the `use` directive - pub id: NodeId, } -impl<'a> ImportResolution<'a> { - pub fn new(id: NodeId, is_public: bool) -> Self { - ImportResolution { - outstanding_references: 0, - id: id, - binding: None, - is_public: is_public, - } +impl<'a> Default for NameResolution<'a> { + fn default() -> Self { + NameResolution { outstanding_references: 0, binding: None } } +} +impl<'a> NameResolution<'a> { pub fn shadowable(&self) -> Shadowable { match self.binding { Some(binding) if binding.defined_with(DefModifiers::PRELUDE) => @@ -216,8 +207,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { debug!("(resolving import error) adding import resolution for `{}`", target); - ImportResolution::new(e.import_directive.id, - e.import_directive.is_public) + NameResolution::default() }); if resolution.binding.is_none() { @@ -402,13 +392,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // The name is an import which has been fully resolved, so we just follow it. Some(resolution) if resolution.outstanding_references == 0 => { - // Import resolutions must be declared with "pub" in order to be exported. - if !resolution.is_public { - return Failed(None); - } - if let Some(binding) = resolution.binding { - self.resolver.record_import_use(name, ns, &resolution); + // Import resolutions must be declared with "pub" in order to be exported. + if !binding.is_public() { + return Failed(None); + } + + self.resolver.record_import_use(name, ns, binding); Success(binding) } else { Failed(None) @@ -549,10 +539,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { import_resolution.binding = Some(self.resolver.new_name_binding(directive.import(name_binding))); - import_resolution.id = directive.id; - import_resolution.is_public = directive.is_public; - self.add_export(module_, target, &import_resolution); + self.add_export(module_, target, import_resolution.binding.unwrap()); } Failed(_) => { // Continue. @@ -644,7 +632,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { lp: LastPrivate) -> ResolveResult<()> { let id = import_directive.id; - let is_public = import_directive.is_public; // This function works in a highly imperative manner; it eagerly adds // everything it can to the list of import resolutions of the module @@ -679,19 +666,17 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let mut import_resolutions = module_.import_resolutions.borrow_mut(); let mut dest_import_resolution = import_resolutions.entry((name, ns)) - .or_insert_with(|| ImportResolution::new(id, is_public)); + .or_insert_with(|| NameResolution::default()); match target_import_resolution.binding { - Some(binding) if target_import_resolution.is_public => { + Some(binding) if binding.is_public() => { self.check_for_conflicting_import(&dest_import_resolution, import_directive.span, name, ns); - dest_import_resolution.id = id; - dest_import_resolution.is_public = is_public; dest_import_resolution.binding = Some(self.resolver.new_name_binding(import_directive.import(binding))); - self.add_export(module_, name, &dest_import_resolution); + self.add_export(module_, name, dest_import_resolution.binding.unwrap()); } _ => {} } @@ -728,12 +713,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { import_directive: &ImportDirective, (name, ns): (Name, Namespace), name_binding: &'b NameBinding<'b>) { - let id = import_directive.id; let is_public = import_directive.is_public; let mut import_resolutions = module_.import_resolutions.borrow_mut(); let dest_import_resolution = import_resolutions.entry((name, ns)).or_insert_with(|| { - ImportResolution::new(id, is_public) + NameResolution::default() }); debug!("(resolving glob import) writing resolution `{}` in `{}` to `{}`", @@ -767,9 +751,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } else { dest_import_resolution.binding = Some(self.resolver.new_name_binding(import_directive.import(name_binding))); - dest_import_resolution.id = id; - dest_import_resolution.is_public = is_public; - self.add_export(module_, name, &dest_import_resolution); + self.add_export(module_, name, dest_import_resolution.binding.unwrap()); } } @@ -779,13 +761,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { (name, ns)); } - fn add_export(&mut self, module: Module<'b>, name: Name, resolution: &ImportResolution<'b>) { - if !resolution.is_public { return } + fn add_export(&mut self, module: Module<'b>, name: Name, binding: &NameBinding<'b>) { + if !binding.is_public() { return } let node_id = match module.def_id() { Some(def_id) => self.resolver.ast_map.as_local_node_id(def_id).unwrap(), None => return, }; - let export = match resolution.binding.as_ref().unwrap().def() { + let export = match binding.def() { Some(def) => Export { name: name, def_id: def.def_id() }, None => return, }; @@ -794,7 +776,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// Checks that imported names and items don't have the same name. fn check_for_conflicting_import(&mut self, - import_resolution: &ImportResolution, + import_resolution: &NameResolution, import_span: Span, name: Name, namespace: Namespace) { @@ -815,8 +797,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } ValueNS => "value", }; - let use_id = import_resolution.id; - let item = self.resolver.ast_map.expect_item(use_id); let mut err = struct_span_err!(self.resolver.session, import_span, E0252, @@ -825,7 +805,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { ns_word, name); span_note!(&mut err, - item.span, + binding.span.unwrap(), "previous import of `{}` here", name); err.emit(); @@ -848,7 +828,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// Checks that imported names and items don't have the same name. fn check_for_conflicts_between_imports_and_items(&mut self, module: Module<'b>, - import: &ImportResolution<'b>, + import: &NameResolution<'b>, import_span: Span, (name, ns): (Name, Namespace)) { // Check for item conflicts. From 16e7ff1bffea9f1b4c9f016989081286b32a1ec7 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 22:40:23 +0000 Subject: [PATCH 07/12] Write and use increment_outstanding_references_for and decrement_outstanding_references_for --- src/librustc_resolve/build_reduced_graph.rs | 12 ++---------- src/librustc_resolve/lib.rs | 20 ++++++++++++++++++++ src/librustc_resolve/resolve_imports.rs | 14 +++++--------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 0c7563f986467..d311afb23976d 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -16,7 +16,6 @@ use DefModifiers; use resolve_imports::ImportDirective; use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport}; -use resolve_imports::NameResolution; use Module; use Namespace::{self, TypeNS, ValueNS}; use {NameBinding, NameBindingKind}; @@ -699,15 +698,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { debug!("(building import directive) building import directive: {}::{}", names_to_string(&module_.imports.borrow().last().unwrap().module_path), target); - - let mut import_resolutions = module_.import_resolutions.borrow_mut(); - for &ns in [TypeNS, ValueNS].iter() { - let mut resolution = import_resolutions.entry((target, ns)).or_insert( - NameResolution::default() - ); - - resolution.outstanding_references += 1; - } + module_.increment_outstanding_references_for(target, ValueNS); + module_.increment_outstanding_references_for(target, TypeNS); } GlobImport => { // Set the glob flag. This tells us that we don't know the diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 5a78315d79cf1..0325bcc24c499 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -670,6 +670,13 @@ impl ResolveResult { Success(t) => f(t), } } + + fn success(self) -> Option { + match self { + Success(t) => Some(t), + _ => None, + } + } } enum FallbackSuggestion { @@ -870,6 +877,19 @@ impl<'a> ModuleS<'a> { } } + fn increment_outstanding_references_for(&self, name: Name, ns: Namespace) { + let mut resolutions = self.import_resolutions.borrow_mut(); + resolutions.entry((name, ns)).or_insert_with(Default::default).outstanding_references += 1; + } + + fn decrement_outstanding_references_for(&self, name: Name, ns: Namespace) { + match self.import_resolutions.borrow_mut().get_mut(&(name, ns)).unwrap() + .outstanding_references { + 0 => panic!("No more outstanding references!"), + ref mut outstanding_references => { *outstanding_references -= 1; } + } + } + fn for_each_local_child)>(&self, mut f: F) { for (&(name, ns), name_binding) in self.children.borrow().iter() { if !name_binding.is_extern_crate() { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index b67de2d170dbb..18404a49a3690 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -511,9 +511,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } // We've successfully resolved the import. Write the results in. - let mut import_resolutions = module_.import_resolutions.borrow_mut(); { + let mut import_resolutions = module_.import_resolutions.borrow_mut(); let mut check_and_write_import = |namespace, result| { let result: &ResolveResult<&NameBinding> = result; @@ -567,14 +567,12 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } let value_def_and_priv = { - let import_resolution_value = import_resolutions.get_mut(&(target, ValueNS)).unwrap(); - assert!(import_resolution_value.outstanding_references >= 1); - import_resolution_value.outstanding_references -= 1; + module_.decrement_outstanding_references_for(target, ValueNS); // 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. - import_resolution_value.binding.as_ref().map(|binding| { + value_result.success().map(|binding| { let def = binding.def().unwrap(); let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; (def, last_private) @@ -582,11 +580,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; let type_def_and_priv = { - let import_resolution_type = import_resolutions.get_mut(&(target, TypeNS)).unwrap(); - assert!(import_resolution_type.outstanding_references >= 1); - import_resolution_type.outstanding_references -= 1; + module_.decrement_outstanding_references_for(target, TypeNS); - import_resolution_type.binding.as_ref().map(|binding| { + type_result.success().map(|binding| { let def = binding.def().unwrap(); let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; (def, last_private) From d881eee608bcfcd89643cb126c9f3aefc6a80661 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 8 Feb 2016 00:54:31 +0000 Subject: [PATCH 08/12] Change try_define_child to return a Result instead of an Option --- src/librustc_resolve/build_reduced_graph.rs | 6 +++--- src/librustc_resolve/lib.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index d311afb23976d..775ed34ab073c 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -100,7 +100,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { fn try_define(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) where T: ToNameBinding<'b> { - parent.try_define_child(name, ns, self.new_name_binding(def.to_name_binding())); + let _ = parent.try_define_child(name, ns, self.new_name_binding(def.to_name_binding())); } /// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined; @@ -108,8 +108,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { fn define>(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) { let binding = self.new_name_binding(def.to_name_binding()); let old_binding = match parent.try_define_child(name, ns, binding) { - Some(old_binding) => old_binding, - None => return, + Ok(()) => return, + Err(old_binding) => old_binding, }; let span = binding.span.unwrap_or(DUMMY_SP); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 0325bcc24c499..47a811bc8ab89 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -870,10 +870,10 @@ impl<'a> ModuleS<'a> { // If the name is not yet defined, define the name and return None. // Otherwise, return the existing definition. fn try_define_child(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) - -> Option<&'a NameBinding<'a>> { + -> Result<(), &'a NameBinding<'a>> { match self.children.borrow_mut().entry((name, ns)) { - hash_map::Entry::Vacant(entry) => { entry.insert(binding); None } - hash_map::Entry::Occupied(entry) => { Some(entry.get()) }, + hash_map::Entry::Vacant(entry) => { entry.insert(binding); Ok(()) } + hash_map::Entry::Occupied(entry) => { Err(entry.get()) }, } } From 7000e708252bd9e45f28926d8fd38cc9ec054e57 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 23:58:14 +0000 Subject: [PATCH 09/12] Replace children and import_resolutions with a single NameResolution-valued map. Refactor away resolve_name_in_module in resolve_imports.rs Rewrite and improve the core name resolution procedure in NameResolution::result and Module::resolve_name Refactor the duplicate checking code into NameResolution::try_define --- src/librustc_resolve/lib.rs | 175 +++---- src/librustc_resolve/resolve_imports.rs | 545 ++++++-------------- src/test/compile-fail/double-type-import.rs | 2 +- src/test/compile-fail/unresolved-import.rs | 4 +- 4 files changed, 224 insertions(+), 502 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 47a811bc8ab89..a5828067fcafc 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -87,7 +87,7 @@ use rustc_front::hir::{TraitRef, Ty, TyBool, TyChar, TyFloat, TyInt}; use rustc_front::hir::{TyRptr, TyStr, TyUint, TyPath, TyPtr}; use rustc_front::util::walk_pat; -use std::collections::{hash_map, HashMap, HashSet}; +use std::collections::{HashMap, HashSet}; use std::cell::{Cell, RefCell}; use std::fmt; use std::mem::replace; @@ -342,11 +342,8 @@ fn resolve_struct_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>, if let Some(sp) = resolver.ast_map.span_if_local(did) { err.span_note(sp, "constant defined here"); } - if let Some(directive) = resolver.current_module - .import_resolutions - .borrow() - .get(&(name, ValueNS)) { - if let Some(binding) = directive.binding { + if let Success(binding) = resolver.current_module.resolve_name(name, ValueNS, true) { + if binding.is_import() { err.span_note(binding.span.unwrap(), "constant imported here"); } } @@ -653,10 +650,10 @@ impl<'a, 'v, 'tcx> Visitor<'v> for Resolver<'a, 'tcx> { } } -type ErrorMessage = Option<(Span, String)>; +pub type ErrorMessage = Option<(Span, String)>; #[derive(Clone, PartialEq, Eq)] -enum ResolveResult { +pub enum ResolveResult { Failed(ErrorMessage), // Failed to resolve the name, optional helpful error message. Indeterminate, // Couldn't determine due to unresolved globs. Success(T), // Successfully resolved the import. @@ -802,7 +799,7 @@ pub struct ModuleS<'a> { is_public: bool, is_extern_crate: bool, - children: RefCell>>, + children: RefCell>>, imports: RefCell>, // The anonymous children of this node. Anonymous children are pseudo- @@ -821,9 +818,6 @@ pub struct ModuleS<'a> { // entry block for `f`. anonymous_children: RefCell>>, - // The status of resolving each import in this module. - import_resolutions: RefCell>>, - // The number of unresolved globs that this module exports. glob_count: Cell, @@ -854,7 +848,6 @@ impl<'a> ModuleS<'a> { children: RefCell::new(HashMap::new()), imports: RefCell::new(Vec::new()), anonymous_children: RefCell::new(NodeMap()), - import_resolutions: RefCell::new(HashMap::new()), glob_count: Cell::new(0), pub_count: Cell::new(0), pub_glob_count: Cell::new(0), @@ -863,39 +856,49 @@ impl<'a> ModuleS<'a> { } } - fn get_child(&self, name: Name, ns: Namespace) -> Option<&'a NameBinding<'a>> { - self.children.borrow().get(&(name, ns)).cloned() + fn resolve_name(&self, name: Name, ns: Namespace, allow_private_imports: bool) + -> ResolveResult<&'a NameBinding<'a>> { + let glob_count = + if allow_private_imports { self.glob_count.get() } else { self.pub_glob_count.get() }; + + self.children.borrow().get(&(name, ns)).cloned().unwrap_or_default().result(glob_count) + .and_then(|binding| { + let allowed = allow_private_imports || !binding.is_import() || binding.is_public(); + if allowed { Success(binding) } else { Failed(None) } + }) } - // If the name is not yet defined, define the name and return None. - // Otherwise, return the existing definition. + // Define the name or return the existing binding if there is a collision. fn try_define_child(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) -> Result<(), &'a NameBinding<'a>> { - match self.children.borrow_mut().entry((name, ns)) { - hash_map::Entry::Vacant(entry) => { entry.insert(binding); Ok(()) } - hash_map::Entry::Occupied(entry) => { Err(entry.get()) }, - } + self.children.borrow_mut().entry((name, ns)).or_insert_with(Default::default) + .try_define(binding) } fn increment_outstanding_references_for(&self, name: Name, ns: Namespace) { - let mut resolutions = self.import_resolutions.borrow_mut(); - resolutions.entry((name, ns)).or_insert_with(Default::default).outstanding_references += 1; + let mut children = self.children.borrow_mut(); + children.entry((name, ns)).or_insert_with(Default::default).outstanding_references += 1; } fn decrement_outstanding_references_for(&self, name: Name, ns: Namespace) { - match self.import_resolutions.borrow_mut().get_mut(&(name, ns)).unwrap() - .outstanding_references { + match self.children.borrow_mut().get_mut(&(name, ns)).unwrap().outstanding_references { 0 => panic!("No more outstanding references!"), ref mut outstanding_references => { *outstanding_references -= 1; } } } + fn for_each_child)>(&self, mut f: F) { + for (&(name, ns), name_resolution) in self.children.borrow().iter() { + name_resolution.binding.map(|binding| f(name, ns, binding)); + } + } + fn for_each_local_child)>(&self, mut f: F) { - for (&(name, ns), name_binding) in self.children.borrow().iter() { - if !name_binding.is_extern_crate() { + self.for_each_child(|name, ns, name_binding| { + if !name_binding.is_import() && !name_binding.is_extern_crate() { f(name, ns, name_binding) } - } + }) } fn def_id(&self) -> Option { @@ -1240,7 +1243,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } #[inline] - fn record_import_use(&mut self, name: Name, ns: Namespace, binding: &NameBinding<'a>) { + fn record_use(&mut self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) { + // track extern crates for unused_extern_crate lint + if let Some(DefId { krate, .. }) = binding.module().and_then(ModuleS::def_id) { + self.used_crates.insert(krate); + } + let import_id = match binding.kind { NameBindingKind::Import { id, .. } => id, _ => return, @@ -1278,8 +1286,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { lp: LastPrivate) -> ResolveResult<(Module<'a>, LastPrivate)> { fn search_parent_externals<'a>(needle: Name, module: Module<'a>) -> Option> { - match module.get_child(needle, TypeNS) { - Some(binding) if binding.is_extern_crate() => Some(module), + match module.resolve_name(needle, TypeNS, false) { + Success(binding) if binding.is_extern_crate() => Some(module), _ => match module.parent_link { ModuleParentLink(ref parent, _) => { search_parent_externals(needle, parent) @@ -1591,53 +1599,21 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { /// given namespace. If successful, returns the binding corresponding to /// the name. fn resolve_name_in_module(&mut self, - module_: Module<'a>, + module: Module<'a>, name: Name, namespace: Namespace, allow_private_imports: bool, record_used: bool) -> ResolveResult<&'a NameBinding<'a>> { - debug!("(resolving name in module) resolving `{}` in `{}`", - name, - module_to_string(&*module_)); - - // First, check the direct children of the module. - build_reduced_graph::populate_module_if_necessary(self, module_); - - if let Some(binding) = module_.get_child(name, namespace) { - debug!("(resolving name in module) found node as child"); - if binding.is_extern_crate() { - // track the extern crate as used. - if let Some(DefId { krate, .. }) = binding.module().unwrap().def_id() { - self.used_crates.insert(krate); - } - } - return Success(binding); - } - - // Check the list of resolved imports. - match module_.import_resolutions.borrow().get(&(name, namespace)) { - Some(import_resolution) => { - if let Some(binding) = import_resolution.binding { - if !allow_private_imports && binding.is_public() { return Failed(None) } - if binding.is_public() && import_resolution.outstanding_references != 0 { - debug!("(resolving name in module) import unresolved; bailing out"); - return Indeterminate; - } + debug!("(resolving name in module) resolving `{}` in `{}`", name, module_to_string(module)); - debug!("(resolving name in module) resolved to import"); - if record_used { - self.record_import_use(name, namespace, binding); - } - return Success(binding); - } + build_reduced_graph::populate_module_if_necessary(self, module); + module.resolve_name(name, namespace, allow_private_imports).and_then(|binding| { + if record_used { + self.record_use(name, namespace, binding); } - None => {} - } - - // We're out of luck. - debug!("(resolving name in module) failed to resolve `{}`", name); - return Failed(None); + Success(binding) + }) } fn report_unresolved_imports(&mut self, module_: Module<'a>) { @@ -1700,22 +1676,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Some(name) => { build_reduced_graph::populate_module_if_necessary(self, &orig_module); - match orig_module.get_child(name, TypeNS) { - None => { - debug!("!!! (with scope) didn't find `{}` in `{}`", - name, - module_to_string(&*orig_module)); - } - Some(name_binding) => { - match name_binding.module() { - None => { - debug!("!!! (with scope) didn't find module for `{}` in `{}`", - name, - module_to_string(&*orig_module)); - } - Some(module_) => { - self.current_module = module_; - } + if let Success(name_binding) = orig_module.resolve_name(name, TypeNS, false) { + match name_binding.module() { + None => { + debug!("!!! (with scope) didn't find module for `{}` in `{}`", + name, + module_to_string(orig_module)); + } + Some(module) => { + self.current_module = module; } } } @@ -3101,7 +3070,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { if name_path.len() == 1 { match this.primitive_type_table.primitive_types.get(last_name) { Some(_) => None, - None => this.current_module.get_child(*last_name, TypeNS) + None => this.current_module.resolve_name(*last_name, TypeNS, true).success() .and_then(NameBinding::module) } } else { @@ -3161,7 +3130,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Look for a method in the current self type's impl module. if let Some(module) = get_module(self, path.span, &name_path) { - if let Some(binding) = module.get_child(name, ValueNS) { + if let Success(binding) = module.resolve_name(name, ValueNS, true) { if let Some(Def::Method(did)) = binding.def() { if is_static_method(self, did) { return StaticMethod(path_names_to_string(&path, 0)); @@ -3484,34 +3453,18 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Look for trait children. build_reduced_graph::populate_module_if_necessary(self, &search_module); - for (&(_, ns), name_binding) in search_module.children.borrow().iter() { - if ns != TypeNS { continue } + search_module.for_each_child(|_, ns, name_binding| { + if ns != TypeNS { return } let trait_def_id = match name_binding.def() { Some(Def::Trait(trait_def_id)) => trait_def_id, - Some(..) | None => continue, + Some(..) | None => return, }; if self.trait_item_map.contains_key(&(name, trait_def_id)) { add_trait_info(&mut found_traits, trait_def_id, name); + let trait_name = self.get_trait_name(trait_def_id); + self.record_use(trait_name, TypeNS, name_binding); } - } - - // Look for imports. - for (&(_, ns), import) in search_module.import_resolutions.borrow().iter() { - if ns != TypeNS { continue } - let binding = match import.binding { - Some(binding) => binding, - None => continue, - }; - let did = match binding.def() { - Some(Def::Trait(trait_def_id)) => trait_def_id, - Some(..) | None => continue, - }; - if self.trait_item_map.contains_key(&(name, did)) { - add_trait_info(&mut found_traits, did, name); - let trait_name = self.get_trait_name(did); - self.record_import_use(trait_name, TypeNS, binding); - } - } + }); match search_module.parent_link { NoParentLink | ModuleParentLink(..) => break, diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 18404a49a3690..39e689ea3fb08 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -25,7 +25,6 @@ use build_reduced_graph; use rustc::lint; use rustc::middle::def::*; -use rustc::middle::def_id::DefId; use rustc::middle::privacy::*; use syntax::ast::{NodeId, Name}; @@ -100,21 +99,12 @@ impl ImportDirective { } } -#[derive(Debug)] -/// An NameResolution records what we know about an imported name in a given namespace. -/// More specifically, it records the number of unresolved `use` directives that import the name, -/// the `use` directive importing the name in the namespace, and the `NameBinding` to which the -/// name in the namespace resolves (if applicable). -/// Different `use` directives may import the same name in different namespaces. +#[derive(Clone, Copy)] +/// Records information about the resolution of a name in a module. pub struct NameResolution<'a> { - // When outstanding_references reaches zero, outside modules can count on the targets being - // correct. Before then, all bets are off; future `use` directives could override the name. - // Since shadowing is forbidden, the only way outstanding_references > 1 in a legal program - // is if the name is imported by exactly two `use` directives, one of which resolves to a - // value and the other of which resolves to a type. + /// The number of unresolved single imports that could define the name. pub outstanding_references: usize, - - /// Resolution of the name in the namespace + /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>, } @@ -125,13 +115,41 @@ impl<'a> Default for NameResolution<'a> { } impl<'a> NameResolution<'a> { - pub fn shadowable(&self) -> Shadowable { - match self.binding { - Some(binding) if binding.defined_with(DefModifiers::PRELUDE) => - Shadowable::Always, - Some(_) => Shadowable::Never, - None => Shadowable::Always, + pub fn result(&self, outstanding_globs: usize) -> ResolveResult<&'a NameBinding<'a>> { + // If no unresolved imports (single or glob) can define the name, self.binding is final. + if self.outstanding_references == 0 && outstanding_globs == 0 { + return self.binding.map(Success).unwrap_or(Failed(None)); + } + + if let Some(binding) = self.binding { + // Single imports will never be shadowable by other single or glob imports. + if !binding.defined_with(DefModifiers::GLOB_IMPORTED) { return Success(binding); } + // Non-PRELUDE glob imports will never be shadowable by other glob imports. + if self.outstanding_references == 0 && !binding.defined_with(DefModifiers::PRELUDE) { + return Success(binding); + } } + + Indeterminate + } + + // Define the name or return the existing binding if there is a collision. + pub fn try_define(&mut self, binding: &'a NameBinding<'a>) -> Result<(), &'a NameBinding<'a>> { + let is_prelude = |binding: &NameBinding| binding.defined_with(DefModifiers::PRELUDE); + let old_binding = match self.binding { + Some(old_binding) if is_prelude(binding) && !is_prelude(old_binding) => return Ok(()), + Some(old_binding) if is_prelude(old_binding) == is_prelude(binding) => old_binding, + _ => { self.binding = Some(binding); return Ok(()); } + }; + + // FIXME #31337: We currently allow items to shadow glob-imported re-exports. + if !old_binding.is_import() && binding.defined_with(DefModifiers::GLOB_IMPORTED) { + if let NameBindingKind::Import { binding, .. } = binding.kind { + if binding.is_import() { return Ok(()); } + } + } + + Err(old_binding) } } @@ -201,27 +219,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // If it's a single failed import then create a "fake" import // resolution for it so that later resolve stages won't complain. if let SingleImport(target, _) = e.import_directive.subclass { - let mut import_resolutions = e.source_module.import_resolutions.borrow_mut(); - - let resolution = import_resolutions.entry((target, ValueNS)).or_insert_with(|| { - debug!("(resolving import error) adding import resolution for `{}`", - target); - - NameResolution::default() + let dummy_binding = self.resolver.new_name_binding(NameBinding { + modifiers: DefModifiers::PRELUDE, + kind: NameBindingKind::Def(Def::Err), + span: None, }); - if resolution.binding.is_none() { - debug!("(resolving import error) adding fake target to import resolution of `{}`", - target); - - let dummy_binding = self.resolver.new_name_binding(NameBinding { - modifiers: DefModifiers::IMPORTABLE, - kind: NameBindingKind::Def(Def::Err), - span: None, - }); - - resolution.binding = Some(dummy_binding); - } + let _ = e.source_module.try_define_child(target, ValueNS, dummy_binding); + let _ = e.source_module.try_define_child(target, TypeNS, dummy_binding); } let path = import_path_to_string(&e.import_directive.module_path, @@ -360,68 +365,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }) } - /// Resolves the name in the namespace of the module because it is being imported by - /// importing_module. Returns the name bindings defining the name. - fn resolve_name_in_module(&mut self, - module: Module<'b>, // Module containing the name - name: Name, - ns: Namespace, - importing_module: Module<'b>) // Module importing the name - -> ResolveResult<&'b NameBinding<'b>> { - build_reduced_graph::populate_module_if_necessary(self.resolver, module); - if let Some(name_binding) = module.get_child(name, ns) { - if name_binding.is_extern_crate() { - // track the extern crate as used. - if let Some(DefId { krate, .. }) = name_binding.module().unwrap().def_id() { - self.resolver.used_crates.insert(krate); - } - } - return Success(name_binding); - } - - // If there is an unresolved glob at this point in the containing module, bail out. - // We don't know enough to be able to resolve the name. - if module.pub_glob_count.get() > 0 { - return Indeterminate; - } - - match module.import_resolutions.borrow().get(&(name, ns)) { - // The containing module definitely doesn't have an exported import with the - // name in question. We can therefore accurately report that names are unbound. - None => Failed(None), - - // The name is an import which has been fully resolved, so we just follow it. - Some(resolution) if resolution.outstanding_references == 0 => { - if let Some(binding) = resolution.binding { - // Import resolutions must be declared with "pub" in order to be exported. - if !binding.is_public() { - return Failed(None); - } - - self.resolver.record_import_use(name, ns, binding); - Success(binding) - } else { - Failed(None) - } - } - - // If module is the same module whose import we are resolving and - // it has an unresolved import with the same name as `name`, then the user - // is actually trying to import an item that is declared in the same scope - // - // e.g - // use self::submodule; - // pub mod submodule; - // - // In this case we continue as if we resolved the import and let - // check_for_conflicts_between_imports_and_items handle the conflict - Some(_) => match (importing_module.def_id(), module.def_id()) { - (Some(id1), Some(id2)) if id1 == id2 => Failed(None), - _ => Indeterminate - }, - } - } - fn resolve_single_import(&mut self, module_: Module<'b>, target_module: Module<'b>, @@ -448,11 +391,23 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } }; + // If this is a circular import, we temporarily count it as determined so that + // it fails (as opposed to being indeterminate) when nothing else can define it. + if target_module.def_id() == module_.def_id() && source == target { + module_.decrement_outstanding_references_for(target, ValueNS); + module_.decrement_outstanding_references_for(target, TypeNS); + } + // We need to resolve both namespaces for this to succeed. let value_result = - self.resolve_name_in_module(target_module, source, ValueNS, module_); + self.resolver.resolve_name_in_module(target_module, source, ValueNS, false, true); let type_result = - self.resolve_name_in_module(target_module, source, TypeNS, module_); + self.resolver.resolve_name_in_module(target_module, source, TypeNS, false, true); + + if target_module.def_id() == module_.def_id() && source == target { + module_.increment_outstanding_references_for(target, ValueNS); + module_.increment_outstanding_references_for(target, TypeNS); + } match (&value_result, &type_result) { (&Success(name_binding), _) if !name_binding.is_import() && @@ -488,82 +443,32 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { _ => {} } - let mut lev_suggestion = "".to_owned(); match (&value_result, &type_result) { (&Indeterminate, _) | (_, &Indeterminate) => return Indeterminate, (&Failed(_), &Failed(_)) => { let children = target_module.children.borrow(); let names = children.keys().map(|&(ref name, _)| name); - if let Some(name) = find_best_match_for_name(names, &source.as_str(), None) { - lev_suggestion = format!(". Did you mean to use `{}`?", name); - } else { - let resolutions = target_module.import_resolutions.borrow(); - let names = resolutions.keys().map(|&(ref name, _)| name); - if let Some(name) = find_best_match_for_name(names, - &source.as_str(), - None) { - lev_suggestion = - format!(". Did you mean to use the re-exported import `{}`?", name); - } - } + let lev_suggestion = match find_best_match_for_name(names, &source.as_str(), None) { + Some(name) => format!(". Did you mean to use `{}`?", name), + None => "".to_owned(), + }; + let msg = format!("There is no `{}` in `{}`{}", + source, + module_to_string(target_module), lev_suggestion); + return Failed(Some((directive.span, msg))); } _ => (), } - // We've successfully resolved the import. Write the results in. - - { - let mut import_resolutions = module_.import_resolutions.borrow_mut(); - let mut check_and_write_import = |namespace, result| { - let result: &ResolveResult<&NameBinding> = result; - - let import_resolution = import_resolutions.get_mut(&(target, namespace)).unwrap(); - let namespace_name = match namespace { - TypeNS => "type", - ValueNS => "value", - }; - - match *result { - Success(name_binding) => { - debug!("(resolving single import) found {:?} target: {:?}", - namespace_name, - name_binding.def()); - self.check_for_conflicting_import(&import_resolution, - directive.span, - target, - namespace); - - self.check_that_import_is_importable(&name_binding, - directive.span, - target); - - import_resolution.binding = - Some(self.resolver.new_name_binding(directive.import(name_binding))); - - self.add_export(module_, target, import_resolution.binding.unwrap()); - } - Failed(_) => { - // Continue. - } - Indeterminate => { - panic!("{:?} result should be known at this point", namespace_name); - } + for &(ns, result) in &[(ValueNS, &value_result), (TypeNS, &type_result)] { + if let Success(binding) = *result { + if !binding.defined_with(DefModifiers::IMPORTABLE) { + let msg = format!("`{}` is not directly importable", target); + span_err!(self.resolver.session, directive.span, E0253, "{}", &msg); } - self.check_for_conflicts_between_imports_and_items(module_, - import_resolution, - directive.span, - (target, namespace)); - }; - check_and_write_import(ValueNS, &value_result); - check_and_write_import(TypeNS, &type_result); - } - - if let (&Failed(_), &Failed(_)) = (&value_result, &type_result) { - let msg = format!("There is no `{}` in `{}`{}", - source, - module_to_string(target_module), lev_suggestion); - return Failed(Some((directive.span, msg))); + self.define(module_, target, ns, directive.import(binding)); + } } let value_def_and_priv = { @@ -624,74 +529,41 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { fn resolve_glob_import(&mut self, module_: Module<'b>, target_module: Module<'b>, - import_directive: &ImportDirective, + directive: &ImportDirective, lp: LastPrivate) -> ResolveResult<()> { - let id = import_directive.id; - - // This function works in a highly imperative manner; it eagerly adds - // everything it can to the list of import resolutions of the module - // node. - debug!("(resolving glob import) resolving glob import {}", id); - - // We must bail out if the node has unresolved imports of any kind - // (including globs). - if (*target_module).pub_count.get() > 0 { + // We must bail out if the node has unresolved imports of any kind (including globs). + if target_module.pub_count.get() > 0 { debug!("(resolving glob import) target module has unresolved pub imports; bailing out"); - return ResolveResult::Indeterminate; - } - - // Add all resolved imports from the containing module. - let import_resolutions = target_module.import_resolutions.borrow(); - - if module_.import_resolutions.borrow_state() != ::std::cell::BorrowState::Unused { - // In this case, target_module == module_ - // This means we are trying to glob import a module into itself, - // and it is a no-go - debug!("(resolving glob imports) target module is current module; giving up"); - return ResolveResult::Failed(Some((import_directive.span, - "Cannot glob-import a module into itself.".into()))); + return Indeterminate; } - for (&(name, ns), target_import_resolution) in import_resolutions.iter() { - debug!("(resolving glob import) writing module resolution {} into `{}`", - name, - module_to_string(module_)); - - // Here we merge two import resolutions. - let mut import_resolutions = module_.import_resolutions.borrow_mut(); - let mut dest_import_resolution = - import_resolutions.entry((name, ns)) - .or_insert_with(|| NameResolution::default()); - - match target_import_resolution.binding { - Some(binding) if binding.is_public() => { - self.check_for_conflicting_import(&dest_import_resolution, - import_directive.span, - name, - ns); - dest_import_resolution.binding = - Some(self.resolver.new_name_binding(import_directive.import(binding))); - self.add_export(module_, name, dest_import_resolution.binding.unwrap()); - } - _ => {} - } + if module_.def_id() == target_module.def_id() { + // This means we are trying to glob import a module into itself, and it is a no-go + let msg = "Cannot glob-import a module into itself.".into(); + return Failed(Some((directive.span, msg))); } // Add all children from the containing module. build_reduced_graph::populate_module_if_necessary(self.resolver, target_module); - - target_module.for_each_local_child(|name, ns, name_binding| { - self.merge_import_resolution(module_, - target_module, - import_directive, - (name, ns), - name_binding); + target_module.for_each_child(|name, ns, binding| { + if !binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) { return } + self.define(module_, name, ns, directive.import(binding)); + + if ns == TypeNS && directive.is_public && + binding.defined_with(DefModifiers::PRIVATE_VARIANT) { + let msg = format!("variant `{}` is private, and cannot be reexported (error \ + E0364), consider declaring its enum as `pub`", name); + self.resolver.session.add_lint(lint::builtin::PRIVATE_IN_PUBLIC, + directive.id, + directive.span, + msg); + } }); // Record the destination of this import if let Some(did) = target_module.def_id() { - self.resolver.def_map.borrow_mut().insert(id, + self.resolver.def_map.borrow_mut().insert(directive.id, PathResolution { base_def: Def::Mod(did), last_private: lp, @@ -700,61 +572,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } debug!("(resolving glob import) successfully resolved import"); - return ResolveResult::Success(()); - } - - fn merge_import_resolution(&mut self, - module_: Module<'b>, - containing_module: Module<'b>, - import_directive: &ImportDirective, - (name, ns): (Name, Namespace), - name_binding: &'b NameBinding<'b>) { - let is_public = import_directive.is_public; - - let mut import_resolutions = module_.import_resolutions.borrow_mut(); - let dest_import_resolution = import_resolutions.entry((name, ns)).or_insert_with(|| { - NameResolution::default() - }); - - debug!("(resolving glob import) writing resolution `{}` in `{}` to `{}`", - name, - module_to_string(&*containing_module), - module_to_string(module_)); - - // Merge the child item into the import resolution. - let modifier = DefModifiers::IMPORTABLE | DefModifiers::PUBLIC; - - if ns == TypeNS && is_public && name_binding.defined_with(DefModifiers::PRIVATE_VARIANT) { - let msg = format!("variant `{}` is private, and cannot be reexported (error \ - E0364), consider declaring its enum as `pub`", name); - self.resolver.session.add_lint(lint::builtin::PRIVATE_IN_PUBLIC, - import_directive.id, - import_directive.span, - msg); - } - - if name_binding.defined_with(modifier) { - let namespace_name = match ns { - TypeNS => "type", - ValueNS => "value", - }; - debug!("(resolving glob import) ... for {} target", namespace_name); - if dest_import_resolution.shadowable() == Shadowable::Never { - let msg = format!("a {} named `{}` has already been imported in this module", - namespace_name, - name); - span_err!(self.resolver.session, import_directive.span, E0251, "{}", msg); - } else { - dest_import_resolution.binding = - Some(self.resolver.new_name_binding(import_directive.import(name_binding))); - self.add_export(module_, name, dest_import_resolution.binding.unwrap()); - } - } - - self.check_for_conflicts_between_imports_and_items(module_, - dest_import_resolution, - import_directive.span, - (name, ns)); + return Success(()); } fn add_export(&mut self, module: Module<'b>, name: Name, binding: &NameBinding<'b>) { @@ -770,119 +588,70 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { self.resolver.export_map.entry(node_id).or_insert(Vec::new()).push(export); } - /// Checks that imported names and items don't have the same name. - fn check_for_conflicting_import(&mut self, - import_resolution: &NameResolution, - import_span: Span, - name: Name, - namespace: Namespace) { - let binding = &import_resolution.binding; - debug!("check_for_conflicting_import: {}; target exists: {}", - name, - binding.is_some()); - - match *binding { - Some(binding) if !binding.defined_with(DefModifiers::PRELUDE) => { - let ns_word = match namespace { - TypeNS => { - match binding.module() { - Some(ref module) if module.is_normal() => "module", - Some(ref module) if module.is_trait() => "trait", - _ => "type", - } - } - ValueNS => "value", - }; - let mut err = struct_span_err!(self.resolver.session, - import_span, - E0252, - "a {} named `{}` has already been imported \ - in this module", - ns_word, - name); - span_note!(&mut err, - binding.span.unwrap(), - "previous import of `{}` here", - name); - err.emit(); - } - Some(_) | None => {} - } - } - - /// Checks that an import is actually importable - fn check_that_import_is_importable(&mut self, - name_binding: &NameBinding, - import_span: Span, - name: Name) { - if !name_binding.defined_with(DefModifiers::IMPORTABLE) { - let msg = format!("`{}` is not directly importable", name); - span_err!(self.resolver.session, import_span, E0253, "{}", &msg[..]); + fn define(&mut self, + parent: Module<'b>, + name: Name, + ns: Namespace, + binding: NameBinding<'b>) { + let binding = self.resolver.new_name_binding(binding); + if let Err(old_binding) = parent.try_define_child(name, ns, binding) { + self.report_conflict(name, ns, binding, old_binding); + } else if binding.is_public() { + self.add_export(parent, name, binding); } } - /// Checks that imported names and items don't have the same name. - fn check_for_conflicts_between_imports_and_items(&mut self, - module: Module<'b>, - import: &NameResolution<'b>, - import_span: Span, - (name, ns): (Name, Namespace)) { - // Check for item conflicts. - let name_binding = match module.get_child(name, ns) { - None => { - // There can't be any conflicts. - return; - } - Some(name_binding) => name_binding, - }; - - if ns == ValueNS { - match import.binding { - Some(binding) if !binding.defined_with(DefModifiers::PRELUDE) => { - let mut err = struct_span_err!(self.resolver.session, - import_span, - E0255, - "import `{}` conflicts with \ - value in this module", - name); - if let Some(span) = name_binding.span { - err.span_note(span, "conflicting value here"); - } - err.emit(); - } - Some(_) | None => {} - } - } else { - match import.binding { - Some(binding) if !binding.defined_with(DefModifiers::PRELUDE) => { - if name_binding.is_extern_crate() { - let msg = format!("import `{0}` conflicts with imported crate \ - in this module (maybe you meant `use {0}::*`?)", - name); - span_err!(self.resolver.session, import_span, E0254, "{}", &msg[..]); - return; - } - - let (what, note) = match name_binding.module() { - Some(ref module) if module.is_normal() => - ("existing submodule", "note conflicting module here"), - Some(ref module) if module.is_trait() => - ("trait in this module", "note conflicting trait here"), - _ => ("type in this module", "note conflicting type here"), - }; - let mut err = struct_span_err!(self.resolver.session, - import_span, - E0256, - "import `{}` conflicts with {}", - name, - what); - if let Some(span) = name_binding.span { - err.span_note(span, note); - } - err.emit(); - } - Some(_) | None => {} - } + fn report_conflict(&mut self, + name: Name, + ns: Namespace, + binding: &'b NameBinding<'b>, + old_binding: &'b NameBinding<'b>) { + if old_binding.is_extern_crate() { + let msg = format!("import `{0}` conflicts with imported crate \ + in this module (maybe you meant `use {0}::*`?)", + name); + span_err!(self.resolver.session, binding.span.unwrap(), E0254, "{}", &msg); + } else if old_binding.is_import() { + let ns_word = match (ns, old_binding.module()) { + (ValueNS, _) => "value", + (TypeNS, Some(module)) if module.is_normal() => "module", + (TypeNS, Some(module)) if module.is_trait() => "trait", + (TypeNS, _) => "type", + }; + let mut err = struct_span_err!(self.resolver.session, + binding.span.unwrap(), + E0252, + "a {} named `{}` has already been imported \ + in this module", + ns_word, + name); + err.span_note(old_binding.span.unwrap(), + &format!("previous import of `{}` here", name)); + err.emit(); + } else if ns == ValueNS { // Check for item conflicts in the value namespace + let mut err = struct_span_err!(self.resolver.session, + binding.span.unwrap(), + E0255, + "import `{}` conflicts with value in this module", + name); + err.span_note(old_binding.span.unwrap(), "conflicting value here"); + err.emit(); + } else { // Check for item conflicts in the type namespace + let (what, note) = match old_binding.module() { + Some(ref module) if module.is_normal() => + ("existing submodule", "note conflicting module here"), + Some(ref module) if module.is_trait() => + ("trait in this module", "note conflicting trait here"), + _ => ("type in this module", "note conflicting type here"), + }; + let mut err = struct_span_err!(self.resolver.session, + binding.span.unwrap(), + E0256, + "import `{}` conflicts with {}", + name, + what); + err.span_note(old_binding.span.unwrap(), note); + err.emit(); } } } diff --git a/src/test/compile-fail/double-type-import.rs b/src/test/compile-fail/double-type-import.rs index d6d7dbb4aecd8..923f95e69d122 100644 --- a/src/test/compile-fail/double-type-import.rs +++ b/src/test/compile-fail/double-type-import.rs @@ -20,5 +20,5 @@ mod foo { } fn main() { - let _ = foo::X; //~ ERROR unresolved name `foo::X` + let _ = foo::X; } diff --git a/src/test/compile-fail/unresolved-import.rs b/src/test/compile-fail/unresolved-import.rs index b6207450d9835..b60d19fcab4aa 100644 --- a/src/test/compile-fail/unresolved-import.rs +++ b/src/test/compile-fail/unresolved-import.rs @@ -14,9 +14,9 @@ use foo::bar; //~ ERROR unresolved import `foo::bar`. Maybe a missing `extern cr use bar::Baz as x; //~ ERROR unresolved import `bar::Baz`. There is no `Baz` in `bar`. Did you mean to use `Bar`? -use food::baz; //~ ERROR unresolved import `food::baz`. There is no `baz` in `food`. Did you mean to use the re-exported import `bag`? +use food::baz; //~ ERROR unresolved import `food::baz`. There is no `baz` in `food`. Did you mean to use `bag`? -use food::{beens as Foo}; //~ ERROR unresolved import `food::beens`. There is no `beens` in `food`. Did you mean to use the re-exported import `beans`? +use food::{beens as Foo}; //~ ERROR unresolved import `food::beens`. There is no `beens` in `food`. Did you mean to use `beans`? mod bar { pub struct Bar; From d7734aebeca4d2f230bdec631ffce1416740afa5 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 7 Feb 2016 23:06:10 +0000 Subject: [PATCH 10/12] Refactor away add_export and cleanup the end of resolve_single_import --- src/librustc_resolve/resolve_imports.rs | 77 ++++++++----------------- 1 file changed, 25 insertions(+), 52 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 39e689ea3fb08..0eab53260a58b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -471,28 +471,19 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } } - let value_def_and_priv = { - module_.decrement_outstanding_references_for(target, ValueNS); - - // 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. - value_result.success().map(|binding| { - let def = binding.def().unwrap(); - let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; - (def, last_private) - }) - }; - - let type_def_and_priv = { - module_.decrement_outstanding_references_for(target, TypeNS); - - type_result.success().map(|binding| { - let def = binding.def().unwrap(); - let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; - (def, last_private) - }) + // 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. + module_.decrement_outstanding_references_for(target, ValueNS); + module_.decrement_outstanding_references_for(target, TypeNS); + + let def_and_priv = |binding: &NameBinding| { + let def = binding.def().unwrap(); + let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) }; + (def, last_private) }; + let value_def_and_priv = value_result.success().map(&def_and_priv); + let type_def_and_priv = type_result.success().map(&def_and_priv); let import_lp = LastImport { value_priv: value_def_and_priv.map(|(_, p)| p), @@ -501,22 +492,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { type_used: Used, }; - if let Some((def, _)) = value_def_and_priv { - self.resolver.def_map.borrow_mut().insert(directive.id, - PathResolution { - base_def: def, - last_private: import_lp, - depth: 0, - }); - } - if let Some((def, _)) = type_def_and_priv { - self.resolver.def_map.borrow_mut().insert(directive.id, - PathResolution { - base_def: def, - last_private: import_lp, - depth: 0, - }); - } + let write_path_resolution = |(def, _)| { + let path_resolution = + PathResolution { base_def: def, last_private: import_lp, depth: 0 }; + self.resolver.def_map.borrow_mut().insert(directive.id, path_resolution); + }; + value_def_and_priv.map(&write_path_resolution); + type_def_and_priv.map(&write_path_resolution); debug!("(resolving single import) successfully resolved import"); return Success(()); @@ -575,19 +557,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return Success(()); } - fn add_export(&mut self, module: Module<'b>, name: Name, binding: &NameBinding<'b>) { - if !binding.is_public() { return } - let node_id = match module.def_id() { - Some(def_id) => self.resolver.ast_map.as_local_node_id(def_id).unwrap(), - None => return, - }; - let export = match binding.def() { - Some(def) => Export { name: name, def_id: def.def_id() }, - None => return, - }; - self.resolver.export_map.entry(node_id).or_insert(Vec::new()).push(export); - } - fn define(&mut self, parent: Module<'b>, name: Name, @@ -596,8 +565,12 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let binding = self.resolver.new_name_binding(binding); if let Err(old_binding) = parent.try_define_child(name, ns, binding) { self.report_conflict(name, ns, binding, old_binding); - } else if binding.is_public() { - self.add_export(parent, name, binding); + } else if binding.is_public() { // Add to the export map + if let (Some(parent_def_id), Some(def)) = (parent.def_id(), binding.def()) { + let parent_node_id = self.resolver.ast_map.as_local_node_id(parent_def_id).unwrap(); + let export = Export { name: name, def_id: def.def_id() }; + self.resolver.export_map.entry(parent_node_id).or_insert(Vec::new()).push(export); + } } } From 3c62d90202ce3750d3278746c89f1a1d2908b6f9 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 6 Feb 2016 23:43:04 +0000 Subject: [PATCH 11/12] Reallow methods from traits that are shadowed by non-import items --- src/librustc_resolve/lib.rs | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index a5828067fcafc..5fbe06a868f07 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -818,6 +818,8 @@ pub struct ModuleS<'a> { // entry block for `f`. anonymous_children: RefCell>>, + shadowed_traits: RefCell>>, + // The number of unresolved globs that this module exports. glob_count: Cell, @@ -848,6 +850,7 @@ impl<'a> ModuleS<'a> { children: RefCell::new(HashMap::new()), imports: RefCell::new(Vec::new()), anonymous_children: RefCell::new(NodeMap()), + shadowed_traits: RefCell::new(Vec::new()), glob_count: Cell::new(0), pub_count: Cell::new(0), pub_glob_count: Cell::new(0), @@ -871,8 +874,19 @@ impl<'a> ModuleS<'a> { // Define the name or return the existing binding if there is a collision. fn try_define_child(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) -> Result<(), &'a NameBinding<'a>> { - self.children.borrow_mut().entry((name, ns)).or_insert_with(Default::default) - .try_define(binding) + let mut children = self.children.borrow_mut(); + let resolution = children.entry((name, ns)).or_insert_with(Default::default); + + // FIXME #31379: We can use methods from imported traits shadowed by non-import items + if let Some(old_binding) = resolution.binding { + if !old_binding.is_import() && binding.is_import() { + if let Some(Def::Trait(_)) = binding.def() { + self.shadowed_traits.borrow_mut().push(binding); + } + } + } + + resolution.try_define(binding) } fn increment_outstanding_references_for(&self, name: Name, ns: Namespace) { @@ -3466,6 +3480,16 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } }); + // Look for shadowed traits. + for binding in search_module.shadowed_traits.borrow().iter() { + let did = binding.def().unwrap().def_id(); + if self.trait_item_map.contains_key(&(name, did)) { + add_trait_info(&mut found_traits, did, name); + let trait_name = self.get_trait_name(did); + self.record_use(trait_name, TypeNS, binding); + } + } + match search_module.parent_link { NoParentLink | ModuleParentLink(..) => break, BlockParentLink(parent_module, _) => { From 3df40c09ec5efb74e16c3da84fef6ce142dae73a Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Tue, 9 Feb 2016 06:26:27 +0000 Subject: [PATCH 12/12] Allow prelude imports to shadow eachother (needed for the [pretty] tests) Derive the Default impl for NameResolution --- src/librustc_resolve/resolve_imports.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 0eab53260a58b..6667e48987087 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -99,7 +99,7 @@ impl ImportDirective { } } -#[derive(Clone, Copy)] +#[derive(Clone, Default)] /// Records information about the resolution of a name in a module. pub struct NameResolution<'a> { /// The number of unresolved single imports that could define the name. @@ -108,12 +108,6 @@ pub struct NameResolution<'a> { pub binding: Option<&'a NameBinding<'a>>, } -impl<'a> Default for NameResolution<'a> { - fn default() -> Self { - NameResolution { outstanding_references: 0, binding: None } - } -} - impl<'a> NameResolution<'a> { pub fn result(&self, outstanding_globs: usize) -> ResolveResult<&'a NameBinding<'a>> { // If no unresolved imports (single or glob) can define the name, self.binding is final. @@ -137,8 +131,8 @@ impl<'a> NameResolution<'a> { pub fn try_define(&mut self, binding: &'a NameBinding<'a>) -> Result<(), &'a NameBinding<'a>> { let is_prelude = |binding: &NameBinding| binding.defined_with(DefModifiers::PRELUDE); let old_binding = match self.binding { - Some(old_binding) if is_prelude(binding) && !is_prelude(old_binding) => return Ok(()), - Some(old_binding) if is_prelude(old_binding) == is_prelude(binding) => old_binding, + Some(_) if is_prelude(binding) => return Ok(()), + Some(old_binding) if !is_prelude(old_binding) => old_binding, _ => { self.binding = Some(binding); return Ok(()); } };