auto merge of #12595 : huonw/rust/pub-vis-typ, r=alexcrichton

These are types that are in exported type signatures, but are not
exported themselves, e.g.

    struct Foo { ... }

    pub fn bar() -> Foo { ... }

will warn about the Foo.

Such types are not listed in documentation, and cannot be named outside
the crate in which they are declared, which is very user-unfriendly.

cc #10573.

20 files changed, 428 insertions, 21 deletions

diff --git a/src/libextra/workcache.rs b/src/libextra/workcache.rs
index 97c0f786071..2a2493688e6 100644
--- a/src/libextra/workcache.rs
+++ b/src/libextra/workcache.rs
@@ -9,6 +9,7 @@
 // except according to those terms.
 
 #[allow(missing_doc)];
+#[allow(visible_private_types)];
 
 use serialize::json;
 use serialize::json::ToJson;
diff --git a/src/libgreen/lib.rs b/src/libgreen/lib.rs
index 8758eb1179e..dca1c869ad2 100644
--- a/src/libgreen/lib.rs
+++ b/src/libgreen/lib.rs
@@ -173,6 +173,7 @@
 
 // NB this does *not* include globs, please keep it that way.
 #[feature(macro_rules)];
+#[allow(visible_private_types)];
 
 use std::mem::replace;
 use std::os;
diff --git a/src/libnative/io/timer_other.rs b/src/libnative/io/timer_other.rs
index 0784b5ee048..9d700550863 100644
--- a/src/libnative/io/timer_other.rs
+++ b/src/libnative/io/timer_other.rs
@@ -71,6 +71,7 @@ struct Inner {
     id: uint,
 }
 
+#[allow(visible_private_types)]
 pub enum Req {
     // Add a new timer to the helper thread.
     NewTimer(~Inner),
diff --git a/src/libnative/io/timer_timerfd.rs b/src/libnative/io/timer_timerfd.rs
index 7feeaa4768c..68277efc9b7 100644
--- a/src/libnative/io/timer_timerfd.rs
+++ b/src/libnative/io/timer_timerfd.rs
@@ -44,6 +44,7 @@ pub struct Timer {
     priv on_worker: bool,
 }
 
+#[allow(visible_private_types)]
 pub enum Req {
     NewTimer(libc::c_int, Chan<()>, bool, imp::itimerspec),
     RemoveTimer(libc::c_int, Chan<()>),
diff --git a/src/librustc/lib.rs b/src/librustc/lib.rs
index 97718849e63..ff1a6bb7f7e 100644
--- a/src/librustc/lib.rs
+++ b/src/librustc/lib.rs
@@ -30,6 +30,8 @@ This API is completely unstable and subject to change.
 #[feature(macro_rules, globs, struct_variant, managed_boxes)];
 #[feature(quote)];
 
+#[allow(visible_private_types)];
+
 extern crate extra;
 extern crate flate;
 extern crate arena;
diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs
index c3e57bd9c60..693f6fb35f4 100644
--- a/src/librustc/middle/lint.rs
+++ b/src/librustc/middle/lint.rs
@@ -98,6 +98,7 @@ pub enum Lint {
     UnusedMut,
     UnnecessaryAllocation,
     DeadCode,
+    VisiblePrivateTypes,
     UnnecessaryTypecast,
 
     MissingDoc,
@@ -312,6 +313,12 @@ static lint_table: &'static [(&'static str, LintSpec)] = &[
         desc: "detect piece of code that will never be used",
         default: warn
     }),
