OpenE2K
/
rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Auto merge of #30294 - jseyfried:fix_shadowed_use_visibility, r=nrc 

This fixes a bug in which the visibility of a use declaration defining a name in one namespace (e.g. the value namespace) is overridden by a later use declaration defining the same name in the other namespace (e.g. the type namespace). For example,
```rust
fn f() {}
pub mod bar {}

mod foo {
    use f; // This import should not be visible outside `foo`,
    pub use bar as f; // but it visible outside of `foo` because of this import.
}

fn main() { foo::f(); }
```
As the example demonstrates, this is a [breaking-change], but it looks unlikely to cause breakage in practice, and any breakage can be fixed by correcting visibility modifiers.
This commit is contained in:
bors 2015-12-11 04:27:53 +00:00
parent ae5d09551e ada87fae5f
commit 672a3d93e3
6 changed files with 172 additions and 167 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/librustc_resolve/build_reduced_graph.rs
View File

@ -16,7 +16,7 @@
use DefModifiers;
use resolve_imports::ImportDirective;
use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport};
use resolve_imports::ImportResolution;
use resolve_imports::{ImportResolution, ImportResolutionPerNamespace};
use Module;
use Namespace::{TypeNS, ValueNS};
use NameBindings;
@ -822,22 +822,23 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
let mut import_resolutions = module_.import_resolutions.borrow_mut();
match import_resolutions.get_mut(&target) {
Some(resolution) => {
Some(resolution_per_ns) => {
debug!("(building import directive) bumping reference");
resolution.outstanding_references += 1;
resolution_per_ns.outstanding_references += 1;
// the source of this name is different now
resolution.type_id = id;
resolution.value_id = id;
resolution.is_public = is_public;
let resolution =
ImportResolution { id: id, is_public: is_public, target: None };
resolution_per_ns[TypeNS] = resolution.clone();
resolution_per_ns[ValueNS] = resolution;
return;
}
None => {}
}
debug!("(building import directive) creating new");
let mut resolution = ImportResolution::new(id, is_public);
resolution.outstanding_references = 1;
import_resolutions.insert(target, resolution);
let mut import_resolution_per_ns = ImportResolutionPerNamespace::new(id, is_public);
import_resolution_per_ns.outstanding_references = 1;
import_resolutions.insert(target, import_resolution_per_ns);
}
GlobImport => {
// Set the glob flag. This tells us that we don't know the

32
src/librustc_resolve/lib.rs
View File

@ -96,7 +96,7 @@ use std::mem::replace;
use std::rc::{Rc, Weak};
use std::usize;
use resolve_imports::{Target, ImportDirective, ImportResolution};
use resolve_imports::{Target, ImportDirective, ImportResolutionPerNamespace};
use resolve_imports::Shadowable;
// NB: This module needs to be declared first so diagnostics are
@ -328,7 +328,7 @@ fn resolve_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>,
.import_resolutions
.borrow()
.get(&name) {
let item = resolver.ast_map.expect_item(directive.value_id);
let item = resolver.ast_map.expect_item(directive.value_ns.id);
resolver.session.span_note(item.span, "constant imported here");
}
}
@ -793,7 +793,7 @@ pub struct Module {
anonymous_children: RefCell<NodeMap<Rc<Module>>>,
// The status of resolving each import in this module.
import_resolutions: RefCell<HashMap<Name, ImportResolution>>,
import_resolutions: RefCell<HashMap<Name, ImportResolutionPerNamespace>>,
// The number of unresolved globs that this module exports.
glob_count: Cell<usize>,
@ -912,7 +912,7 @@ bitflags! {
// Records a possibly-private value, type, or module definition.
#[derive(Debug)]
struct NsDef {
modifiers: DefModifiers, // see note in ImportResolution about how to use this
modifiers: DefModifiers, // see note in ImportResolutionPerNamespace about how to use this
def_or_module: DefOrModule,
span: Option<Span>,
}
@ -1491,7 +1491,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
// adjacent import statements are processed as though they mutated the
// current scope.
if let Some(import_resolution) = module_.import_resolutions.borrow().get(&name) {
match (*import_resolution).target_for_namespace(namespace) {
match import_resolution[namespace].target.clone() {
None => {
// Not found; continue.
debug!("(resolving item in lexical scope) found import resolution, but not \
@ -1501,7 +1501,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
Some(target) => {
debug!("(resolving item in lexical scope) using import resolution");
// track used imports and extern crates as well
let id = import_resolution.id(namespace);
let id = import_resolution[namespace].id;
self.used_imports.insert((id, namespace));
self.record_import_use(id, name);
if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() {
@ -1712,13 +1712,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
// Check the list of resolved imports.
match module_.import_resolutions.borrow().get(&name) {
Some(import_resolution) if allow_private_imports || import_resolution.is_public => {
Some(import_resolution) if allow_private_imports ||
import_resolution[namespace].is_public => {
if import_resolution.is_public && import_resolution.outstanding_references != 0 {
if import_resolution[namespace].is_public &&
import_resolution.outstanding_references != 0 {
debug!("(resolving name in module) import unresolved; bailing out");
return Indeterminate;
}
match import_resolution.target_for_namespace(namespace) {
match import_resolution[namespace].target.clone() {
None => {
debug!("(resolving name in module) name found, but not in namespace {:?}",
namespace);
@ -1726,7 +1728,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
Some(target) => {
debug!("(resolving name in module) resolved to import");
// track used imports and extern crates as well
let id = import_resolution.id(namespace);
let id = import_resolution[namespace].id;
self.used_imports.insert((id, namespace));
self.record_import_use(id, name);
if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() {
@ -3651,9 +3653,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
// Look for imports.
for (_, import) in search_module.import_resolutions.borrow().iter() {
let target = match import.target_for_namespace(TypeNS) {
let target = match import.type_ns.target {
None => continue,
Some(target) => target,
Some(ref target) => target,
};
let did = match target.binding.def() {
Some(DefTrait(trait_def_id)) => trait_def_id,
@ -3661,7 +3663,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
};
if self.trait_item_map.contains_key(&(name, did)) {
add_trait_info(&mut found_traits, did, name);
let id = import.type_id;
let id = import.type_ns.id;
self.used_imports.insert((id, TypeNS));
let trait_name = self.get_trait_name(did);
self.record_import_use(id, trait_name);
@ -3734,7 +3736,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
let import_resolutions = module_.import_resolutions.borrow();
for (&name, import_resolution) in import_resolutions.iter() {
let value_repr;
match import_resolution.target_for_namespace(ValueNS) {
match import_resolution.value_ns.target {
None => {
value_repr = "".to_string();
}
@ -3745,7 +3747,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
let type_repr;
match import_resolution.target_for_namespace(TypeNS) {
match import_resolution.type_ns.target {
None => {
type_repr = "".to_string();
}

11
src/librustc_resolve/record_exports.rs
View File

@ -130,13 +130,14 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> {
fn add_exports_for_module(&mut self, exports: &mut Vec<Export>, module_: &Module) {
for (name, import_resolution) in module_.import_resolutions.borrow().iter() {
if !import_resolution.is_public {
continue;
}
let xs = [TypeNS, ValueNS];
for &ns in &xs {
match import_resolution.target_for_namespace(ns) {
Some(target) => {
if !import_resolution[ns].is_public {
continue;
}
match import_resolution[ns].target {
Some(ref target) => {
debug!("(computing exports) maybe export '{}'", name);
self.add_export_of_namebinding(exports, *name, &target.binding)
}

231
src/librustc_resolve/resolve_imports.rs
View File

@ -58,7 +58,7 @@ pub struct ImportDirective {
pub subclass: ImportDirectiveSubclass,
pub span: Span,
pub id: NodeId,
pub is_public: bool, // see note in ImportResolution about how to use this
pub is_public: bool, // see note in ImportResolutionPerNamespace about how to use this
pub shadowable: Shadowable,
}
@ -102,80 +102,61 @@ impl Target {
}
}
/// An ImportResolution represents a particular `use` directive.
#[derive(Debug)]
pub struct ImportResolution {
/// Whether this resolution came from a `use` or a `pub use`. Note that this
/// should *not* be used whenever resolution is being performed. Privacy
/// testing occurs during a later phase of compilation.
pub is_public: bool,
// The number of outstanding references to this name. When this reaches
// zero, outside modules can count on the targets being correct. Before
// then, all bets are off; future imports could override this name.
// Note that this is usually either 0 or 1 - shadowing is forbidden the only
// way outstanding_references is > 1 in a legal program is if the name is
// used in both namespaces.
/// An ImportResolutionPerNamespace records what we know about an imported name.
/// More specifically, it records the number of unresolved `use` directives that import the name,
/// and for each namespace, it records the `use` directive importing the name in the namespace
/// and the `Target` to which the name in the namespace resolves (if applicable).
/// Different `use` directives may import the same name in different namespaces.
pub struct ImportResolutionPerNamespace {
// When outstanding_references reaches zero, outside modules can count on the targets being
// correct. Before then, all bets are off; future `use` directives could override the name.
// Since shadowing is forbidden, the only way outstanding_references > 1 in a legal program
// is if the name is imported by exactly two `use` directives, one of which resolves to a
// value and the other of which resolves to a type.
pub outstanding_references: usize,
/// The value that this `use` directive names, if there is one.
pub value_target: Option<Target>,
/// The source node of the `use` directive leading to the value target
/// being non-none
pub value_id: NodeId,
/// The type that this `use` directive names, if there is one.
pub type_target: Option<Target>,
/// The source node of the `use` directive leading to the type target
/// being non-none
pub type_id: NodeId,
pub type_ns: ImportResolution,
pub value_ns: ImportResolution,
}
impl ImportResolution {
pub fn new(id: NodeId, is_public: bool) -> ImportResolution {
ImportResolution {
type_id: id,
value_id: id,
outstanding_references: 0,
value_target: None,
type_target: None,
is_public: is_public,
}
}
/// Records what we know about an imported name in a namespace (see `ImportResolutionPerNamespace`).
#[derive(Clone,Debug)]
pub struct ImportResolution {
/// Whether the name in the namespace was imported with a `use` or a `pub use`.
pub is_public: bool,
pub fn target_for_namespace(&self, namespace: Namespace) -> Option<Target> {
match namespace {
TypeNS => self.type_target.clone(),
ValueNS => self.value_target.clone(),
}
}
/// Resolution of the name in the namespace
pub target: Option<Target>,
pub fn id(&self, namespace: Namespace) -> NodeId {
match namespace {
TypeNS => self.type_id,
ValueNS => self.value_id,
/// The source node of the `use` directive
pub id: NodeId,
}
impl ::std::ops::Index<Namespace> for ImportResolutionPerNamespace {
type Output = ImportResolution;
fn index(&self, ns: Namespace) -> &ImportResolution {
match ns { TypeNS => &self.type_ns, ValueNS => &self.value_ns }
}
}
impl ::std::ops::IndexMut<Namespace> for ImportResolutionPerNamespace {
fn index_mut(&mut self, ns: Namespace) -> &mut ImportResolution {
match ns { TypeNS => &mut self.type_ns, ValueNS => &mut self.value_ns }
}
}
impl ImportResolutionPerNamespace {
pub fn new(id: NodeId, is_public: bool) -> Self {
let resolution = ImportResolution { id: id, is_public: is_public, target: None };
ImportResolutionPerNamespace {
outstanding_references: 0, type_ns: resolution.clone(), value_ns: resolution,
}
}
pub fn shadowable(&self, namespace: Namespace) -> Shadowable {
let target = self.target_for_namespace(namespace);
if target.is_none() {
return Shadowable::Always;
}
target.unwrap().shadowable
}
pub fn set_target_and_id(&mut self, namespace: Namespace, target: Option<Target>, id: NodeId) {
match namespace {
TypeNS => {
self.type_target = target;
self.type_id = id;
}
ValueNS => {
self.value_target = target;
self.value_id = id;
}
match self[namespace].target {
Some(ref target) => target.shadowable,
None => Shadowable::Always,
}
}
}
@ -523,18 +504,18 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
Some(import_resolution) if import_resolution.outstanding_references == 0 => {
fn get_binding(this: &mut Resolver,
import_resolution: &ImportResolution,
import_resolution: &ImportResolutionPerNamespace,
namespace: Namespace,
source: Name)
-> NamespaceResult {
// Import resolutions must be declared with "pub"
// in order to be exported.
if !import_resolution.is_public {
if !import_resolution[namespace].is_public {
return UnboundResult;
}
match import_resolution.target_for_namespace(namespace) {
match import_resolution[namespace].target.clone() {
None => {
return UnboundResult;
}
@ -545,7 +526,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
}) => {
debug!("(resolving single import) found import in ns {:?}",
namespace);
let id = import_resolution.id(namespace);
let id = import_resolution[namespace].id;
// track used imports and extern crates as well
this.used_imports.insert((id, namespace));
this.record_import_use(id, source);
@ -567,14 +548,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
import_resolution,
ValueNS,
source);
value_used_reexport = import_resolution.is_public;
value_used_reexport = import_resolution.value_ns.is_public;
}
if type_result.is_unknown() {
type_result = get_binding(self.resolver,
import_resolution,
TypeNS,
source);
type_used_reexport = import_resolution.is_public;
type_used_reexport = import_resolution.type_ns.is_public;
}
}
@ -663,11 +644,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
directive.span,
target);
let target = Some(Target::new(target_module.clone(),
name_binding.clone(),
directive.shadowable));
import_resolution.set_target_and_id(namespace, target, directive.id);
import_resolution.is_public = directive.is_public;
import_resolution[namespace] = ImportResolution {
target: Some(Target::new(target_module.clone(),
name_binding.clone(),
directive.shadowable)),
id: directive.id,
is_public: directive.is_public
};
*used_public = name_binding.is_public();
}
UnboundResult => {
@ -702,7 +685,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
// Record what this import resolves to for later uses in documentation,
// this may resolve to either a value or a type, but for documentation
// purposes it's good enough to just favor one over the other.
let value_def_and_priv = import_resolution.value_target.as_ref().map(|target| {
let value_def_and_priv = import_resolution.value_ns.target.as_ref().map(|target| {
let def = target.binding.def().unwrap();
(def,
if value_used_public {
@ -711,7 +694,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
DependsOn(def.def_id())
})
});
let type_def_and_priv = import_resolution.type_target.as_ref().map(|target| {
let type_def_and_priv = import_resolution.type_ns.target.as_ref().map(|target| {
let def = target.binding.def().unwrap();
(def,
if type_used_public {
@ -791,54 +774,26 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
*name,
module_to_string(module_));
if !target_import_resolution.is_public {
debug!("(resolving glob import) nevermind, just kidding");
continue;
}
// Here we merge two import resolutions.
let mut import_resolutions = module_.import_resolutions.borrow_mut();
match import_resolutions.get_mut(name) {
Some(dest_import_resolution) => {
// Merge the two import resolutions at a finer-grained
// level.
let mut dest_import_resolution = import_resolutions.entry(*name).or_insert_with(|| {
ImportResolutionPerNamespace::new(id, is_public)
});
match target_import_resolution.value_target {
None => {
// Continue.
}
Some(ref value_target) => {
self.check_for_conflicting_import(&dest_import_resolution,
import_directive.span,
*name,
ValueNS);
dest_import_resolution.value_target = Some(value_target.clone());
}
for &ns in [TypeNS, ValueNS].iter() {
match target_import_resolution[ns].target {
Some(ref target) if target_import_resolution[ns].is_public => {
self.check_for_conflicting_import(&dest_import_resolution,
import_directive.span,
*name,
ns);
dest_import_resolution[ns] = ImportResolution {
id: id, is_public: is_public, target: Some(target.clone())
};
}
match target_import_resolution.type_target {
None => {
// Continue.
}
Some(ref type_target) => {
self.check_for_conflicting_import(&dest_import_resolution,
import_directive.span,
*name,
TypeNS);
dest_import_resolution.type_target = Some(type_target.clone());
}
}
dest_import_resolution.is_public = is_public;
continue;
_ => {}
}
None => {}
}
// Simple: just copy the old import resolution.
let mut new_import_resolution = ImportResolution::new(id, is_public);
new_import_resolution.value_target = target_import_resolution.value_target.clone();
new_import_resolution.type_target = target_import_resolution.type_target.clone();
import_resolutions.insert(*name, new_import_resolution);
}
// Add all children from the containing module.
@ -877,10 +832,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
let is_public = import_directive.is_public;
let mut import_resolutions = module_.import_resolutions.borrow_mut();
let dest_import_resolution = import_resolutions.entry(name)
.or_insert_with(|| {
ImportResolution::new(id, is_public)
});
let dest_import_resolution = import_resolutions.entry(name).or_insert_with(|| {
ImportResolutionPerNamespace::new(id, is_public)
});
debug!("(resolving glob import) writing resolution `{}` in `{}` to `{}`",
name,
@ -909,10 +863,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
"{}",
msg);
} else {
let target = Target::new(containing_module.clone(),
name_bindings[namespace].clone(),
import_directive.shadowable);
dest_import_resolution.set_target_and_id(namespace, Some(target), id);
dest_import_resolution[namespace] = ImportResolution {
target: Some(Target::new(containing_module.clone(),
name_bindings[namespace].clone(),
import_directive.shadowable)),
id: id,
is_public: is_public
};
}
}
};
@ -920,8 +877,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
merge_child_item(TypeNS);
}
dest_import_resolution.is_public = is_public;
self.check_for_conflicts_between_imports_and_items(module_,
dest_import_resolution,
import_directive.span,
@ -930,16 +885,16 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
/// Checks that imported names and items don't have the same name.
fn check_for_conflicting_import(&mut self,
import_resolution: &ImportResolution,
import_resolution: &ImportResolutionPerNamespace,
import_span: Span,
name: Name,
namespace: Namespace) {
let target = import_resolution.target_for_namespace(namespace);
let target = &import_resolution[namespace].target;
debug!("check_for_conflicting_import: {}; target exists: {}",
name,
target.is_some());
match target {
match *target {
Some(ref target) if target.shadowable != Shadowable::Always => {
let ns_word = match namespace {
TypeNS => {
@ -957,7 +912,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
"a {} named `{}` has already been imported in this module",
ns_word,
name);
let use_id = import_resolution.id(namespace);
let use_id = import_resolution[namespace].id;
let item = self.resolver.ast_map.expect_item(use_id);
// item is syntax::ast::Item;
span_note!(self.resolver.session,
@ -983,14 +938,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
/// Checks that imported names and items don't have the same name.
fn check_for_conflicts_between_imports_and_items(&mut self,
module: &Module,
import_resolution: &ImportResolution,
import: &ImportResolutionPerNamespace,
import_span: Span,
name: Name) {
// First, check for conflicts between imports and `extern crate`s.
if module.external_module_children
.borrow()
.contains_key(&name) {
match import_resolution.type_target {
match import.type_ns.target {
Some(ref target) if target.shadowable != Shadowable::Always => {
let msg = format!("import `{0}` conflicts with imported crate in this module \
(maybe you meant `use {0}::*`?)",
@ -1011,7 +966,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
Some(ref name_bindings) => (*name_bindings).clone(),
};
match import_resolution.value_target {
match import.value_ns.target {
Some(ref target) if target.shadowable != Shadowable::Always => {
if let Some(ref value) = *name_bindings.value_ns.borrow() {
span_err!(self.resolver.session,
@ -1027,7 +982,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
Some(_) | None => {}
}
match import_resolution.type_target {
match import.type_ns.target {
Some(ref target) if target.shadowable != Shadowable::Always => {
if let Some(ref ty) = *name_bindings.type_ns.borrow() {
let (what, note) = match ty.module() {

26
src/test/compile-fail/shadowed-use-visibility.rs Normal file
View File

@ -0,0 +1,26 @@
// 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.
mod foo {
pub fn f() {}
use foo as bar;
pub use self::f as bar;
}
mod bar {
use foo::bar::f as g; //~ ERROR unresolved import
use foo as f;
pub use foo::*;
}
use bar::f::f; //~ ERROR unresolved import
fn main() {}

20
src/test/run-pass/shadowed-use-visibility.rs Normal file
View File

@ -0,0 +1,20 @@
// 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.
mod foo {
pub fn f() {}
pub use self::f as bar;
use foo as bar;
}
fn main() {
foo::bar();
}