From 61f1f4d21ec99b12f334490871e244f4f4a4988f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 7 Mar 2016 09:59:08 +0000 Subject: [PATCH 1/4] Add a field `pub_outstanding_references` to `NameResolution`. Add an argument `allow_private_imports` to some methods. --- src/librustc_resolve/build_reduced_graph.rs | 4 +- src/librustc_resolve/resolve_imports.rs | 53 ++++++++++++++------- 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index b3d7be4775e42..f391597e3b159 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -681,8 +681,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { match subclass { SingleImport { target, .. } => { - module_.increment_outstanding_references_for(target, ValueNS); - module_.increment_outstanding_references_for(target, TypeNS); + module_.increment_outstanding_references_for(target, ValueNS, is_public); + module_.increment_outstanding_references_for(target, TypeNS, is_public); } GlobImport => { // Set the glob flag. This tells us that we don't know the diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index f1f47381e4c81..a3076edef0f12 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -125,7 +125,8 @@ impl ImportDirective { /// Records information about the resolution of a name in a module. pub struct NameResolution<'a> { /// The number of unresolved single imports that could define the name. - outstanding_references: usize, + outstanding_references: u32, + pub_outstanding_references: u32, /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>, duplicate_globs: Vec<&'a NameBinding<'a>>, @@ -151,7 +152,7 @@ impl<'a> NameResolution<'a> { } // Returns the resolution of the name assuming no more globs will define it. - fn result(&self) -> ResolveResult<&'a NameBinding<'a>> { + fn result(&self, allow_private_imports: bool) -> ResolveResult<&'a NameBinding<'a>> { match self.binding { Some(binding) if !binding.defined_with(DefModifiers::GLOB_IMPORTED) => Success(binding), _ if self.outstanding_references > 0 => Indeterminate, @@ -162,8 +163,9 @@ impl<'a> NameResolution<'a> { // Returns Some(the resolution of the name), or None if the resolution depends // on whether more globs can define the name. - fn try_result(&self) -> Option>> { - match self.result() { + fn try_result(&self, allow_private_imports: bool) + -> Option>> { + match self.result(allow_private_imports) { Success(binding) if binding.defined_with(DefModifiers::PRELUDE) => None, Failed(_) => None, result @ _ => Some(result), @@ -200,7 +202,7 @@ impl<'a> ::ModuleS<'a> { }; let resolution = resolutions.get(&(name, ns)).cloned().unwrap_or_default(); - if let Some(result) = resolution.try_result() { + if let Some(result) = resolution.try_result(allow_private_imports) { // If the resolution doesn't depend on glob definability, check privacy and return. return result.and_then(|binding| { let allowed = allow_private_imports || !binding.is_import() || binding.is_public(); @@ -234,7 +236,7 @@ impl<'a> ::ModuleS<'a> { } } - resolution.result() + resolution.result(true) } // Define the name or return the existing binding if there is a collision. @@ -246,15 +248,26 @@ impl<'a> ::ModuleS<'a> { }) } - pub fn increment_outstanding_references_for(&self, name: Name, ns: Namespace) { + pub fn increment_outstanding_references_for(&self, name: Name, ns: Namespace, is_public: bool) { let mut resolutions = self.resolutions.borrow_mut(); - resolutions.entry((name, ns)).or_insert_with(Default::default).outstanding_references += 1; + let resolution = resolutions.entry((name, ns)).or_insert_with(Default::default); + resolution.outstanding_references += 1; + if is_public { + resolution.pub_outstanding_references += 1; + } } - fn decrement_outstanding_references_for(&self, name: Name, ns: Namespace) { - self.update_resolution(name, ns, |resolution| match resolution.outstanding_references { - 0 => panic!("No more outstanding references!"), - ref mut outstanding_references => *outstanding_references -= 1, + fn decrement_outstanding_references_for(&self, name: Name, ns: Namespace, is_public: bool) { + let decrement_references = |count: &mut _| { + assert!(*count > 0); + *count -= 1; + }; + + self.update_resolution(name, ns, |resolution| { + decrement_references(&mut resolution.outstanding_references); + if is_public { + decrement_references(&mut resolution.pub_outstanding_references); + } }) } @@ -265,11 +278,11 @@ impl<'a> ::ModuleS<'a> { { let mut resolutions = self.resolutions.borrow_mut(); let resolution = resolutions.entry((name, ns)).or_insert_with(Default::default); - let was_success = resolution.try_result().and_then(ResolveResult::success).is_some(); + let was_success = resolution.try_result(false).and_then(ResolveResult::success).is_some(); let t = update(resolution); if !was_success { - if let Some(Success(binding)) = resolution.try_result() { + if let Some(Success(binding)) = resolution.try_result(false) { self.define_in_glob_importers(name, ns, binding); } } @@ -460,10 +473,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let mut resolve_in_ns = |ns, determined: bool| { // Temporarily count the directive as determined so that the resolution fails // (as opposed to being indeterminate) when it can only be defined by the directive. - if !determined { module_.decrement_outstanding_references_for(target, ns) } + if !determined { + module_.decrement_outstanding_references_for(target, ns, directive.is_public) + } let result = self.resolver.resolve_name_in_module(target_module, source, ns, false, true); - if !determined { module_.increment_outstanding_references_for(target, ns) } + if !determined { + module_.increment_outstanding_references_for(target, ns, directive.is_public) + } result }; (resolve_in_ns(ValueNS, value_determined.get()), @@ -494,7 +511,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { self.report_conflict(target, ns, &directive.import(binding, None), old_binding); } } - module_.decrement_outstanding_references_for(target, ns); + module_.decrement_outstanding_references_for(target, ns, directive.is_public); } match (&value_result, &type_result) { @@ -601,7 +618,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } for (&(name, ns), resolution) in target_module.resolutions.borrow().iter() { - if let Some(Success(binding)) = resolution.try_result() { + if let Some(Success(binding)) = resolution.try_result(false) { if binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) { let _ = module_.try_define_child(name, ns, directive.import(binding, None)); } From a0efd7ebdbeefc16e58674afc83189ec0a550c2c Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 7 Mar 2016 10:00:22 +0000 Subject: [PATCH 2/4] Deduce that a name resolution fails (as opposed to being indeterminte) in more cases. --- src/librustc_resolve/resolve_imports.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index a3076edef0f12..b5e5114d895ea 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -155,6 +155,9 @@ impl<'a> NameResolution<'a> { fn result(&self, allow_private_imports: bool) -> ResolveResult<&'a NameBinding<'a>> { match self.binding { Some(binding) if !binding.defined_with(DefModifiers::GLOB_IMPORTED) => Success(binding), + // If we don't allow private imports and no public imports can define the name, fail. + _ if !allow_private_imports && self.pub_outstanding_references == 0 && + !self.binding.map(NameBinding::is_public).unwrap_or(false) => Failed(None), _ if self.outstanding_references > 0 => Indeterminate, Some(binding) => Success(binding), None => Failed(None), From 162fa8642b140bfe8aeee21ceb546b7e182c20af Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 7 Mar 2016 10:03:53 +0000 Subject: [PATCH 3/4] Add regression test --- src/test/compile-fail/issue-32089.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 src/test/compile-fail/issue-32089.rs diff --git a/src/test/compile-fail/issue-32089.rs b/src/test/compile-fail/issue-32089.rs new file mode 100644 index 0000000000000..5da7b9fff6e9a --- /dev/null +++ b/src/test/compile-fail/issue-32089.rs @@ -0,0 +1,23 @@ +// Copyright 2016 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. + +#![feature(rustc_attrs)] +#![allow(unused_imports)] + +pub type Type = i32; + +mod one { use super::Type; } +pub use self::one::*; + +mod two { use super::Type; } +pub use self::two::*; + +#[rustc_error] +fn main() {} //~ ERROR compilation successful From 4dc4cae79a07017695889b672d335e7c63c95416 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 7 Mar 2016 22:43:56 +0000 Subject: [PATCH 4/4] Add a comment --- src/librustc_resolve/resolve_imports.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index b5e5114d895ea..97124b7f46a73 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -124,8 +124,9 @@ impl ImportDirective { #[derive(Clone, Default)] /// Records information about the resolution of a name in a module. pub struct NameResolution<'a> { - /// The number of unresolved single imports that could define the name. + /// The number of unresolved single imports of any visibility that could define the name. outstanding_references: u32, + /// The number of unresolved `pub` single imports that could define the name. pub_outstanding_references: u32, /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>,