Auto merge of #27439 - vberger:more_perseverant_resolve, r=nrc

(This is a second try at #26242. This time I think things should be ok.)

The current algorithm handling import resolutions works sequentially, handling imports in the order they appear in the source file, and blocking/bailing on the first one generating an error/being unresolved.

This can lead to situations where the order of the `use` statements can make the difference between "this code compiles" and "this code fails on an unresolved import" (see #18083 for example). This is especially true when considering glob imports.

This PR changes the behaviour of the algorithm to instead try to resolve all imports in a module. If one fails, it is recorded and the next one is tried (instead of directly giving up). Also, all errors generated are stored (and not reported directly).

The main loop of the algorithms guaranties that the algorithm will always finish: if a round of resolution does not resolve anything new, we are stuck and give up. At this point, the new version of the algorithm will display all errors generated by the last round of resolve. This way we are sure to not silence relevant errors or help messages, but also to not give up too early.

**As a consequence, the import resolution becomes independent of the order in which the `use` statements are written in the source files.** I personally don't see any situations where this could be a problem, but this might need some thought.

I passed `rpass` and `cfail` tests on my computer, and now am compiling a full stage2 compiler to ensure the crates reporting errors in my previous attempts still build correctly. I guess once I have checked it, this will need a crater run?

Fixes #18083.

r? @alexcrichton , cc @nrc @brson

src/librustc_resolve/lib.rs

@@ -20,6 +20,7 @@
       html_root_url = "http://doc.rust-lang.org/nightly/")]
 
 #![feature(associated_consts)]
+#![feature(borrow_state)]
 #![feature(rc_weak)]
 #![feature(rustc_diagnostic_macros)]
 #![feature(rustc_private)]
@@ -176,7 +177,7 @@ pub enum ResolutionError<'a> {
     /// error E0431: `self` import can only appear in an import list with a non-empty prefix
     SelfImportOnlyInImportListWithNonEmptyPrefix,
     /// error E0432: unresolved import
-    UnresolvedImport(Option<(&'a str, Option<&'a str>)>),
+    UnresolvedImport(Option<(&'a str, &'a str)>),
     /// error E0433: failed to resolve
     FailedToResolve(&'a str),
     /// error E0434: can't capture dynamic environment in a fn item
@@ -359,8 +360,7 @@ fn resolve_error<'b, 'a:'b, 'tcx:'a>(resolver: &'b Resolver<'a, 'tcx>, span: syn
         }
         ResolutionError::UnresolvedImport(name) => {
             let msg = match name {
-                Some((n, Some(p))) => format!("unresolved import `{}`{}", n, p),
-                Some((n, None)) => format!("unresolved import (maybe you meant `{}::*`?)", n),
+                Some((n, p)) => format!("unresolved import `{}`{}", n, p),
                 None => "unresolved import".to_owned()
             };
             span_err!(resolver.session, span, E0432, "{}", msg);
@@ -539,8 +539,8 @@ enum ResolveResult<T> {
 }
 
 impl<T> ResolveResult<T> {
-    fn indeterminate(&self) -> bool {
-        match *self { Indeterminate => true, _ => false }
+    fn success(&self) -> bool {
+        match *self { Success(_) => true, _ => false }
     }
 }
 
@@ -732,7 +732,12 @@ impl Module {
     }
 
     fn all_imports_resolved(&self) -> bool {
-        self.imports.borrow().len() == self.resolved_import_count.get()
+        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()
+        }
     }
 }
 
@@ -1815,19 +1820,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         let imports = module_.imports.borrow();
         let import_count = imports.len();
         if index != import_count {
-            let sn = self.session
-                         .codemap()
-                         .span_to_snippet((*imports)[index].span)
-                         .unwrap();
-            if sn.contains("::") {
-                resolve_error(self,
-                              (*imports)[index].span,
-                              ResolutionError::UnresolvedImport(None));
-            } else {
-                resolve_error(self,
-                              (*imports)[index].span,
-                              ResolutionError::UnresolvedImport(Some((&*sn, None))));
-            }
+            resolve_error(self,
+                          (*imports)[index].span,
+                          ResolutionError::UnresolvedImport(None));
         }
 
         // Descend into children and anonymous children.

