Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve: fix bug in visibility of shadowed use declarations #30294

Merged
merged 2 commits into from
Dec 11, 2015
Merged

resolve: fix bug in visibility of shadowed use declarations #30294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27c4f9e
Fix a bug in which the visibility of a use declaration defining a nam…
jseyfried Dec 1, 2015
ada87fa
Rename (Ns)ImportResolution
jseyfried Dec 11, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
32 changes: 17 additions & 15 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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");
}
}
Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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>,
}
Expand Down Expand Up @@ -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 \
Expand All @@ -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() {
Expand Down Expand Up @@ -1712,21 +1712,23 @@ 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);
}
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() {
Expand Down Expand Up @@ -3651,17 +3653,17 @@ 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,
Some(..) | None => continue,
};
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);
Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}
Expand Down
11 changes: 6 additions & 5 deletions src/librustc_resolve/record_exports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a labelled continue? It looks like you should continue to the next import_resolution, not to the next namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if the import resolution is not public in one namespace it still could be public in the other namespace. For example, consider

mod foo {}
pub fn f() {}

use foo as bar;
pub use f as bar;

Here, there is a single import_resolution corresponding to bar that is private in the type namespace (i.e. !import_resolution[TypeNS].is_public) but public in the value namespace (i.e. import_resolution[ValueNS].is_public). If we skipped to the next import_resolution after seeing that the type namespace is private, we would not export bar in the value namespace.

}

match import_resolution[ns].target {
Some(ref target) => {
debug!("(computing exports) maybe export '{}'", name);
self.add_export_of_namebinding(exports, *name, &target.binding)
}
Expand Down
Loading