From b69bf1153d6d59ce449d47a15eeccfcc37917113 Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Wed, 5 Aug 2015 21:47:01 +0200 Subject: [PATCH 1/5] Block import resolution only on 'pub' imports. When resolving 'use' statements, only consider pub imports of the target module as blocking. Closes #4865 --- src/librustc_resolve/build_reduced_graph.rs | 8 +++++++ src/librustc_resolve/lib.rs | 8 +++++++ src/librustc_resolve/resolve_imports.rs | 26 ++++++++++++++++----- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 656d6a36614..355578ddccf 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -926,6 +926,11 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { is_public, shadowable)); self.unresolved_imports += 1; + + if is_public { + module_.pub_count.set(module_.pub_count.get() + 1); + } + // Bump the reference count on the name. Or, if this is a glob, set // the appropriate flag. @@ -959,6 +964,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { // module's exports ahead of time. module_.glob_count.set(module_.glob_count.get() + 1); + if is_public { + module_.pub_glob_count.set(module_.pub_glob_count.get() + 1); + } } } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 8c2bb9a8802..1dd26fe7c58 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -699,6 +699,12 @@ pub struct Module { // The number of unresolved globs that this module exports. glob_count: Cell, + // The number of unresolved pub imports (both regular and globs) in this module + pub_count: Cell, + + // 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, @@ -726,6 +732,8 @@ impl Module { anonymous_children: RefCell::new(NodeMap()), import_resolutions: RefCell::new(HashMap::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), } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 412643ba945..18fefa967ea 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -409,11 +409,19 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { GlobImport => { assert!(module_.glob_count.get() >= 1); module_.glob_count.set(module_.glob_count.get() - 1); + if import_directive.is_public { + assert!(module_.pub_glob_count.get() >= 1); + module_.pub_glob_count.set(module_.pub_glob_count.get() - 1); + } } SingleImport(..) => { // Ignore. } } + if import_directive.is_public { + assert!(module_.pub_count.get() >= 1); + module_.pub_count.set(module_.pub_count.get() - 1); + } } return resolution_result; @@ -503,8 +511,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // containing module, bail out. We don't know enough to be // able to resolve this import. - if target_module.glob_count.get() > 0 { - debug!("(resolving single import) unresolved glob; \ + if target_module.pub_glob_count.get() > 0 { + debug!("(resolving single import) unresolved pub glob; \ bailing out"); return ResolveResult::Indeterminate; } @@ -767,16 +775,22 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // We must bail out if the node has unresolved imports of any kind // (including globs). - if !(*target_module).all_imports_resolved() { + if (*target_module).pub_count.get() > 0 { debug!("(resolving glob import) target module has unresolved \ - imports; bailing out"); + pub imports; bailing out"); return ResolveResult::Indeterminate; } - assert_eq!(target_module.glob_count.get(), 0); - // Add all resolved imports from the containing module. let import_resolutions = target_module.import_resolutions.borrow(); + + if module_.import_resolutions.borrow_state() != ::std::cell::BorrowState::Unused { + // In this case, target_module == module_ + // This means we are trying to glob import a module into itself, + // and it is a no-go + return ResolveResult::Indeterminate; + } + for (ident, target_import_resolution) in import_resolutions.iter() { debug!("(resolving glob import) writing module resolution \ {} into `{}`", From 751675938edef0e904086b29dc4d15bdaf519a41 Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Wed, 5 Aug 2015 21:56:49 +0200 Subject: [PATCH 2/5] Update tests and add some more. --- src/test/compile-fail/issue-25396.rs | 10 +++++----- src/test/run-pass/issue-4865-2.rs | 27 +++++++++++++++++++++++++++ src/test/run-pass/issue-4865-3.rs | 22 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 src/test/run-pass/issue-4865-2.rs create mode 100644 src/test/run-pass/issue-4865-3.rs diff --git a/src/test/compile-fail/issue-25396.rs b/src/test/compile-fail/issue-25396.rs index bf26a591b4b..3ada57c9993 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 bar::Quux; //~ ERROR a trait named `Quux` has already been imported use foo::Quux; +use bar::Quux; //~ ERROR a trait named `Quux` has already been imported -use foo::blah; //~ ERROR a type named `blah` has already been imported -use bar::blah; +use foo::blah; +use bar::blah; //~ ERROR a type named `blah` has already been imported -use foo::WOMP; //~ ERROR a value named `WOMP` has already been imported -use bar::WOMP; +use foo::WOMP; +use bar::WOMP; //~ ERROR a value named `WOMP` has already been imported fn main() {} diff --git a/src/test/run-pass/issue-4865-2.rs b/src/test/run-pass/issue-4865-2.rs new file mode 100644 index 00000000000..cd435bf6473 --- /dev/null +++ b/src/test/run-pass/issue-4865-2.rs @@ -0,0 +1,27 @@ +// 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 use hello::*; + +pub mod say { + pub fn hello() { println!("hello"); } +} + +pub mod hello { + use say; + + pub fn hello() { + say::hello(); + } +} + +fn main() { + hello(); +} diff --git a/src/test/run-pass/issue-4865-3.rs b/src/test/run-pass/issue-4865-3.rs new file mode 100644 index 00000000000..0ed3a233587 --- /dev/null +++ b/src/test/run-pass/issue-4865-3.rs @@ -0,0 +1,22 @@ +// 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::*; +} + +pub mod b { + use a::*; +} + +use a::*; + +fn main() { +} From 8e24091f98675390620172ba25fbfbbea87d150b Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Thu, 6 Aug 2015 12:47:10 +0200 Subject: [PATCH 3/5] Factor inc/dec count methods. --- src/librustc_resolve/build_reduced_graph.rs | 6 +++--- src/librustc_resolve/lib.rs | 24 +++++++++++++++++++++ src/librustc_resolve/resolve_imports.rs | 9 +++----- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 355578ddccf..75b99bed431 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -928,7 +928,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { self.unresolved_imports += 1; if is_public { - module_.pub_count.set(module_.pub_count.get() + 1); + module_.inc_pub_count(); } // Bump the reference count on the name. Or, if this is a glob, set @@ -963,9 +963,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { // Set the glob flag. This tells us that we don't know the // module's exports ahead of time. - module_.glob_count.set(module_.glob_count.get() + 1); + module_.inc_glob_count(); if is_public { - module_.pub_glob_count.set(module_.pub_glob_count.get() + 1); + module_.inc_pub_glob_count(); } } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 1dd26fe7c58..50a8c588533 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -749,6 +749,30 @@ impl Module { } } +impl Module { + pub fn inc_glob_count(&self) { + self.glob_count.set(self.glob_count.get() + 1); + } + pub fn dec_glob_count(&self) { + assert!(self.glob_count.get() > 0); + self.glob_count.set(self.glob_count.get() - 1); + } + pub fn inc_pub_count(&self) { + self.pub_count.set(self.pub_count.get() + 1); + } + pub fn dec_pub_count(&self) { + assert!(self.pub_count.get() > 0); + self.pub_count.set(self.pub_count.get() - 1); + } + pub fn inc_pub_glob_count(&self) { + self.pub_glob_count.set(self.pub_glob_count.get() + 1); + } + pub fn dec_pub_glob_count(&self) { + assert!(self.pub_glob_count.get() > 0); + self.pub_glob_count.set(self.pub_glob_count.get() - 1); + } +} + impl fmt::Debug for Module { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{:?}, kind: {:?}, {}", diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 18fefa967ea..bab9f19bbff 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -407,11 +407,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { if resolution_result.success() { match import_directive.subclass { GlobImport => { - assert!(module_.glob_count.get() >= 1); - module_.glob_count.set(module_.glob_count.get() - 1); + module_.dec_glob_count(); if import_directive.is_public { - assert!(module_.pub_glob_count.get() >= 1); - module_.pub_glob_count.set(module_.pub_glob_count.get() - 1); + module_.dec_pub_glob_count(); } } SingleImport(..) => { @@ -419,8 +417,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } } if import_directive.is_public { - assert!(module_.pub_count.get() >= 1); - module_.pub_count.set(module_.pub_count.get() - 1); + module_.dec_pub_count(); } } From 3d041bd46dfbd77a392228b6e4c0cb866d4404fc Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Thu, 6 Aug 2015 12:48:42 +0200 Subject: [PATCH 4/5] Add test descriptions. --- src/test/run-pass/issue-4865-2.rs | 6 ++++++ src/test/run-pass/issue-4865-3.rs | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/test/run-pass/issue-4865-2.rs b/src/test/run-pass/issue-4865-2.rs index cd435bf6473..6de2f437b20 100644 --- a/src/test/run-pass/issue-4865-2.rs +++ b/src/test/run-pass/issue-4865-2.rs @@ -8,6 +8,12 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Previously, this would have failed to resolve due to the circular +// block between `use say` and `pub use hello::*`. +// +// Now, as `use say` is not `pub`, the glob import can resolve +// without any problem and this resolves fine. + pub use hello::*; pub mod say { diff --git a/src/test/run-pass/issue-4865-3.rs b/src/test/run-pass/issue-4865-3.rs index 0ed3a233587..d800ea6a665 100644 --- a/src/test/run-pass/issue-4865-3.rs +++ b/src/test/run-pass/issue-4865-3.rs @@ -8,12 +8,15 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// This should resolve fine even with the circular imports as +// they are not `pub`. + pub mod a { - use b::*; + use b::*; } pub mod b { - use a::*; + use a::*; } use a::*; From 5847ea76195d9cbe9d066418bdf44ba2a0398649 Mon Sep 17 00:00:00 2001 From: Victor Berger Date: Fri, 7 Aug 2015 12:00:46 +0200 Subject: [PATCH 5/5] Customize error messages for self glob imports. --- src/librustc_resolve/resolve_imports.rs | 6 +++++- src/test/compile-fail/issue-8208.rs | 12 +++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index bab9f19bbff..5a377d2c5fe 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -785,7 +785,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // In this case, target_module == module_ // This means we are trying to glob import a module into itself, // and it is a no-go - return ResolveResult::Indeterminate; + debug!("(resolving glob imports) target module is current module; giving up"); + return ResolveResult::Failed(Some(( + import_directive.span, + "Cannot glob-import a module into itself.".into() + ))); } for (ident, target_import_resolution) in import_resolutions.iter() { diff --git a/src/test/compile-fail/issue-8208.rs b/src/test/compile-fail/issue-8208.rs index 7e3f1171e25..318089b3030 100644 --- a/src/test/compile-fail/issue-8208.rs +++ b/src/test/compile-fail/issue-8208.rs @@ -8,7 +8,17 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use self::*; //~ ERROR: unresolved import +use self::*; //~ ERROR: unresolved import `self::*`. Cannot glob-import a module into itself. + +mod foo { + use foo::*; //~ ERROR: unresolved import `foo::*`. Cannot glob-import a module into itself. + + mod bar { + use super::bar::*; + //~^ ERROR: unresolved import `super::bar::*`. Cannot glob-import a module into itself. + } + +} fn main() { }