src/librustc_resolve/resolve_imports.rs

@@ -184,6 +184,11 @@ impl ImportResolution {
     }
 }
 
+struct ImportResolvingError {
+    span: Span,
+    path: String,
+    help: String,
+}
 
 struct ImportResolver<'a, 'b:'a, 'tcx:'b> {
     resolver: &'a mut Resolver<'b, 'tcx>
@@ -208,15 +213,28 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                    i, self.resolver.unresolved_imports);
 
             let module_root = self.resolver.graph_root.get_module();
-            self.resolve_imports_for_module_subtree(module_root.clone());
+            let errors = self.resolve_imports_for_module_subtree(module_root.clone());
 
             if self.resolver.unresolved_imports == 0 {
                 debug!("(resolving imports) success");
                 break;
             }
 
             if self.resolver.unresolved_imports == prev_unresolved_imports {
-                self.resolver.report_unresolved_imports(module_root);
+                // resolving failed
+                if errors.len() > 0 {
+                    for e in errors {
+                        resolve_error(self.resolver,
+                                      e.span,
+                                      ResolutionError::UnresolvedImport(Some((&e.path, &e.help))));
+                    }
+                } else {
+                    // Report unresolved imports only if no hard error was already reported
+                    // 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);
+                }
                 break;
             }
 
@@ -227,11 +245,13 @@ 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_: Rc<Module>) {
+    fn resolve_imports_for_module_subtree(&mut self, module_: Rc<Module>)
+                                          -> Vec<ImportResolvingError> {
+        let mut errors = Vec::new();
         debug!("(resolving imports for module subtree) resolving {}",
                module_to_string(&*module_));
         let orig_module = replace(&mut self.resolver.current_module, module_.clone());
-        self.resolve_imports_for_module(module_.clone());
+        errors.extend(self.resolve_imports_for_module(module_.clone()));
         self.resolver.current_module = orig_module;
 
         build_reduced_graph::populate_module_if_necessary(self.resolver, &module_);
@@ -241,53 +261,67 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                     // Nothing to do.
                 }
                 Some(child_module) => {
-                    self.resolve_imports_for_module_subtree(child_module);
+                    errors.extend(self.resolve_imports_for_module_subtree(child_module));
                 }
             }
         }
 
         for (_, child_module) in module_.anonymous_children.borrow().iter() {
-            self.resolve_imports_for_module_subtree(child_module.clone());
+            errors.extend(self.resolve_imports_for_module_subtree(child_module.clone()));
         }
+
+        errors
     }
 
     /// Attempts to resolve imports for the given module only.
-    fn resolve_imports_for_module(&mut self, module: Rc<Module>) {
+    fn resolve_imports_for_module(&mut self, module: Rc<Module>) -> Vec<ImportResolvingError> {
+        let mut errors = Vec::new();
+
         if module.all_imports_resolved() {
             debug!("(resolving imports for module) all imports resolved for \
                    {}",
                    module_to_string(&*module));
-            return;
+            return errors;
         }
 
-        let imports = module.imports.borrow();
+        let mut imports = module.imports.borrow_mut();
         let import_count = imports.len();
-        while module.resolved_import_count.get() < import_count {
+        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();
-            let import_directive = &(*imports)[import_index];
             match self.resolve_import_for_module(module.clone(),
-                                                 import_directive) {
+                                                 &imports[import_index]) {
                 ResolveResult::Failed(err) => {
+                    let import_directive = &imports[import_index];
                     let (span, help) = match err {
                         Some((span, msg)) => (span, format!(". {}", msg)),
                         None => (import_directive.span, String::new())
                     };
-                    resolve_error(self.resolver,
-                                    span,
-                                    ResolutionError::UnresolvedImport(
-                                                Some((&*import_path_to_string(
-                                                        &import_directive.module_path,
-                                                        import_directive.subclass),
-                                                      Some(&*help))))
-                                   );
+                    errors.push(ImportResolvingError {
+                                    span: span,
+                                    path: import_path_to_string(
+                                            &import_directive.module_path,
+                                            import_directive.subclass
+                                         ),
+                                    help: help
+                                });
+                }
+                ResolveResult::Indeterminate => {}
+                ResolveResult::Success(()) => {
+                    // count success
+                    module.resolved_import_count
+                          .set(module.resolved_import_count.get() + 1);
+                    continue;
                 }
-                ResolveResult::Indeterminate => break, // Bail out. We'll come around next time.
-                ResolveResult::Success(()) => () // Good. Continue.
             }
+            // This resolution was not successful, keep it for later
+            indeterminate_imports.push(imports.swap_remove(import_index));
 
-            module.resolved_import_count
-                  .set(module.resolved_import_count.get() + 1);
         }
+
+        imports.extend(indeterminate_imports);
+
+        errors
     }
 
     /// Attempts to resolve the given import. The return value indicates
@@ -367,11 +401,10 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         }
 
         // Decrement the count of unresolved globs if necessary. But only if
