From 845ad1b4ed9e1ca67abaa20414698c1fa0959489 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 17 Feb 2016 09:29:17 +0000 Subject: [PATCH 1/2] Stop trying to resolve an import directive after the resolution fails --- src/librustc_resolve/resolve_imports.rs | 37 ++++++++++++++---------- src/test/compile-fail/import-shadow-6.rs | 4 +-- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 1253d4f9ceff0..459a7392f6cf4 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -173,13 +173,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { fn resolve_imports(&mut self) { let mut i = 0; let mut prev_unresolved_imports = 0; + let mut errors = Vec::new(); + loop { debug!("(resolving imports) iteration {}, {} imports left", i, self.resolver.unresolved_imports); - let module_root = self.resolver.graph_root; - let errors = self.resolve_imports_for_module_subtree(module_root); + self.resolve_imports_for_module_subtree(self.resolver.graph_root, &mut errors); if self.resolver.unresolved_imports == 0 { debug!("(resolving imports) success"); @@ -197,7 +198,7 @@ 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.resolver.report_unresolved_imports(module_root); + self.resolver.report_unresolved_imports(self.resolver.graph_root); } break; } @@ -234,30 +235,27 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// Attempts to resolve imports for the given module and all of its /// submodules. fn resolve_imports_for_module_subtree(&mut self, - module_: Module<'b>) - -> Vec> { - let mut errors = Vec::new(); + 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_); - errors.extend(self.resolve_imports_for_module(module_)); + self.resolve_imports_for_module(module_, errors); self.resolver.current_module = orig_module; for (_, child_module) in module_.module_children.borrow().iter() { - errors.extend(self.resolve_imports_for_module_subtree(child_module)); + self.resolve_imports_for_module_subtree(child_module, errors); } - - errors } /// Attempts to resolve imports for the given module only. - fn resolve_imports_for_module(&mut self, module: Module<'b>) -> Vec> { - let mut errors = Vec::new(); - + fn resolve_imports_for_module(&mut self, + module: Module<'b>, + errors: &mut Vec>) { if module.all_imports_resolved() { debug!("(resolving imports for module) all imports resolved for {}", module_to_string(&module)); - return errors; + return; } let mut imports = module.imports.borrow_mut(); @@ -278,6 +276,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { span: span, help: help, }); + module.resolved_import_count.set(module.resolved_import_count.get() + 1); + continue; } ResolveResult::Indeterminate => {} ResolveResult::Success(()) => { @@ -293,8 +293,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } imports.extend(indeterminate_imports); - - errors } /// Attempts to resolve the given import. The return value indicates @@ -562,6 +560,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { ns: Namespace, binding: &'b NameBinding<'b>, old_binding: &'b NameBinding<'b>) { + // Error on the second of two conflicting imports + if old_binding.is_import() && binding.is_import() && + old_binding.span.unwrap().lo > binding.span.unwrap().lo { + self.report_conflict(name, ns, old_binding, binding); + return; + } + if old_binding.is_extern_crate() { let msg = format!("import `{0}` conflicts with imported crate \ in this module (maybe you meant `use {0}::*`?)", diff --git a/src/test/compile-fail/import-shadow-6.rs b/src/test/compile-fail/import-shadow-6.rs index 0f3d54d5fe3d8..fa3b75c70f0b6 100644 --- a/src/test/compile-fail/import-shadow-6.rs +++ b/src/test/compile-fail/import-shadow-6.rs @@ -12,8 +12,8 @@ #![no_implicit_prelude] -use qux::*; //~ERROR a type named `Baz` has already been imported in this module -use foo::*; +use qux::*; +use foo::*; //~ERROR a type named `Baz` has already been imported in this module mod foo { pub type Baz = isize; From 5ad84f130169136cc32b63d9172143ef4422a09f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 17 Feb 2016 22:45:05 +0000 Subject: [PATCH 2/2] Replace the field `imports` in Module with `unresolved_imports` and refactor away `resolved_import_count` --- src/librustc_resolve/build_reduced_graph.rs | 7 ++-- src/librustc_resolve/lib.rs | 28 +++------------- src/librustc_resolve/resolve_imports.rs | 37 +++++---------------- 3 files changed, 16 insertions(+), 56 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 5c94c6e4369b0..a25968174fd4e 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 Module; use Namespace::{self, TypeNS, ValueNS}; use {NameBinding, NameBindingKind}; -use {names_to_string, module_to_string}; +use module_to_string; use ParentLink::{ModuleParentLink, BlockParentLink}; use Resolver; use resolve_imports::Shadowable; @@ -682,7 +682,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { id: NodeId, is_public: bool, shadowable: Shadowable) { - module_.imports + module_.unresolved_imports .borrow_mut() .push(ImportDirective::new(module_path, subclass, span, id, is_public, shadowable)); self.unresolved_imports += 1; @@ -696,9 +696,6 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { match subclass { SingleImport(target, _) => { - debug!("(building import directive) building import directive: {}::{}", - names_to_string(&module_.imports.borrow().last().unwrap().module_path), - target); module_.increment_outstanding_references_for(target, ValueNS); module_.increment_outstanding_references_for(target, TypeNS); } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 3244d2f1d9609..6d04e69138b1f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -18,7 +18,6 @@ #![cfg_attr(not(stage0), deny(warnings))] #![feature(associated_consts)] -#![feature(borrow_state)] #![feature(rustc_diagnostic_macros)] #![feature(rustc_private)] #![feature(staged_api)] @@ -799,7 +798,7 @@ pub struct ModuleS<'a> { is_extern_crate: bool, resolutions: RefCell>>, - imports: RefCell>, + unresolved_imports: RefCell>, // The module children of this node, including normal modules and anonymous modules. // Anonymous children are pseudo-modules that are implicitly created around items @@ -828,9 +827,6 @@ pub struct ModuleS<'a> { // The number of unresolved pub glob imports in this module pub_glob_count: Cell, - // The index of the import we're resolving. - resolved_import_count: Cell, - // Whether this module is populated. If not populated, any attempt to // access the children must be preceded with a // `populate_module_if_necessary` call. @@ -847,13 +843,12 @@ impl<'a> ModuleS<'a> { is_public: is_public, is_extern_crate: false, resolutions: RefCell::new(HashMap::new()), - imports: RefCell::new(Vec::new()), + unresolved_imports: RefCell::new(Vec::new()), module_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), - resolved_import_count: Cell::new(0), populated: Cell::new(!external), } } @@ -924,15 +919,6 @@ impl<'a> ModuleS<'a> { } } - fn all_imports_resolved(&self) -> bool { - if self.imports.borrow_state() == ::std::cell::BorrowState::Writing { - // it is currently being resolved ! so nope - false - } else { - self.imports.borrow().len() == self.resolved_import_count.get() - } - } - pub fn inc_glob_count(&self) { self.glob_count.set(self.glob_count.get() + 1); } @@ -1622,13 +1608,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } fn report_unresolved_imports(&mut self, module_: Module<'a>) { - let index = module_.resolved_import_count.get(); - let imports = module_.imports.borrow(); - let import_count = imports.len(); - if index != import_count { - resolve_error(self, - (*imports)[index].span, - ResolutionError::UnresolvedImport(None)); + for import in module_.unresolved_imports.borrow().iter() { + resolve_error(self, import.span, ResolutionError::UnresolvedImport(None)); + break; } // Descend into children and anonymous children. diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 459a7392f6cf4..a039675428215 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -252,47 +252,28 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { fn resolve_imports_for_module(&mut self, module: Module<'b>, errors: &mut Vec>) { - if module.all_imports_resolved() { - debug!("(resolving imports for module) all imports resolved for {}", - module_to_string(&module)); - return; - } + let mut imports = Vec::new(); + let mut unresolved_imports = module.unresolved_imports.borrow_mut(); + ::std::mem::swap(&mut imports, &mut unresolved_imports); - let mut imports = module.imports.borrow_mut(); - let import_count = imports.len(); - let mut indeterminate_imports = Vec::new(); - while module.resolved_import_count.get() + indeterminate_imports.len() < import_count { - let import_index = module.resolved_import_count.get(); - match self.resolve_import_for_module(module, &imports[import_index]) { - ResolveResult::Failed(err) => { - let import_directive = &imports[import_index]; + for import_directive in imports { + match self.resolve_import_for_module(module, &import_directive) { + Failed(err) => { let (span, help) = match err { Some((span, msg)) => (span, format!(". {}", msg)), None => (import_directive.span, String::new()), }; errors.push(ImportResolvingError { source_module: module, - import_directive: import_directive.clone(), + import_directive: import_directive, span: span, help: help, }); - module.resolved_import_count.set(module.resolved_import_count.get() + 1); - continue; - } - ResolveResult::Indeterminate => {} - ResolveResult::Success(()) => { - // count success - module.resolved_import_count - .set(module.resolved_import_count.get() + 1); - continue; } + Indeterminate => unresolved_imports.push(import_directive), + Success(()) => {} } - // This resolution was not successful, keep it for later - indeterminate_imports.push(imports.swap_remove(import_index)); - } - - imports.extend(indeterminate_imports); } /// Attempts to resolve the given import. The return value indicates