+    ("visible_private_types",
+     LintSpec {
+        lint: VisiblePrivateTypes,
+        desc: "detect use of private types in exported type signatures",
+        default: warn
+    }),
 
     ("missing_doc",
      LintSpec {
diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs
index 453160dc001..afe4d001036 100644
--- a/src/librustc/middle/privacy.rs
+++ b/src/librustc/middle/privacy.rs
@@ -16,6 +16,7 @@ use std::mem::replace;
 use collections::{HashSet, HashMap};
 
 use metadata::csearch;
+use middle::lint;
 use middle::resolve;
 use middle::ty;
 use middle::typeck::{MethodMap, MethodOrigin, MethodParam};
@@ -1169,6 +1170,251 @@ impl SanePrivacyVisitor {
     }
 }
 
+struct VisiblePrivateTypesVisitor<'a> {
+    tcx: ty::ctxt,
+    exported_items: &'a ExportedItems,
+    public_items: &'a PublicItems,
+}
+
+struct CheckTypeForPrivatenessVisitor<'a, 'b> {
+    inner: &'b VisiblePrivateTypesVisitor<'a>,
+    /// whether the type refers to private types.
+    contains_private: bool,
+    /// whether we've recurred at all (i.e. if we're pointing at the
+    /// first type on which visit_ty was called).
+    at_outer_type: bool,
+    // whether that first type is a public path.
+    outer_type_is_public_path: bool,
+}
+
+impl<'a> VisiblePrivateTypesVisitor<'a> {
+    fn path_is_private_type(&self, path_id: ast::NodeId) -> bool {
+        let did = match self.tcx.def_map.borrow().get().find_copy(&path_id) {
+            // `int` etc. (None doesn't seem to occur.)
+            None | Some(ast::DefPrimTy(..)) => return false,
+            Some(def) => def_id_of_def(def)
+        };
+        // A path can only be private if:
+        // it's in this crate...
+        is_local(did) &&
+            // ... it's not exported (obviously) ...
+            !self.exported_items.contains(&did.node) &&
+            // .. and it corresponds to a type in the AST (this returns None for
+            // type parameters)
+            self.tcx.map.find(did.node).is_some()
+    }
+
+    fn trait_is_public(&self, trait_id: ast::NodeId) -> bool {
+        // FIXME: this would preferably be using `exported_items`, but all
+        // traits are exported currently (see `EmbargoVisitor.exported_trait`)
+        self.public_items.contains(&trait_id)
+    }
+}
+
+impl<'a, 'b> Visitor<()> for CheckTypeForPrivatenessVisitor<'a, 'b> {
+    fn visit_ty(&mut self, ty: &ast::Ty, _: ()) {
+        match ty.node {
+            ast::TyPath(_, _, path_id) => {
+                if self.inner.path_is_private_type(path_id) {
+                    self.contains_private = true;
+                    // found what we're looking for so let's stop
+                    // working.
+                    return
+                } else if self.at_outer_type {
+                    self.outer_type_is_public_path = true;
+                }
+            }
+            _ => {}
+        }
+        self.at_outer_type = false;
+        visit::walk_ty(self, ty, ())
+    }
+
+    // don't want to recurse into [, .. expr]
+    fn visit_expr(&mut self, _: &ast::Expr, _: ()) {}
+}
+
+impl<'a> Visitor<()> for VisiblePrivateTypesVisitor<'a> {
+    fn visit_item(&mut self, item: &ast::Item, _: ()) {
+        match item.node {
+            // contents of a private mod can be reexported, so we need
+            // to check internals.
+            ast::ItemMod(_) => {}
+
+            // An `extern {}` doesn't introduce a new privacy
+            // namespace (the contents have their own privacies).
+            ast::ItemForeignMod(_) => {}
+
+            ast::ItemTrait(..) if !self.trait_is_public(item.id) => return,
+
+            // impls need some special handling to try to offer useful
+            // 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
+            // publically visible.
+            ast::ItemImpl(ref g, ref trait_ref, self_, ref methods) => {
+                // `impl [... for] Private` is never visible.
+                let self_contains_private;
+                // impl [... for] Public<...>, but not `impl [... for]
+                // ~[Public]` or `(Public,)` etc.
+                let self_is_public_path;
+
+                // check the properties of the Self type:
+                {
+                    let mut visitor = CheckTypeForPrivatenessVisitor {
+                        inner: self,
+                        contains_private: false,
+                        at_outer_type: true,
+                        outer_type_is_public_path: false,
+                    };
+                    visitor.visit_ty(self_, ());
+                    self_contains_private = visitor.contains_private;
+                    self_is_public_path = visitor.outer_type_is_public_path;
+                }
+
+                // miscellanous info about the impl
+
+                // `true` iff this is `impl Private for ...`.
+                let not_private_trait =
+                    trait_ref.as_ref().map_or(true, // no trait counts as public trait
+                                              |tr| {
+                        let did = ty::trait_ref_to_def_id(self.tcx, tr);
+
+                        !is_local(did) || self.trait_is_public(did.node)
+                    });
+
+                // `true` iff this is a trait impl or at least one method is public.
+                //
+                // `impl Public { $( fn ...() {} )* }` is not visible.
+                //
+                // This is required over just using the methods' privacy
+                // directly because we might have `impl<T: Foo<Private>> ...`,
+                // and we shouldn't warn about the generics if all the methods
+                // are private (because `T` won't be visible externally).
+                let trait_or_some_public_method =
+                    trait_ref.is_some() ||
+                    methods.iter().any(|m| self.exported_items.contains(&m.id));
+
+                if !self_contains_private &&
+                        not_private_trait &&
+                        trait_or_some_public_method {
+
+                    visit::walk_generics(self, g, ());
+
+                    match *trait_ref {
+                        None => {
+                            for method in methods.iter() {
+                                visit::walk_method_helper(self, *method, ())
+                            }
+                        }
+                        Some(ref tr) => {
+                            // Any private types in a trait impl fall into two
+                            // categories.
+                            // 1. mentioned in the trait definition
+                            // 2. mentioned in the type params/generics
+                            //
+                            // Those in 1. can only occur if the trait is in
+                            // this crate and will've been warned about on the
+                            // trait definition (there's no need to warn twice
+                            // so we don't check the methods).
+                            //
+                            // Those in 2. are warned via walk_generics and this
+                            // call here.
+                            visit::walk_trait_ref_helper(self, tr, ())
+                        }
+                    }
+                } else if trait_ref.is_none() && self_is_public_path {
+                    // impl Public<Private> { ... }. Any public static
+                    // methods will be visible as `Public::foo`.
+                    let mut found_pub_static = false;
+                    for method in methods.iter() {
+                        if method.explicit_self.node == ast::SelfStatic &&
+                            self.exported_items.contains(&method.id) {
+                            found_pub_static = true;
+                            visit::walk_method_helper(self, *method, ());
+                        }
+                    }
+                    if found_pub_static {
+                        visit::walk_generics(self, g, ())
+                    }
+                }
+                return
+            }
+
+            // `type ... = ...;` can contain private types, because
+            // we're introducing a new name.
+            ast::ItemTy(..) => return,
+
+            // not at all public, so we don't care
+            _ if !self.exported_items.contains(&item.id) => return,
+
+            _ => {}
+        }
+
+        // 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.
+        visit::walk_item(self, item, ());
+    }
+
+    fn visit_foreign_item(&mut self, item: &ast::ForeignItem, _: ()) {
+        if self.exported_items.contains(&item.id) {
+            visit::walk_foreign_item(self, item, ())
+        }
+    }
+
+    fn visit_fn(&mut self,
+                fk: &visit::FnKind, fd: &ast::FnDecl, b: &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, id, ());
+        }
+    }
+
+    fn visit_ty(&mut self, t: &ast::Ty, _: ()) {
+        match t.node {
+            ast::TyPath(ref p, _, path_id) => {
+                if self.path_is_private_type(path_id) {
+                    self.tcx.sess.add_lint(lint::VisiblePrivateTypes,
+                                           path_id, p.span,
+                                           ~"private type in exported type signature");
+                }
+            }
+            _ => {}
+        }
+        visit::walk_ty(self, t, ())
+    }
+
+    fn visit_variant(&mut self, v: &ast::Variant, g: &ast::Generics, _: ()) {
+        if self.exported_items.contains(&v.node.id) {
+            visit::walk_variant(self, v, g, ());
+        }
+    }
+
+    fn visit_struct_field(&mut self, s: &ast::StructField, _: ()) {
+        match s.node.kind {
+            // the only way to get here is by being inside a public
+            // struct/enum variant, so the only way to have a private
+            // field is with an explicit `priv`.
+            ast::NamedField(_, ast::Private) => {}
+
+            _ => visit::walk_struct_field(self, s, ())
+        }
+    }
+
+
+    // we don't need to introspect into these at all: an
+    // expression/block context can't possibly contain exported
+    // things, and neither do view_items. (Making them no-ops stops us
+    // from traversing the whole AST without having to be super
+    // careful about our `walk_...` calls above.)
+    fn visit_view_item(&mut self, _: &ast::ViewItem, _: ()) {}
+    fn visit_block(&mut self, _: &ast::Block, _: ()) {}
+    fn visit_expr(&mut self, _: &ast::Expr, _: ()) {}
+}
+
 pub fn check_crate(tcx: ty::ctxt,
                    method_map: &MethodMap,
                    exp_map2: &resolve::ExportMap2,
@@ -1225,5 +1471,14 @@ pub fn check_crate(tcx: ty::ctxt,
     }
 
     let EmbargoVisitor { exported_items, public_items, .. } = visitor;
+
+    {
+        let mut visitor = VisiblePrivateTypesVisitor {
+            tcx: tcx,
+            exported_items: &exported_items,
+            public_items: &public_items
+        };
+        visit::walk_crate(&mut visitor, krate, ());
+    }
     return (exported_items, public_items);
 }
diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs
index d52389f15a3..919a7b208d4 100644
--- a/src/librustdoc/html/render.rs
+++ b/src/librustdoc/html/render.rs
@@ -99,7 +99,7 @@ pub enum ExternalLocation {
 }
 
 /// Different ways an implementor of a trait can be rendered.
-enum Implementor {
+pub enum Implementor {
     /// Paths are displayed specially by omitting the `impl XX for` cruft
     PathType(clean::Type),
     /// This is the generic representation of a trait implementor, used for
diff --git a/src/librustuv/lib.rs b/src/librustuv/lib.rs
index 9b327dc4ee4..890f44faabc 100644
--- a/src/librustuv/lib.rs
+++ b/src/librustuv/lib.rs
@@ -41,6 +41,7 @@ via `close` and `delete` methods.
 
 #[feature(macro_rules)];
 #[deny(unused_result, unused_must_use)];
+#[allow(visible_private_types)];
 
 #[cfg(test)] extern crate green;
 
diff --git a/src/libstd/libc.rs b/src/libstd/libc.rs
index 07be753925f..ef641bbb665 100644
--- a/src/libstd/libc.rs
+++ b/src/libstd/libc.rs
@@ -1130,7 +1130,7 @@ pub mod types {
                     Data4: [BYTE, ..8],
                 }
 
-                struct WSAPROTOCOLCHAIN {
+                pub struct WSAPROTOCOLCHAIN {
                     ChainLen: c_int,
                     ChainEntries: [DWORD, ..MAX_PROTOCOL_CHAIN],
                 }
diff --git a/src/libstd/rt/local.rs b/src/libstd/rt/local.rs
index 76a672b79ca..3cfa494d382 100644
--- a/src/libstd/rt/local.rs
+++ b/src/libstd/rt/local.rs
@@ -24,6 +24,7 @@ pub trait Local<Borrowed> {
     unsafe fn try_unsafe_borrow() -> Option<*mut Self>;
 }
 
+#[allow(visible_private_types)]
 impl Local<local_ptr::Borrowed<Task>> for Task {
     #[inline]
     fn put(value: ~Task) { unsafe { local_ptr::put(value) } }
@@ -127,4 +128,3 @@ mod test {
     }
 
 }
-
diff --git a/src/libstd/rt/local_ptr.rs b/src/libstd/rt/local_ptr.rs
index d4e57ab19b1..898004c665d 100644
--- a/src/libstd/rt/local_ptr.rs
+++ b/src/libstd/rt/local_ptr.rs
@@ -366,6 +366,7 @@ pub mod native {
 
     #[inline]
     #[cfg(not(test))]
+    #[allow(visible_private_types)]
     pub fn maybe_tls_key() -> Option<tls::Key> {
         unsafe {
             // NB: This is a little racy because, while the key is
diff --git a/src/libstd/rt/unwind.rs b/src/libstd/rt/unwind.rs
index b9459aed582..b194a9fe308 100644
--- a/src/libstd/rt/unwind.rs
+++ b/src/libstd/rt/unwind.rs
@@ -280,6 +280,7 @@ fn rust_exception_class() -> uw::_Unwind_Exception_Class {
 
 #[cfg(not(target_arch = "arm"), not(test))]
 #[doc(hidden)]
+#[allow(visible_private_types)]
 pub mod eabi {
     use uw = super::libunwind;
     use libc::c_int;
@@ -333,6 +334,7 @@ pub mod eabi {
 // ARM EHABI uses a slightly different personality routine signature,
 // but otherwise works the same.
 #[cfg(target_arch = "arm", not(test))]
+#[allow(visible_private_types)]
 pub mod eabi {
     use uw = super::libunwind;
     use libc::c_int;
diff --git a/src/libsync/arc.rs b/src/libsync/arc.rs
index db4260a30ee..17a35f33170 100644
--- a/src/libsync/arc.rs
+++ b/src/libsync/arc.rs
@@ -49,14 +49,15 @@ use std::kinds::marker;
 use std::sync::arc::UnsafeArc;
 use std::task;
 
-/// As sync::condvar, a mechanism for unlock-and-descheduling and signaling.
-pub struct Condvar<'a> {
+/// As sync::condvar, a mechanism for unlock-and-descheduling and
+/// signaling, for use with the Arc types.
+pub struct ArcCondvar<'a> {
     priv is_mutex: bool,
     priv failed: &'a bool,
     priv cond: &'a sync::Condvar<'a>
 }
 
-impl<'a> Condvar<'a> {
+impl<'a> ArcCondvar<'a> {
     /// Atomically exit the associated Arc and block until a signal is sent.
     #[inline]
     pub fn wait(&self) { self.wait_on(0) }
@@ -219,14 +220,14 @@ impl<T:Send> MutexArc<T> {
 
     /// As access(), but with a condvar, as sync::mutex.lock_cond().
     #[inline]
-    pub fn access_cond<U>(&self, blk: |x: &mut T, c: &Condvar| -> U) -> U {
+    pub fn access_cond<U>(&self, blk: |x: &mut T, c: &ArcCondvar| -> U) -> U {
         let state = self.x.get();
         unsafe {
             (&(*state).lock).lock_cond(|cond| {
                 check_poison(true, (*state).failed);
                 let _z = PoisonOnFail::new(&mut (*state).failed);
                 blk(&mut (*state).data,
-                    &Condvar {is_mutex: true,
+                    &ArcCondvar {is_mutex: true,
                             failed: &(*state).failed,
                             cond: cond })
             })
@@ -345,7 +346,7 @@ impl<T:Freeze + Send> RWArc<T> {
     /// As write(), but with a condvar, as sync::rwlock.write_cond().
     #[inline]
     pub fn write_cond<U>(&self,
-                         blk: |x: &mut T, c: &Condvar| -> U)
+                         blk: |x: &mut T, c: &ArcCondvar| -> U)
                          -> U {
         unsafe {
             let state = self.x.get();
@@ -353,7 +354,7 @@ impl<T:Freeze + Send> RWArc<T> {
                 check_poison(false, (*state).failed);
                 let _z = PoisonOnFail::new(&mut (*state).failed);
                 blk(&mut (*state).data,
-                    &Condvar {is_mutex: false,
+                    &ArcCondvar {is_mutex: false,
                               failed: &(*state).failed,
                               cond: cond})
             })
@@ -481,7 +482,7 @@ impl<'a, T:Freeze + Send> RWWriteMode<'a, T> {
 
     /// Access the pre-downgrade RWArc in write mode with a condvar.
     pub fn write_cond<U>(&mut self,
-                         blk: |x: &mut T, c: &Condvar| -> U)
+                         blk: |x: &mut T, c: &ArcCondvar| -> U)
                          -> U {
         match *self {
             RWWriteMode {
@@ -491,7 +492,7 @@ impl<'a, T:Freeze + Send> RWWriteMode<'a, T> {
             } => {
                 token.write_cond(|cond| {
                     unsafe {
-                        let cvar = Condvar {
+                        let cvar = ArcCondvar {
                             is_mutex: false,
                             failed: &*poison.flag,
                             cond: cond
@@ -915,7 +916,7 @@ mod tests {
         // rwarc gives us extra shared state to help check for the race.
         // If you want to see this test fail, go to sync.rs and replace the
         // line in RWLock::write_cond() that looks like:
-        //     "blk(&Condvar { order: opt_lock, ..*cond })"
+        //     "blk(&ArcCondvar { order: opt_lock, ..*cond })"
         // with just "blk(cond)".
         let x = RWArc::new(true);
         let (wp, wc) = Chan::new();
diff --git a/src/libsync/lib.rs b/src/libsync/lib.rs
index d96685c7f55..80abcce0df3 100644
--- a/src/libsync/lib.rs
+++ b/src/libsync/lib.rs
@@ -17,7 +17,7 @@
 #[crate_type = "dylib"];
 #[license = "MIT/ASL2"];
 
-pub use arc::{Arc, MutexArc, RWArc, RWWriteMode, RWReadMode, Condvar, CowArc};
+pub use arc::{Arc, MutexArc, RWArc, RWWriteMode, RWReadMode, ArcCondvar, CowArc};
 pub use sync::{Mutex, RWLock, Condvar, Semaphore, RWLockWriteMode,
     RWLockReadMode, Barrier, one, mutex};
 pub use comm::{DuplexStream, SyncChan, SyncPort, rendezvous};
diff --git a/src/libsyntax/abi.rs b/src/libsyntax/abi.rs
index c01f3721fad..861cd8ae7d3 100644
--- a/src/libsyntax/abi.rs
+++ b/src/libsyntax/abi.rs
@@ -48,7 +48,7 @@ pub enum Architecture {
 static IntelBits: u32 = (1 << (X86 as uint)) | (1 << (X86_64 as uint));
 static ArmBits: u32 = (1 << (Arm as uint));
 
-struct AbiData {
+pub struct AbiData {
     abi: Abi,
 
     // Name of this ABI as we like it called.
@@ -59,7 +59,7 @@ struct AbiData {
     abi_arch: AbiArchitecture
 }
 
-enum AbiArchitecture {
+pub enum AbiArchitecture {
     RustArch,   // Not a real ABI (e.g., intrinsic)
     AllArch,    // An ABI that specifies cross-platform defaults (e.g., "C")
     Archs(u32)  // Multiple architectures (bitset)
diff --git a/src/libsyntax/lib.rs b/src/libsyntax/lib.rs
index 42c9ab461aa..260375b5f81 100644
--- a/src/libsyntax/lib.rs
+++ b/src/libsyntax/lib.rs
@@ -31,6 +31,7 @@ This API is completely unstable and subject to change.
 #[feature(quote)];
 
 #[deny(non_camel_case_types)];
+#[allow(visible_private_types)];
 
 extern crate serialize;
 extern crate term;
diff --git a/src/libsyntax/visit.rs b/src/libsyntax/visit.rs
index 248ba593c1f..39989977d69 100644
--- a/src/libsyntax/visit.rs
+++ b/src/libsyntax/visit.rs
@@ -193,9 +193,11 @@ fn walk_explicit_self<E: Clone, V: Visitor<E>>(visitor: &mut V,
     }
 }
 
-fn walk_trait_ref<E: Clone, V: Visitor<E>>(visitor: &mut V,
-                                           trait_ref: &TraitRef,
-                                           env: E) {
+/// Like with walk_method_helper this doesn't correspond to a method
+/// in Visitor, and so it gets a _helper suffix.
+pub fn walk_trait_ref_helper<E: Clone, V: Visitor<E>>(visitor: &mut V,
+                                                      trait_ref: &TraitRef,
+                                                      env: E) {
     visitor.visit_path(&trait_ref.path, trait_ref.ref_id, env)
 }
 
@@ -239,7 +241,8 @@ pub fn walk_item<E: Clone, V: Visitor<E>>(visitor: &mut V, item: &Item, env: E)
                  ref methods) => {
             visitor.visit_generics(type_parameters, env.clone());
             match *trait_reference {
-                Some(ref trait_reference) => walk_trait_ref(visitor, trait_reference, env.clone()),
+                Some(ref trait_reference) => walk_trait_ref_helper(visitor,
+                                                                   trait_reference, env.clone()),
                 None => ()
             }
             visitor.visit_ty(typ, env.clone());
@@ -459,7 +462,7 @@ pub fn walk_ty_param_bounds<E: Clone, V: Visitor<E>>(visitor: &mut V,
     for bound in bounds.iter() {
         match *bound {
             TraitTyParamBound(ref typ) => {
-                walk_trait_ref(visitor, typ, env.clone())
+                walk_trait_ref_helper(visitor, typ, env.clone())
             }
             RegionTyParamBound => {}
         }
diff --git a/src/test/compile-fail/lint-dead-code-1.rs b/src/test/compile-fail/lint-dead-code-1.rs
index b6965a003e0..629a203fcbb 100644
--- a/src/test/compile-fail/lint-dead-code-1.rs
+++ b/src/test/compile-fail/lint-dead-code-1.rs
@@ -11,6 +11,7 @@
 #[no_std];
 #[allow(unused_variable)];
 #[allow(non_camel_case_types)];
+#[allow(visible_private_types)];
 #[deny(dead_code)];
 
 #[crate_type="lib"];
diff --git a/src/test/compile-fail/lint-visible-private-types.rs b/src/test/compile-fail/lint-visible-private-types.rs
new file mode 100644
index 00000000000..6d77f8b324c
--- /dev/null
+++ b/src/test/compile-fail/lint-visible-private-types.rs
@@ -0,0 +1,129 @@
+// Copyright 2014 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.
+
+#[feature(struct_variant)];
+#[deny(visible_private_types)];
+#[allow(dead_code)];
+#[crate_type="lib"];
+
+struct Private<T>;
+pub struct Public<T>;
+
+impl Private<Public<int>> {
+    pub fn a(&self) -> Private<int> { fail!() }
+    fn b(&self) -> Private<int> { fail!() }
+
+    pub fn c() -> Private<int> { fail!() }
+    fn d() -> Private<int> { fail!() }
+}
+impl Private<int> {
+    pub fn e(&self) -> Private<int> { fail!() }
+    fn f(&self) -> Private<int> { fail!() }
+}
+
+impl Public<Private<int>> {
+    pub fn a(&self) -> Private<int> { fail!() }
+    fn b(&self) -> Private<int> { fail!() }
+
+    pub fn c() -> Private<int> { fail!() } //~ ERROR private type in exported type signature
+    fn d() -> Private<int> { fail!() }
+}
+impl Public<int> {
+    pub fn e(&self) -> Private<int> { fail!() } //~ ERROR private type in exported type signature
+    fn f(&self) -> Private<int> { fail!() }
+}
+
+pub fn x(_: Private<int>) {} //~ ERROR private type in exported type signature
+
+fn y(_: Private<int>) {}
+
+
+pub struct Foo {
+    x: Private<int>, //~ ERROR private type in exported type signature
+    priv y: Private<int>
+}
+
+struct Bar {
+    x: Private<int>,
+}
+
+pub enum Baz {
+    Baz1(Private<int>), //~ ERROR private type in exported type signature
+    Baz2 {
+        x: Private<int>, //~ ERROR private type in exported type signature
+        priv y: Private<int>
+    },
+
+    priv Baz3(Private<int>),
+    priv Baz4 {
+        x: Private<int>,
+    }
+}
+
+enum Qux {
+    Qux1(Private<int>),
+    Qux2 {
+        x: Private<int>,
+    }
+}
+
+pub trait PubTrait {
+    fn foo(&self) -> Private<int> { fail!( )} //~ ERROR private type in exported type signature
+    fn bar(&self) -> Private<int>; //~ ERROR private type in exported type signature
+    fn baz() -> Private<int>; //~ ERROR private type in exported type signature
+}
+
+impl PubTrait for Public<int> {
+    fn bar(&self) -> Private<int> { fail!() }
+    fn baz() -> Private<int> { fail!() }
+}
+impl PubTrait for Public<Private<int>> {
+    fn bar(&self) -> Private<int> { fail!() }
+    fn baz() -> Private<int> { fail!() }
+}
+
+impl PubTrait for Private<int> {
+    fn bar(&self) -> Private<int> { fail!() }
+    fn baz() -> Private<int> { fail!() }
+}
+impl PubTrait for (Private<int>,) {
+    fn bar(&self) -> Private<int> { fail!() }
+    fn baz() -> Private<int> { fail!() }
+}
+
+
+trait PrivTrait {
+    fn foo(&self) -> Private<int> { fail!( )}
+    fn bar(&self) -> Private<int>;
+}
+impl PrivTrait for Private<int> {
+    fn bar(&self) -> Private<int> { fail!() }
+}
+impl PrivTrait for (Private<int>,) {
+    fn bar(&self) -> Private<int> { fail!() }
+}
+
+pub trait ParamTrait<T> {
+    fn foo() -> T;
+}
+
+impl ParamTrait<Private<int>> //~ ERROR private type in exported type signature
+   for Public<int> {
+    fn foo() -> Private<int> { fail!() }
+}
+
+impl ParamTrait<Private<int>> for Private<int> {
+    fn foo() -> Private<int> { fail!( )}
+}
+
+impl<T: ParamTrait<Private<int>>>  //~ ERROR private type in exported type signature
+     ParamTrait<T> for Public<i8> {
+    fn foo() -> T { fail!() }
+}