-        // the resolution result is indeterminate -- otherwise we'll stop
-        // processing imports here. (See the loop in
-        // resolve_imports_for_module).
+        // the resolution result is a success -- other cases will
+        // be handled by the main loop.
 
-        if !resolution_result.indeterminate() {
+        if resolution_result.success() {
             match import_directive.subclass {
                 GlobImport => {
                     assert!(module_.glob_count.get() >= 1);

src/test/compile-fail/import-shadow-6.rs

@@ -12,8 +12,8 @@
 
 #![no_implicit_prelude]
 
-use qux::*;
-use foo::*; //~ERROR a type named `Baz` has already been imported in this module
+use qux::*; //~ERROR a type named `Baz` has already been imported in this module
+use foo::*;
 
 mod foo {
     pub type Baz = isize;

src/test/compile-fail/issue-25396.rs

@@ -11,14 +11,14 @@
 use foo::baz;
 use bar::baz; //~ ERROR a module named `baz` has already been imported
 
-use foo::Quux;
 use bar::Quux; //~ ERROR a trait named `Quux` has already been imported
+use foo::Quux;
 
-use foo::blah;
-use bar::blah; //~ ERROR a type named `blah` has already been imported
+use foo::blah; //~ ERROR a type named `blah` has already been imported
+use bar::blah;
 
-use foo::WOMP;
-use bar::WOMP; //~ ERROR a value named `WOMP` has already been imported
+use foo::WOMP; //~ ERROR a value named `WOMP` has already been imported
+use bar::WOMP;
 
 fn main() {}
 

src/test/run-pass/import-glob-1.rs

@@ -0,0 +1,34 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// This should resolve fine. Prior to fix, the last import
+// was being tried too early, and marked as unrsolved before
+// the glob import had a chance to be resolved.
+
+mod bar {
+    pub use self::middle::*;
+
+    mod middle {
+        pub use self::baz::Baz;
+
+        mod baz {
+            pub enum Baz {
+                Baz1,
+                Baz2
+            }
+        }
+    }
+}
+
+mod foo {
+    use bar::Baz::{Baz1, Baz2};
+}
+
+fn main() {}

src/test/run-pass/issue-18083.rs

@@ -0,0 +1,32 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// These crossed imports should resolve fine, and not block on
+// each other and be reported as unresolved.
+
+mod a {
+    use b::{B};
+    pub use self::inner::A;
+
+    mod inner {
+        pub struct A;
+    }
+}
+
+mod b {
+    use a::{A};
+    pub use self::inner::B;
+
+    mod inner {
+        pub struct B;
+    }
+}
+
+fn main() {}

src/test/run-pass/issue-4865-1.rs

@@ -0,0 +1,41 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// This should resolve fine.
+// Prior to fix, the crossed imports between a and b
+// would block on the glob import, itself never being resolved
+// because these previous imports were not resolved.
+
+pub mod a {
+    use b::fn_b;
+    use c::*;
+
+    pub fn fn_a(){
+    }
+}
+
+pub mod b {
+    use a::fn_a;
+    use c::*;
+
+    pub fn fn_b(){
+    }
+}
+
+pub mod c{
+    pub fn fn_c(){
+    }
+}
+
+use a::fn_a;
+use b::fn_b;
+
+fn main() {
+}