From 88c2914e56fe54f2179c37914a7d61c7fe4a2605 Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Fri, 31 Jul 2015 10:22:13 +0200 Subject: [PATCH 1/4] Remove unused resolve error message. There is not situation where `foo` would be unresolved but not `foo::*`. --- src/librustc_resolve/lib.rs | 21 +++++---------------- src/librustc_resolve/resolve_imports.rs | 2 +- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index fa9c7a2038c80..f66dfa83ff948 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -176,7 +176,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 +359,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); @@ -1815,19 +1814,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. diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 0a88a3c0aefbb..a9ad0cf137bb9 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -278,7 +278,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some((&*import_path_to_string( &import_directive.module_path, import_directive.subclass), - Some(&*help)))) + &*help))) ); } ResolveResult::Indeterminate => break, // Bail out. We'll come around next time. From 96041ccd1054aa05c4ce7f6741df278fbf96c06b Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Fri, 31 Jul 2015 18:58:59 +0200 Subject: [PATCH 2/4] More perseverant about indeterminate imports. Most errors generated by resolve might be caused by not-yet-resolved glob imports. This changes the behavior of the resolve imports algorithms to not fail prematurally on first error, but instead buffer intermediate errors and report them only when stuck. Fixes #18083 --- src/librustc_resolve/lib.rs | 12 +++- src/librustc_resolve/resolve_imports.rs | 82 ++++++++++++++++--------- 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index f66dfa83ff948..45dbf08071d19 100644 --- a/src/librustc_resolve/lib.rs +++ b/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)] @@ -538,8 +539,8 @@ enum ResolveResult { } impl ResolveResult { - fn indeterminate(&self) -> bool { - match *self { Indeterminate => true, _ => false } + fn success(&self) -> bool { + match *self { Success(_) => true, _ => false } } } @@ -731,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() + } } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index a9ad0cf137bb9..b368e16d6feac 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -208,7 +208,7 @@ 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"); @@ -216,7 +216,20 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } if self.resolver.unresolved_imports == prev_unresolved_imports { - self.resolver.report_unresolved_imports(module_root); + // resolving failed + if errors.len() > 0 { + for (span, path, help) in errors { + resolve_error(self.resolver, + span, + ResolutionError::UnresolvedImport(Some((&*path, &*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 undeterminate 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 +240,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) { + fn resolve_imports_for_module_subtree(&mut self, module_: Rc) + -> Vec<(Span, String, String)> { + 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 +256,65 @@ 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) { + fn resolve_imports_for_module(&mut self, module: Rc) -> Vec<(Span, String, String)> { + 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), - &*help))) - ); + errors.push((span, + import_path_to_string( + &import_directive.module_path, + import_directive.subclass + ), + 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 +394,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); From f9f9f509a09fadf96a85b0c4b68a13b9b8ef6dfb Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Fri, 31 Jul 2015 19:04:34 +0200 Subject: [PATCH 3/4] Fix resolve tests and add some more. The precedent resolve modification changed the order in which imports are handled, so 2 tests needed to be updated. --- src/test/compile-fail/import-shadow-6.rs | 4 +-- src/test/compile-fail/issue-25396.rs | 10 +++---- src/test/run-pass/import-glob-1.rs | 32 +++++++++++++++++++++ src/test/run-pass/issue-18083.rs | 29 +++++++++++++++++++ src/test/run-pass/issue-4865-1.rs | 36 ++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 src/test/run-pass/import-glob-1.rs create mode 100644 src/test/run-pass/issue-18083.rs create mode 100644 src/test/run-pass/issue-4865-1.rs diff --git a/src/test/compile-fail/import-shadow-6.rs b/src/test/compile-fail/import-shadow-6.rs index fa3b75c70f0b6..0f3d54d5fe3d8 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::*; -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; diff --git a/src/test/compile-fail/issue-25396.rs b/src/test/compile-fail/issue-25396.rs index 3ada57c999305..bf26a591b4b5d 100644 --- a/src/test/compile-fail/issue-25396.rs +++ b/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() {} diff --git a/src/test/run-pass/import-glob-1.rs b/src/test/run-pass/import-glob-1.rs new file mode 100644 index 0000000000000..9dc570fbe5186 --- /dev/null +++ b/src/test/run-pass/import-glob-1.rs @@ -0,0 +1,32 @@ +// Copyright 2012-2014 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![allow(unused_imports, dead_code)] + +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() {} diff --git a/src/test/run-pass/issue-18083.rs b/src/test/run-pass/issue-18083.rs new file mode 100644 index 0000000000000..8c3dc67c313b1 --- /dev/null +++ b/src/test/run-pass/issue-18083.rs @@ -0,0 +1,29 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +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() {} diff --git a/src/test/run-pass/issue-4865-1.rs b/src/test/run-pass/issue-4865-1.rs new file mode 100644 index 0000000000000..a025273073d19 --- /dev/null +++ b/src/test/run-pass/issue-4865-1.rs @@ -0,0 +1,36 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +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() { +} From 58e35d7c2ab93637c6c549b03a04f900fb3499d2 Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Tue, 4 Aug 2015 08:14:32 +0200 Subject: [PATCH 4/4] Addressing nits & tests explanations. --- src/librustc_resolve/resolve_imports.rs | 37 +++++++++++++++---------- src/test/run-pass/import-glob-1.rs | 6 ++-- src/test/run-pass/issue-18083.rs | 3 ++ src/test/run-pass/issue-4865-1.rs | 5 ++++ 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index b368e16d6feac..412643ba94546 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/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> @@ -218,16 +223,16 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { if self.resolver.unresolved_imports == prev_unresolved_imports { // resolving failed if errors.len() > 0 { - for (span, path, help) in errors { + for e in errors { resolve_error(self.resolver, - span, - ResolutionError::UnresolvedImport(Some((&*path, &*help)))); + 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 undeterminate at this point are actually blocked - // by errored imports, so there is no point reporting them + // 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; @@ -241,7 +246,7 @@ 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) - -> Vec<(Span, String, String)> { + -> Vec { let mut errors = Vec::new(); debug!("(resolving imports for module subtree) resolving {}", module_to_string(&*module_)); @@ -269,7 +274,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } /// Attempts to resolve imports for the given module only. - fn resolve_imports_for_module(&mut self, module: Rc) -> Vec<(Span, String, String)> { + fn resolve_imports_for_module(&mut self, module: Rc) -> Vec { let mut errors = Vec::new(); if module.all_imports_resolved() { @@ -292,12 +297,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some((span, msg)) => (span, format!(". {}", msg)), None => (import_directive.span, String::new()) }; - errors.push((span, - import_path_to_string( - &import_directive.module_path, - import_directive.subclass - ), - help)) + errors.push(ImportResolvingError { + span: span, + path: import_path_to_string( + &import_directive.module_path, + import_directive.subclass + ), + help: help + }); } ResolveResult::Indeterminate => {} ResolveResult::Success(()) => { diff --git a/src/test/run-pass/import-glob-1.rs b/src/test/run-pass/import-glob-1.rs index 9dc570fbe5186..a7949e7d6e3b1 100644 --- a/src/test/run-pass/import-glob-1.rs +++ b/src/test/run-pass/import-glob-1.rs @@ -1,4 +1,4 @@ -// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT +// 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. // @@ -8,7 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -#![allow(unused_imports, dead_code)] +// 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::*; diff --git a/src/test/run-pass/issue-18083.rs b/src/test/run-pass/issue-18083.rs index 8c3dc67c313b1..ff26e186db393 100644 --- a/src/test/run-pass/issue-18083.rs +++ b/src/test/run-pass/issue-18083.rs @@ -8,6 +8,9 @@ // 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; diff --git a/src/test/run-pass/issue-4865-1.rs b/src/test/run-pass/issue-4865-1.rs index a025273073d19..3c4777951302e 100644 --- a/src/test/run-pass/issue-4865-1.rs +++ b/src/test/run-pass/issue-4865-1.rs @@ -8,6 +8,11 @@ // 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::*;