Auto merge of #31747 - jseyfried:stop_resolve_after_fail, r=nrc

Now that #31461 is merged, a failing resolution can never become indeterminate or succeed, so we no longer have to keep trying to resolve failing import directives.
r? @nrc

4 files changed, 37 insertions, 72 deletions

diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs
index 5c94c6e4369..a25968174fd 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 0508a9ef729..11c51522b67 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)]
@@ -810,7 +809,7 @@ pub struct ModuleS<'a> {
     is_extern_crate: bool,
 
     resolutions: RefCell<HashMap<(Name, Namespace), NameResolution<'a>>>,
-    imports: RefCell<Vec<ImportDirective>>,
+    unresolved_imports: RefCell<Vec<ImportDirective>>,
 
     // The module children of this node, including normal modules and anonymous modules.
     // Anonymous children are pseudo-modules that are implicitly created around items
@@ -839,9 +838,6 @@ pub struct ModuleS<'a> {
     // The number of unresolved pub glob imports in this module
     pub_glob_count: Cell<usize>,
 
-    // The index of the import we're resolving.
-    resolved_import_count: Cell<usize>,
-
     // Whether this module is populated. If not populated, any attempt to
     // access the children must be preceded with a
     // `populate_module_if_necessary` call.
@@ -859,13 +855,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),
         }
     }
@@ -936,15 +931,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);
     }
@@ -1635,13 +1621,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 ce56f484b02..9c6e51c647c 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;
             }
@@ -236,67 +237,45 @@ 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<ImportResolvingError<'b>> {
-        let mut errors = Vec::new();
+                                          module_: Module<'b>,
+                                          errors: &mut Vec<ImportResolvingError<'b>>) {
         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<ImportResolvingError<'b>> {
-        let mut errors = Vec::new();
-
-        if module.all_imports_resolved() {
-            debug!("(resolving imports for module) all imports resolved for {}",
-                   module_to_string(&module));
-            return errors;
-        }
-
-        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];
+    fn resolve_imports_for_module(&mut self,
+                                  module: Module<'b>,
+                                  errors: &mut Vec<ImportResolvingError<'b>>) {
+        let mut imports = Vec::new();
+        let mut unresolved_imports = module.unresolved_imports.borrow_mut();
+        ::std::mem::swap(&mut imports, &mut unresolved_imports);
+
+        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,
                     });
                 }
-                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);
-
-        errors
     }
 
     /// Attempts to resolve the given import. The return value indicates
@@ -564,6 +543,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 0f3d54d5fe3..fa3b75c70f0 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;