From 5f4791555486c856fdf4b1c782305a589b9cbf25 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 17 Apr 2016 19:23:25 +0000 Subject: [PATCH 1/4] Refactor `is_prelude` to only apply to glob imports --- src/librustc_resolve/build_reduced_graph.rs | 25 ++++++--------------- src/librustc_resolve/resolve_imports.rs | 21 ++++++++--------- 2 files changed, 16 insertions(+), 30 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 2547c756b9d6a..51d00266b7191 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -177,13 +177,9 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { } let subclass = ImportDirectiveSubclass::single(binding, source_name); + let span = view_path.span; + parent.add_import_directive(module_path, subclass, span, item.id, vis); self.unresolved_imports += 1; - parent.add_import_directive(module_path, - subclass, - view_path.span, - item.id, - vis, - is_prelude); } ViewPathList(_, ref source_items) => { // Make sure there's at most one `mod` import in the list. @@ -228,23 +224,16 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { } }; let subclass = ImportDirectiveSubclass::single(rename, name); + let (span, id) = (source_item.span, source_item.node.id()); + parent.add_import_directive(module_path, subclass, span, id, vis); self.unresolved_imports += 1; - parent.add_import_directive(module_path, - subclass, - source_item.span, - source_item.node.id(), - vis, - is_prelude); } } ViewPathGlob(_) => { + let subclass = GlobImport { is_prelude: is_prelude }; + let span = view_path.span; + parent.add_import_directive(module_path, subclass, span, item.id, vis); self.unresolved_imports += 1; - parent.add_import_directive(module_path, - GlobImport, - view_path.span, - item.id, - vis, - is_prelude); } } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 03492043dd449..a337cb6a4c113 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -41,7 +41,7 @@ pub enum ImportDirectiveSubclass { type_determined: Cell, value_determined: Cell, }, - GlobImport, + GlobImport { is_prelude: bool }, } impl ImportDirectiveSubclass { @@ -64,7 +64,6 @@ pub struct ImportDirective<'a> { subclass: ImportDirectiveSubclass, span: Span, vis: ty::Visibility, // see note in ImportResolutionPerNamespace about how to use this - is_prelude: bool, } impl<'a> ImportDirective<'a> { @@ -84,7 +83,7 @@ impl<'a> ImportDirective<'a> { } pub fn is_glob(&self) -> bool { - match self.subclass { ImportDirectiveSubclass::GlobImport => true, _ => false } + match self.subclass { ImportDirectiveSubclass::GlobImport { .. } => true, _ => false } } } @@ -191,7 +190,7 @@ impl<'a> NameResolution<'a> { }; let name = match directive.subclass { SingleImport { source, .. } => source, - GlobImport => unreachable!(), + GlobImport { .. } => unreachable!(), }; match target_module.resolve_name(name, ns, false) { Failed(_) => {} @@ -282,8 +281,7 @@ impl<'a> ::ModuleS<'a> { subclass: ImportDirectiveSubclass, span: Span, id: NodeId, - vis: ty::Visibility, - is_prelude: bool) { + vis: ty::Visibility) { let directive = self.arenas.alloc_import_directive(ImportDirective { module_path: module_path, target_module: Cell::new(None), @@ -291,7 +289,6 @@ impl<'a> ::ModuleS<'a> { span: span, id: id, vis: vis, - is_prelude: is_prelude, }); self.unresolved_imports.borrow_mut().push(directive); @@ -304,8 +301,8 @@ impl<'a> ::ModuleS<'a> { } // We don't add prelude imports to the globs since they only affect lexical scopes, // which are not relevant to import resolution. - GlobImport if directive.is_prelude => {} - GlobImport => self.globs.borrow_mut().push(directive), + GlobImport { is_prelude: true } => {} + GlobImport { .. } => self.globs.borrow_mut().push(directive), } } @@ -496,7 +493,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let (source, target, value_determined, type_determined) = match directive.subclass { SingleImport { source, target, ref value_determined, ref type_determined } => (source, target, value_determined, type_determined), - GlobImport => return self.resolve_glob_import(target_module, directive), + GlobImport { .. } => return self.resolve_glob_import(target_module, directive), }; // We need to resolve both namespaces for this to succeed. @@ -644,7 +641,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } self.resolver.populate_module_if_necessary(target_module); - if directive.is_prelude { + if let GlobImport { is_prelude: true } = directive.subclass { *module_.prelude.borrow_mut() = Some(target_module); return Success(()); } @@ -747,7 +744,7 @@ fn import_path_to_string(names: &[Name], subclass: &ImportDirectiveSubclass) -> fn import_directive_subclass_to_string(subclass: &ImportDirectiveSubclass) -> String { match *subclass { SingleImport { source, .. } => source.to_string(), - GlobImport => "*".to_string(), + GlobImport { .. } => "*".to_string(), } } From 5b12832012c4d92296889d03737f0705ac2d5698 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 17 Apr 2016 20:23:10 +0000 Subject: [PATCH 2/4] Make import resolution and error resolution reporting deterministic. These tasks used to depend on the iteration order of `module_children`. --- src/librustc_resolve/lib.rs | 11 +++++++- src/librustc_resolve/resolve_imports.rs | 37 ++++++++----------------- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index f32cf7aa9f41a..cf302886f11e1 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1103,6 +1103,7 @@ pub struct Resolver<'a, 'tcx: 'a> { struct ResolverArenas<'a> { modules: arena::TypedArena>, + local_modules: RefCell>>, name_bindings: arena::TypedArena>, import_directives: arena::TypedArena>, name_resolutions: arena::TypedArena>>, @@ -1110,7 +1111,14 @@ struct ResolverArenas<'a> { impl<'a> ResolverArenas<'a> { fn alloc_module(&'a self, module: ModuleS<'a>) -> Module<'a> { - self.modules.alloc(module) + let module = self.modules.alloc(module); + if module.def_id().map(|def_id| def_id.is_local()).unwrap_or(true) { + self.local_modules.borrow_mut().push(module); + } + module + } + fn local_modules(&'a self) -> ::std::cell::Ref<'a, Vec>> { + self.local_modules.borrow() } fn alloc_name_binding(&'a self, name_binding: NameBinding<'a>) -> &'a NameBinding<'a> { self.name_bindings.alloc(name_binding) @@ -1189,6 +1197,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { fn arenas() -> ResolverArenas<'a> { ResolverArenas { modules: arena::TypedArena::new(), + local_modules: RefCell::new(Vec::new()), name_bindings: arena::TypedArena::new(), import_directives: arena::TypedArena::new(), name_resolutions: arena::TypedArena::new(), diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index a337cb6a4c113..69536b3f1bbdc 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -29,7 +29,6 @@ use syntax::attr::AttrMetaMethods; use syntax::codemap::Span; use syntax::util::lev_distance::find_best_match_for_name; -use std::mem::replace; use std::cell::{Cell, RefCell}; /// Contains data for specific types of import directives. @@ -371,11 +370,17 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { i, self.resolver.unresolved_imports); - self.resolve_imports_for_module_subtree(self.resolver.graph_root, &mut errors); + // Attempt to resolve imports in all local modules. + for module in self.resolver.arenas.local_modules().iter() { + self.resolver.current_module = module; + self.resolve_imports_in_current_module(&mut errors); + } if self.resolver.unresolved_imports == 0 { debug!("(resolving imports) success"); - self.finalize_resolutions(self.resolver.graph_root, false); + for module in self.resolver.arenas.local_modules().iter() { + self.finalize_resolutions_in(module, false); + } break; } @@ -385,7 +390,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // to avoid generating multiple errors on the same import. // Imports that are still indeterminate at this point are actually blocked // by errored imports, so there is no point reporting them. - self.finalize_resolutions(self.resolver.graph_root, errors.len() == 0); + for module in self.resolver.arenas.local_modules().iter() { + self.finalize_resolutions_in(module, errors.len() == 0); + } for e in errors { self.import_resolving_error(e) } @@ -422,22 +429,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { ResolutionError::UnresolvedImport(Some((&path, &e.help)))); } - /// Attempts to resolve imports for the given module and all of its - /// submodules. - fn resolve_imports_for_module_subtree(&mut self, - module_: Module<'b>, - errors: &mut Vec>) { - debug!("(resolving imports for module subtree) resolving {}", - module_to_string(&module_)); - let orig_module = replace(&mut self.resolver.current_module, module_); - self.resolve_imports_in_current_module(errors); - self.resolver.current_module = orig_module; - - for (_, child_module) in module_.module_children.borrow().iter() { - self.resolve_imports_for_module_subtree(child_module, errors); - } - } - /// Attempts to resolve imports for the given module only. fn resolve_imports_in_current_module(&mut self, errors: &mut Vec>) { let mut imports = Vec::new(); @@ -675,7 +666,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // Miscellaneous post-processing, including recording reexports, recording shadowed traits, // reporting conflicts, reporting the PRIVATE_IN_PUBLIC lint, and reporting unresolved imports. - fn finalize_resolutions(&mut self, module: Module<'b>, report_unresolved_imports: bool) { + fn finalize_resolutions_in(&mut self, module: Module<'b>, report_unresolved_imports: bool) { // Since import resolution is finished, globs will not define any more names. *module.globs.borrow_mut() = Vec::new(); @@ -723,10 +714,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { break; } } - - for (_, child) in module.module_children.borrow().iter() { - self.finalize_resolutions(child, report_unresolved_imports); - } } } From b01e63009fdae72b2bb2ca1892ef55eede8d80dc Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sun, 17 Apr 2016 20:41:57 +0000 Subject: [PATCH 3/4] Refactor the per-module node map `module_children` into a per-resolver map. --- src/librustc_resolve/build_reduced_graph.rs | 4 +-- src/librustc_resolve/lib.rs | 40 ++++++++++----------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 51d00266b7191..effc751c50759 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -260,7 +260,7 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { let def = Def::Mod(self.ast_map.local_def_id(item.id)); let module = self.new_module(parent_link, Some(def), false, vis); self.define(parent, name, TypeNS, (module, sp)); - parent.module_children.borrow_mut().insert(item.id, module); + self.module_map.insert(item.id, module); *parent_ref = module; } @@ -398,7 +398,7 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> { let parent_link = BlockParentLink(parent, block_id); let new_module = self.new_module(parent_link, None, false, parent.vis); - parent.module_children.borrow_mut().insert(block_id, new_module); + self.module_map.insert(block_id, new_module); *parent = new_module; } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index cf302886f11e1..d5e0d35950fa9 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -827,22 +827,6 @@ pub struct ModuleS<'a> { resolutions: RefCell>>>, unresolved_imports: RefCell>>, - // The module children of this node, including normal modules and anonymous modules. - // Anonymous children are pseudo-modules that are implicitly created around items - // contained within blocks. - // - // For example, if we have this: - // - // fn f() { - // fn g() { - // ... - // } - // } - // - // There will be an anonymous module created around `g` with the ID of the - // entry block for `f`. - module_children: RefCell>>, - prelude: RefCell>>, glob_importers: RefCell, &'a ImportDirective<'a>)>>, @@ -871,7 +855,6 @@ impl<'a> ModuleS<'a> { extern_crate_id: None, resolutions: RefCell::new(HashMap::new()), unresolved_imports: RefCell::new(Vec::new()), - module_children: RefCell::new(NodeMap()), prelude: RefCell::new(None), glob_importers: RefCell::new(Vec::new()), globs: RefCell::new((Vec::new())), @@ -1078,6 +1061,22 @@ pub struct Resolver<'a, 'tcx: 'a> { export_map: ExportMap, trait_map: TraitMap, + // A map from nodes to modules, both normal (`mod`) modules and anonymous modules. + // Anonymous modules are pseudo-modules that are implicitly created around items + // contained within blocks. + // + // For example, if we have this: + // + // fn f() { + // fn g() { + // ... + // } + // } + // + // There will be an anonymous module created around `g` with the ID of the + // entry block for `f`. + module_map: NodeMap>, + // Whether or not to print error messages. Can be set to true // when getting additional info for error message suggestions, // so as to avoid printing duplicate errors @@ -1179,6 +1178,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { freevars_seen: NodeMap(), export_map: NodeMap(), trait_map: NodeMap(), + module_map: NodeMap(), used_imports: HashSet::new(), used_crates: HashSet::new(), @@ -1578,7 +1578,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { fn with_scope(&mut self, id: NodeId, f: F) where F: FnOnce(&mut Resolver) { - if let Some(module) = self.current_module.module_children.borrow().get(&id) { + let module = self.module_map.get(&id).cloned(); // clones a reference + if let Some(module) = module { // Move down in the graph. let orig_module = ::std::mem::replace(&mut self.current_module, module); self.value_ribs.push(Rib::new(ModuleRibKind(module))); @@ -2129,8 +2130,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { debug!("(resolving block) entering block"); // Move down in the graph, if there's an anonymous module rooted here. let orig_module = self.current_module; - let anonymous_module = - orig_module.module_children.borrow().get(&block.id).map(|module| *module); + let anonymous_module = self.module_map.get(&block.id).cloned(); // clones a reference if let Some(anonymous_module) = anonymous_module { debug!("(resolving block) found anonymous module, moving down"); From 1e134a47d35bb82ccf41338f1c485a479f7807a5 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 18 Apr 2016 00:26:18 +0000 Subject: [PATCH 4/4] Update outdated comment --- src/librustc_resolve/resolve_imports.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 69536b3f1bbdc..f335f145a1072 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -664,8 +664,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return Success(()); } - // Miscellaneous post-processing, including recording reexports, recording shadowed traits, - // reporting conflicts, reporting the PRIVATE_IN_PUBLIC lint, and reporting unresolved imports. + // Miscellaneous post-processing, including recording reexports, reporting conflicts, + // reporting the PRIVATE_IN_PUBLIC lint, and reporting unresolved imports. fn finalize_resolutions_in(&mut self, module: Module<'b>, report_unresolved_imports: bool) { // Since import resolution is finished, globs will not define any more names. *module.globs.borrow_mut() = Vec::new();