Fix private module loophole in the 'private type in public item' check

13 files changed, 76 insertions, 65 deletions

diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs
index 1b09be05020..e368a669133 100644
--- a/src/librustc/session/config.rs
+++ b/src/librustc/session/config.rs
@@ -736,8 +736,8 @@ mod opt {
     use getopts;
     use super::RustcOptGroup;
 
-    type R = RustcOptGroup;
-    type S<'a> = &'a str;
+    pub type R = RustcOptGroup;
+    pub type S<'a> = &'a str;
 
     fn stable(g: getopts::OptGroup) -> R { RustcOptGroup::stable(g) }
     fn unstable(g: getopts::OptGroup) -> R { RustcOptGroup::unstable(g) }
diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs
index 27f807ebe42..bfce2f0062d 100644
--- a/src/librustc_privacy/lib.rs
+++ b/src/librustc_privacy/lib.rs
@@ -1186,6 +1186,7 @@ impl<'a, 'tcx> VisiblePrivateTypesVisitor<'a, 'tcx> {
         if !is_local(did) {
             return false
         }
+
         // .. and it corresponds to a private type in the AST (this returns
         // None for type parameters)
         match self.tcx.map.find(did.node) {
@@ -1206,12 +1207,15 @@ impl<'a, 'tcx> VisiblePrivateTypesVisitor<'a, 'tcx> {
             if !self.tcx.sess.features.borrow().visible_private_types &&
                 self.path_is_private_type(trait_ref.trait_ref.ref_id) {
                     let span = trait_ref.trait_ref.path.span;
-                    self.tcx.sess.span_err(span,
-                                           "private trait in exported type \
-                                            parameter bound");
+                    self.tcx.sess.span_err(span, "private trait in exported type \
+                                                  parameter bound");
             }
         }
     }
+
+    fn item_is_public(&self, id: &ast::NodeId, vis: ast::Visibility) -> bool {
+        self.exported_items.contains(id) || vis == ast::Public
+    }
 }
 
 impl<'a, 'b, 'tcx, 'v> Visitor<'v> for CheckTypeForPrivatenessVisitor<'a, 'b, 'tcx> {
@@ -1259,7 +1263,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
             // error messages without (too many) false positives
             // (i.e. we could just return here to not check them at
             // all, or some worse estimation of whether an impl is
-            // publicly visible.
+            // publicly visible).
             ast::ItemImpl(_, _, ref g, ref trait_ref, ref self_, ref impl_items) => {
                 // `impl [... for] Private` is never visible.
                 let self_contains_private;
@@ -1321,7 +1325,22 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
                     match *trait_ref {
                         None => {
                             for impl_item in impl_items {
-                                visit::walk_impl_item(self, impl_item);
+                                // This is where we choose whether to walk down
+                                // further into the impl to check its items. We
+                                // should only walk into public items so that we
+                                // don't erroneously report errors for private
+                                // types in private items.
+                                match impl_item.node {
+                                    ast::MethodImplItem(..)
+                                        if self.item_is_public(&impl_item.id, impl_item.vis) =>
+                                    {
+                                        visit::walk_impl_item(self, impl_item)
+                                    }
+                                    ast::TypeImplItem(..) => {
+                                        visit::walk_impl_item(self, impl_item)
+                                    }
+                                    _ => {}
+                                }
                             }
                         }
                         Some(ref tr) => {
@@ -1360,7 +1379,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
                         match impl_item.node {
                             ast::MethodImplItem(ref sig, _) => {
                                 if sig.explicit_self.node == ast::SelfStatic &&
-                                   self.exported_items.contains(&impl_item.id) {
+                                        self.item_is_public(&impl_item.id, impl_item.vis) {
                                     found_pub_static = true;
                                     visit::walk_impl_item(self, impl_item);
                                 }
@@ -1381,15 +1400,18 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
             ast::ItemTy(..) => return,
 
             // not at all public, so we don't care
-            _ if !self.exported_items.contains(&item.id) => return,
+            _ if !self.item_is_public(&item.id, item.vis) => {
+                return;
+            }
 
             _ => {}
         }
 
-        // we've carefully constructed it so that if we're here, then
+        // We've carefully constructed it so that if we're here, then
         // any `visit_ty`'s will be called on things that are in
         // public signatures, i.e. things that we're interested in for
         // this visitor.
+        debug!("VisiblePrivateTypesVisitor entering item {:?}", item);
         visit::walk_item(self, item);
     }
 
@@ -1420,20 +1442,12 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
         }
     }
 
-    fn visit_fn(&mut self, fk: visit::FnKind<'v>, fd: &'v ast::FnDecl,
-                b: &'v ast::Block, s: Span, id: ast::NodeId) {
-        // needs special handling for methods.
-        if self.exported_items.contains(&id) {
-            visit::walk_fn(self, fk, fd, b, s);
-        }
-    }
-
     fn visit_ty(&mut self, t: &ast::Ty) {
+        debug!("VisiblePrivateTypesVisitor checking ty {:?}", t);
         if let ast::TyPath(_, ref p) = t.node {
             if !self.tcx.sess.features.borrow().visible_private_types &&
                 self.path_is_private_type(t.id) {
-                self.tcx.sess.span_err(p.span,
-                                       "private type in exported type signature");
+                self.tcx.sess.span_err(p.span, "private type in exported type signature");
             }
         }
         visit::walk_ty(self, t)
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
index 67e9f71551a..b619c6a77d0 100644
--- a/src/librustc_resolve/lib.rs
+++ b/src/librustc_resolve/lib.rs
@@ -382,7 +382,7 @@ enum ModuleKind {
 }
 
 /// One node in the tree of modules.
-struct Module {
+pub struct Module {
     parent_link: ParentLink,
     def_id: Cell<Option<DefId>>,
     kind: Cell<ModuleKind>,
@@ -491,7 +491,7 @@ struct ValueNsDef {
 // Records the definitions (at most one for each namespace) that a name is
 // bound to.
 #[derive(Debug)]
-struct NameBindings {
+pub struct NameBindings {
     type_def: RefCell<Option<TypeNsDef>>,   //< Meaning in type namespace.
     value_def: RefCell<Option<ValueNsDef>>, //< Meaning in value namespace.
 }
@@ -767,7 +767,7 @@ impl PrimitiveTypeTable {
 }
 
 /// The main resolver class.
-struct Resolver<'a, 'tcx:'a> {
+pub struct Resolver<'a, 'tcx:'a> {
     session: &'a Session,
 
     ast_map: &'a ast_map::Map<'tcx>,
diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs
index 5f2803835d5..f584de7c47f 100644
--- a/src/librustc_trans/trans/base.rs
+++ b/src/librustc_trans/trans/base.rs
@@ -1464,7 +1464,7 @@ pub fn arg_kind<'a, 'tcx>(cx: &FunctionContext<'a, 'tcx>, t: Ty<'tcx>)
 }
 
 // work around bizarre resolve errors
-type RvalueDatum<'tcx> = datum::Datum<'tcx, datum::Rvalue>;
+pub type RvalueDatum<'tcx> = datum::Datum<'tcx, datum::Rvalue>;
 
 // create_datums_for_fn_args: creates rvalue datums for each of the
 // incoming function arguments. These will later be stored into
diff --git a/src/librustc_trans/trans/common.rs b/src/librustc_trans/trans/common.rs
index 941ac5d627f..d74ac9029f7 100644
--- a/src/librustc_trans/trans/common.rs
+++ b/src/librustc_trans/trans/common.rs
@@ -378,7 +378,7 @@ pub fn validate_substs(substs: &Substs) {
 
 // work around bizarre resolve errors
 type RvalueDatum<'tcx> = datum::Datum<'tcx, datum::Rvalue>;
-type LvalueDatum<'tcx> = datum::Datum<'tcx, datum::Lvalue>;
+pub type LvalueDatum<'tcx> = datum::Datum<'tcx, datum::Lvalue>;
 
 // Function context.  Every LLVM function we create will have one of
 // these.
diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs
index de0978bc409..afcd9d51709 100644
--- a/src/librustc_typeck/check/mod.rs
+++ b/src/librustc_typeck/check/mod.rs
@@ -195,7 +195,7 @@ type DeferredCallResolutionHandler<'tcx> = Box<DeferredCallResolution<'tcx>+'tcx
 /// When type-checking an expression, we propagate downward
 /// whatever type hint we are able in the form of an `Expectation`.
 #[derive(Copy)]
-enum Expectation<'tcx> {
+pub enum Expectation<'tcx> {
     /// We know nothing about what type this expression should have.
     NoExpectation,
 
diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs
index e1bcad2af37..2b937768e2e 100644
--- a/src/librustc_typeck/check/regionck.rs
+++ b/src/librustc_typeck/check/regionck.rs
@@ -178,7 +178,7 @@ pub struct Rcx<'a, 'tcx: 'a> {
 
 }
 
-struct RepeatingScope(ast::NodeId);
+pub struct RepeatingScope(ast::NodeId);
 pub enum SubjectNode { Subject(ast::NodeId), None }
 
 impl<'a, 'tcx> Rcx<'a, 'tcx> {
diff --git a/src/librustc_typeck/lib.rs b/src/librustc_typeck/lib.rs
index bbc64a54013..6bdfb17ec1c 100644
--- a/src/librustc_typeck/lib.rs
+++ b/src/librustc_typeck/lib.rs
@@ -128,12 +128,12 @@ mod constrained_type_params;
 mod coherence;
 mod variance;
 
-struct TypeAndSubsts<'tcx> {
+pub struct TypeAndSubsts<'tcx> {
     pub substs: subst::Substs<'tcx>,
     pub ty: Ty<'tcx>,
 }
 
-struct CrateCtxt<'a, 'tcx: 'a> {
+pub struct CrateCtxt<'a, 'tcx: 'a> {
     // A mapping from method call sites to traits that have that method.
     trait_map: ty::TraitMap,
     /// A vector of every trait accessible in the whole crate
diff --git a/src/libstd/sys/unix/stack_overflow.rs b/src/libstd/sys/unix/stack_overflow.rs
index 1f212ea5a61..35706682047 100644
--- a/src/libstd/sys/unix/stack_overflow.rs
+++ b/src/libstd/sys/unix/stack_overflow.rs
@@ -144,7 +144,7 @@ mod imp {
         munmap(handler._data, SIGSTKSZ);
     }
 
-    type sighandler_t = *mut libc::c_void;
+    pub type sighandler_t = *mut libc::c_void;
 
     #[cfg(any(all(target_os = "linux", target_arch = "x86"), // may not match
               all(target_os = "linux", target_arch = "x86_64"),
@@ -156,7 +156,7 @@ mod imp {
               target_os = "android"))] // may not match
     mod signal {
         use libc;
-        use super::sighandler_t;
+        pub use super::sighandler_t;
 
         pub static SA_ONSTACK: libc::c_int = 0x08000000;
         pub static SA_SIGINFO: libc::c_int = 0x00000004;
@@ -210,7 +210,7 @@ mod imp {
               target_os = "openbsd"))]
     mod signal {
         use libc;
-        use super::sighandler_t;
+        pub use super::sighandler_t;
 
         pub const SA_ONSTACK: libc::c_int = 0x0001;
         pub const SA_SIGINFO: libc::c_int = 0x0040;
diff --git a/src/libstd/sys/windows/backtrace.rs b/src/libstd/sys/windows/backtrace.rs
index 8638099ca69..8935f97ce5d 100644
--- a/src/libstd/sys/windows/backtrace.rs
+++ b/src/libstd/sys/windows/backtrace.rs
@@ -104,7 +104,7 @@ struct ADDRESS64 {
     Mode: ADDRESS_MODE,
 }
 
-struct STACKFRAME64 {
+pub struct STACKFRAME64 {
     AddrPC: ADDRESS64,
     AddrReturn: ADDRESS64,
     AddrFrame: ADDRESS64,
diff --git a/src/test/compile-fail/priv_in_pub_sig_priv_mod.rs b/src/test/compile-fail/priv_in_pub_sig_priv_mod.rs
new file mode 100644
index 00000000000..f589daf3f39
--- /dev/null
+++ b/src/test/compile-fail/priv_in_pub_sig_priv_mod.rs
@@ -0,0 +1,28 @@
+// 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.
+
+// Test that we properly check for private types in public signatures, even
+// inside a private module (#22261).
+
+mod a {
+    struct Priv;
+
+    pub fn expose_a() -> Priv { //~Error: private type in exported type signature
+        panic!();
+    }
+
+    mod b {
+        pub fn expose_b() -> super::Priv { //~Error: private type in exported type signature
+            panic!();
+        }
+    }
+}
+
+pub fn main() {}
diff --git a/src/test/run-pass/export-unexported-dep.rs b/src/test/run-pass/export-unexported-dep.rs
deleted file mode 100644
index 807d28feb6e..00000000000
--- a/src/test/run-pass/export-unexported-dep.rs
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright 2012 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.
-
-// This tests that exports can have visible dependencies on things
-// that are not exported, allowing for a sort of poor-man's ADT
-
-mod foo {
-    // not exported
-    #[derive(Copy)]
-    enum t { t1, t2, }
-
-    impl PartialEq for t {
-        fn eq(&self, other: &t) -> bool {
-            ((*self) as uint) == ((*other) as uint)
-        }
-        fn ne(&self, other: &t) -> bool { !(*self).eq(other) }
-    }
-
-    pub fn f() -> t { return t::t1; }
-
-    pub fn g(v: t) { assert!((v == t::t1)); }
-}
-
-pub fn main() { foo::g(foo::f()); }
diff --git a/src/test/run-pass/issue-15774.rs b/src/test/run-pass/issue-15774.rs
index 77fa862f7d4..e2f42278cbc 100644
--- a/src/test/run-pass/issue-15774.rs
+++ b/src/test/run-pass/issue-15774.rs
@@ -11,7 +11,7 @@
 #![deny(warnings)]
 #![allow(unused_imports)]
 
-enum Foo { A }
+pub enum Foo { A }
 mod bar {
     pub fn normal(x: ::Foo) {
         use Foo::A;