Fix 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).

6 files changed, 153 insertions, 147 deletions

diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs
index 766a6d361dd..0fc87c8b74a 100644
--- a/src/librustc_resolve/build_reduced_graph.rs
+++ b/src/librustc_resolve/build_reduced_graph.rs
@@ -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, NsImportResolution};
 use Module;
 use Namespace::{TypeNS, ValueNS};
 use NameBindings;
@@ -827,9 +827,10 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
                         resolution.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 ns_resolution =
+                            NsImportResolution { id: id, is_public: is_public, target: None };
+                        resolution[TypeNS] = ns_resolution.clone();
+                        resolution[ValueNS] = ns_resolution;
                         return;
                     }
                     None => {}
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
index ddada1b513d..3cb0edf88c7 100644
--- a/src/librustc_resolve/lib.rs
+++ b/src/librustc_resolve/lib.rs
@@ -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");
             }
         }
@@ -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();
                 }
diff --git a/src/librustc_resolve/record_exports.rs b/src/librustc_resolve/record_exports.rs
index 3a6a5a031b6..59cf83e91d2 100644
--- a/src/librustc_resolve/record_exports.rs
+++ b/src/librustc_resolve/record_exports.rs
@@ -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)
                     }
diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs
index 460c851e13f..18dfaa096df 100644
--- a/src/librustc_resolve/resolve_imports.rs
+++ b/src/librustc_resolve/resolve_imports.rs
@@ -102,80 +102,61 @@ impl Target {
     }
 }
 
-/// An ImportResolution represents a particular `use` directive.
 #[derive(Debug)]
+/// An ImportResolution 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 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.
+    // 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,
+    pub type_ns: NsImportResolution,
+    pub value_ns: NsImportResolution,
+}
+
+/// Records what we know about an imported name in a namespace (see `ImportResolution`).
+#[derive(Clone,Debug)]
+pub struct NsImportResolution {
+    /// Whether the name in the namespace was imported with a `use` or a `pub use`.
     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.
-    pub outstanding_references: usize,
+    /// Resolution of the name in the namespace
+    pub target: Option<Target>,
 
-    /// 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,
+    /// The source node of the `use` directive
+    pub id: NodeId,
 }
 
-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,
-        }
+impl ::std::ops::Index<Namespace> for ImportResolution {
+    type Output = NsImportResolution;
+    fn index(&self, ns: Namespace) -> &NsImportResolution {
+        match ns { TypeNS => &self.type_ns, ValueNS => &self.value_ns }
     }
+}
 
-    pub fn target_for_namespace(&self, namespace: Namespace) -> Option<Target> {
-        match namespace {
-            TypeNS => self.type_target.clone(),
-            ValueNS => self.value_target.clone(),
-        }
+impl ::std::ops::IndexMut<Namespace> for ImportResolution {
+    fn index_mut(&mut self, ns: Namespace) -> &mut NsImportResolution {
+        match ns { TypeNS => &mut self.type_ns, ValueNS => &mut self.value_ns }
     }
+}
 
-    pub fn id(&self, namespace: Namespace) -> NodeId {
-        match namespace {
-            TypeNS => self.type_id,
-            ValueNS => self.value_id,
+impl ImportResolution {
+    pub fn new(id: NodeId, is_public: bool) -> ImportResolution {
+        let resolution = NsImportResolution { id: id, is_public: is_public, target: None };
+        ImportResolution {
+            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,
         }
     }
 }
@@ -530,11 +511,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
 
                             // 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] = NsImportResolution {
+                            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.
-
-                    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());
-                        }
-                    }
-                    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());
-                        }
+            let mut dest_import_resolution = import_resolutions.entry(*name).or_insert_with(|| {
+                ImportResolution::new(id, is_public)
+            });
+
+            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] = NsImportResolution {
+                            id: id, is_public: is_public, target: Some(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.
@@ -909,10 +864,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] = NsImportResolution {
+                            target: Some(Target::new(containing_module.clone(),
+                                                     name_bindings[namespace].clone(),
+                                                     import_directive.shadowable)),
+                            id: id,
+                            is_public: is_public
+                        };
                     }
                 }
             };
@@ -920,8 +878,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,
@@ -934,12 +890,12 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                                     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 +913,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,
@@ -990,7 +946,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         if module.external_module_children
                  .borrow()
                  .contains_key(&name) {
-            match import_resolution.type_target {
+            match import_resolution.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 +967,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_resolution.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 +983,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
             Some(_) | None => {}
         }
 
-        match import_resolution.type_target {
+        match import_resolution.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() {
diff --git a/src/test/compile-fail/shadowed-use-visibility.rs b/src/test/compile-fail/shadowed-use-visibility.rs
new file mode 100644
index 00000000000..1bf7f393384
--- /dev/null
+++ b/src/test/compile-fail/shadowed-use-visibility.rs
@@ -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() {}
diff --git a/src/test/run-pass/shadowed-use-visibility.rs b/src/test/run-pass/shadowed-use-visibility.rs
new file mode 100644
index 00000000000..d2a32da4fea
--- /dev/null
+++ b/src/test/run-pass/shadowed-use-visibility.rs
@@ -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();
+}