Check the privacy of implemented traits

This bug showed up because the visitor only visited the path of the implemented
trait via walk_path (with no corresponding visit_path function). I have modified
the visitor to use visit_path (which is now overridable), and the privacy
visitor overrides this function and now properly checks for the privacy of all
paths.

Closes #10857

7 files changed, 63 insertions, 35 deletions

diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs
index 77dde581c33..f335c789db3 100644
--- a/src/librustc/middle/borrowck/check_loans.rs
+++ b/src/librustc/middle/borrowck/check_loans.rs
@@ -58,6 +58,9 @@ impl<'self> Visitor<()> for CheckLoanCtxt<'self> {
                 b:ast::P<ast::Block>, s:Span, n:ast::NodeId, _:()) {
         check_loans_in_fn(self, fk, fd, b, s, n);
     }
+
+    // FIXME(#10894) should continue recursing
+    fn visit_ty(&mut self, _t: &ast::Ty, _: ()) {}
 }
 
 pub fn check_loans(bccx: &BorrowckCtxt,
diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs
index 2a69b8226ae..9e1ef411c2d 100644
--- a/src/librustc/middle/lint.rs
+++ b/src/librustc/middle/lint.rs
@@ -1364,6 +1364,9 @@ impl<'self> Visitor<()> for Context<'self> {
             visit::walk_variant(cx, v, g, ());
         })
     }
+
+    // FIXME(#10894) should continue recursing
+    fn visit_ty(&mut self, _t: &ast::Ty, _: ()) {}
 }
 
 impl<'self> IdVisitingOperation for Context<'self> {
diff --git a/src/librustc/middle/moves.rs b/src/librustc/middle/moves.rs
index 8cdca441d8a..c9911480199 100644
--- a/src/librustc/middle/moves.rs
+++ b/src/librustc/middle/moves.rs
@@ -202,6 +202,8 @@ impl visit::Visitor<()> for VisitContext {
     fn visit_local(&mut self, l:@Local, _:()) {
         compute_modes_for_local(self, l);
     }
+    // FIXME(#10894) should continue recursing
+    fn visit_ty(&mut self, _t: &Ty, _: ()) {}
 }
 
 pub fn compute_moves(tcx: ty::ctxt,
diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs
index b5d0fad7f95..8b60bbce401 100644
--- a/src/librustc/middle/privacy.rs
+++ b/src/librustc/middle/privacy.rs
@@ -312,6 +312,7 @@ struct PrivacyVisitor<'self> {
     tcx: ty::ctxt,
     curitem: ast::NodeId,
     in_fn: bool,
+    in_foreign: bool,
     method_map: &'self method_map,
     parents: HashMap<ast::NodeId, ast::NodeId>,
     external_exports: resolve::ExternalExports,
@@ -625,7 +626,9 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
                 let t = ty::type_autoderef(self.tcx,
                                            ty::expr_ty(self.tcx, base));
                 match ty::get(t).sty {
-                    ty::ty_struct(id, _) => self.check_field(expr.span, id, ident),
+                    ty::ty_struct(id, _) => {
+                        self.check_field(expr.span, id, ident);
+                    }
                     _ => {}
                 }
             }
@@ -649,9 +652,6 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
                     _ => {}
                 }
             }
-            ast::ExprPath(ref path) => {
-                self.check_path(expr.span, expr.id, path);
-            }
             ast::ExprStruct(_, ref fields, _) => {
                 match ty::get(ty::expr_ty(self.tcx, expr)).sty {
                     ty::ty_struct(id, _) => {
@@ -697,25 +697,14 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
         visit::walk_expr(self, expr, ());
     }
 
-    fn visit_ty(&mut self, t: &ast::Ty, _: ()) {
-        match t.node {
-            ast::ty_path(ref path, _, id) => self.check_path(t.span, id, path),
-            _ => {}
-        }
-        visit::walk_ty(self, t, ());
-    }
-
     fn visit_view_item(&mut self, a: &ast::view_item, _: ()) {
         match a.node {
             ast::view_item_extern_mod(..) => {}
             ast::view_item_use(ref uses) => {
                 for vpath in uses.iter() {
                     match vpath.node {
-                        ast::view_path_simple(_, ref path, id) |
-                        ast::view_path_glob(ref path, id) => {
-                            debug!("privacy - glob/simple {}", id);
-                            self.check_path(vpath.span, id, path);
-                        }
+                        ast::view_path_simple(..) |
+                        ast::view_path_glob(..) => {}
                         ast::view_path_list(_, ref list, _) => {
                             for pid in list.iter() {
                                 debug!("privacy - list {}", pid.node.id);
@@ -737,9 +726,16 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
                 }
             }
         }
+        visit::walk_view_item(self, a, ());
     }
 
     fn visit_pat(&mut self, pattern: &ast::Pat, _: ()) {
+        // Foreign functions do not have their patterns mapped in the def_map,
+        // and there's nothing really relevant there anyway, so don't bother
+        // checking privacy. If you can name the type then you can pass it to an
+        // external C function anyway.
+        if self.in_foreign { return }
+
         match pattern.node {
             ast::PatStruct(_, ref fields, _) => {
                 match ty::get(ty::pat_ty(self.tcx, pattern)).sty {
@@ -773,6 +769,17 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
 
         visit::walk_pat(self, pattern, ());
     }
+
+    fn visit_foreign_item(&mut self, fi: @ast::foreign_item, _: ()) {
+        self.in_foreign = true;
+        visit::walk_foreign_item(self, fi, ());
+        self.in_foreign = false;
+    }
+
+    fn visit_path(&mut self, path: &ast::Path, id: ast::NodeId, _: ()) {
+        self.check_path(path.span, id, path);
+        visit::walk_path(self, path, ());
+    }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -999,6 +1006,7 @@ pub fn check_crate(tcx: ty::ctxt,
     let mut visitor = PrivacyVisitor {
         curitem: ast::DUMMY_NODE_ID,
         in_fn: false,
+        in_foreign: false,
         tcx: tcx,
         parents: visitor.parents,
         method_map: method_map,
diff --git a/src/librustc/middle/typeck/check/writeback.rs b/src/librustc/middle/typeck/check/writeback.rs
index 93463f507c7..f90ae3fdc63 100644
--- a/src/librustc/middle/typeck/check/writeback.rs
+++ b/src/librustc/middle/typeck/check/writeback.rs
@@ -325,6 +325,8 @@ impl Visitor<()> for WbCtxt {
     fn visit_block(&mut self, b:ast::P<ast::Block>, _:()) { visit_block(b, self); }
     fn visit_pat(&mut self, p:&ast::Pat, _:()) { visit_pat(p, self); }
     fn visit_local(&mut self, l:@ast::Local, _:()) { visit_local(l, self); }
+    // FIXME(#10894) should continue recursing
+    fn visit_ty(&mut self, _t: &ast::Ty, _:()) {}
 }
 
 pub fn resolve_type_vars_in_expr(fcx: @mut FnCtxt, e: @ast::Expr) -> bool {
diff --git a/src/libsyntax/visit.rs b/src/libsyntax/visit.rs
index 985dcd27271..a5a8513fa71 100644
--- a/src/libsyntax/visit.rs
+++ b/src/libsyntax/visit.rs
@@ -82,7 +82,7 @@ pub trait Visitor<E:Clone> {
     fn visit_decl(&mut self, d:@Decl, e:E) { walk_decl(self, d, e) }
     fn visit_expr(&mut self, ex:@Expr, e:E) { walk_expr(self, ex, e) }
     fn visit_expr_post(&mut self, _ex:@Expr, _e:E) { }
-    fn visit_ty(&mut self, _t:&Ty, _e:E) { }
+    fn visit_ty(&mut self, t:&Ty, e:E) { walk_ty(self, t, e) }
     fn visit_generics(&mut self, g:&Generics, e:E) { walk_generics(self, g, e) }
     fn visit_fn(&mut self, fk:&fn_kind, fd:&fn_decl, b:P<Block>, s:Span, n:NodeId, e:E) {
         walk_fn(self, fk, fd, b, s, n , e)
@@ -120,6 +120,9 @@ pub trait Visitor<E:Clone> {
     fn visit_mac(&mut self, macro:&mac, e:E) {
         walk_mac(self, macro, e)
     }
+    fn visit_path(&mut self, path: &Path, _id: ast::NodeId, e: E) {
+        walk_path(self, path, e)
+    }
 }
 
 pub fn walk_crate<E:Clone, V:Visitor<E>>(visitor: &mut V, crate: &Crate, env: E) {
@@ -143,21 +146,21 @@ pub fn walk_view_item<E:Clone, V:Visitor<E>>(visitor: &mut V, vi: &view_item, en
         }
         view_item_use(ref paths) => {
             for vp in paths.iter() {
-                let path = match vp.node {
-                    view_path_simple(ident, ref path, _) => {
+                match vp.node {
+                    view_path_simple(ident, ref path, id) => {
                         visitor.visit_ident(vp.span, ident, env.clone());
-                        path
+                        visitor.visit_path(path, id, env.clone());
+                    }
+                    view_path_glob(ref path, id) => {
+                        visitor.visit_path(path, id, env.clone());
                     }
-                    view_path_glob(ref path, _) => path,
                     view_path_list(ref path, ref list, _) => {
                         for id in list.iter() {
                             visitor.visit_ident(id.span, id.node.name, env.clone())
                         }
-                        path
+                        walk_path(visitor, path, env.clone());
                     }
-                };
-
-                walk_path(visitor, path, env.clone());
+                }
             }
         }
     }
@@ -187,7 +190,7 @@ 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: &ast::trait_ref,
                             env: E) {
-    walk_path(visitor, &trait_ref.path, env)
+    visitor.visit_path(&trait_ref.path, trait_ref.ref_id, env)
 }
 
 pub fn walk_item<E:Clone, V:Visitor<E>>(visitor: &mut V, item: &item, env: E) {
@@ -248,7 +251,9 @@ pub fn walk_item<E:Clone, V:Visitor<E>>(visitor: &mut V, item: &item, env: E) {
         item_trait(ref generics, ref trait_paths, ref methods) => {
             visitor.visit_generics(generics, env.clone());
             for trait_path in trait_paths.iter() {
-                walk_path(visitor, &trait_path.path, env.clone())
+                visitor.visit_path(&trait_path.path,
+                                   trait_path.ref_id,
+                                   env.clone())
             }
             for method in methods.iter() {
                 visitor.visit_trait_method(method, env.clone())
@@ -331,8 +336,8 @@ pub fn walk_ty<E:Clone, V:Visitor<E>>(visitor: &mut V, typ: &Ty, env: E) {
             walk_lifetime_decls(visitor, &function_declaration.lifetimes,
                                 env.clone());
         }
-        ty_path(ref path, ref bounds, _) => {
-            walk_path(visitor, path, env.clone());
+        ty_path(ref path, ref bounds, id) => {
+            visitor.visit_path(path, id, env.clone());
             for bounds in bounds.iter() {
                 walk_ty_param_bounds(visitor, bounds, env.clone())
             }
@@ -372,7 +377,7 @@ pub fn walk_path<E:Clone, V:Visitor<E>>(visitor: &mut V, path: &Path, env: E) {
 pub fn walk_pat<E:Clone, V:Visitor<E>>(visitor: &mut V, pattern: &Pat, env: E) {
     match pattern.node {
         PatEnum(ref path, ref children) => {
-            walk_path(visitor, path, env.clone());
+            visitor.visit_path(path, pattern.id, env.clone());
             for children in children.iter() {
                 for child in children.iter() {
                     visitor.visit_pat(*child, env.clone())
@@ -380,7 +385,7 @@ pub fn walk_pat<E:Clone, V:Visitor<E>>(visitor: &mut V, pattern: &Pat, env: E) {
             }
         }
         PatStruct(ref path, ref fields, _) => {
-            walk_path(visitor, path, env.clone());
+            visitor.visit_path(path, pattern.id, env.clone());
             for field in fields.iter() {
                 visitor.visit_pat(field.pat, env.clone())
             }
@@ -396,7 +401,7 @@ pub fn walk_pat<E:Clone, V:Visitor<E>>(visitor: &mut V, pattern: &Pat, env: E) {
             visitor.visit_pat(subpattern, env)
         }
         PatIdent(_, ref path, ref optional_subpattern) => {
-            walk_path(visitor, path, env.clone());
+            visitor.visit_path(path, pattern.id, env.clone());
             match *optional_subpattern {
                 None => {}
                 Some(subpattern) => visitor.visit_pat(subpattern, env),
@@ -617,7 +622,7 @@ pub fn walk_expr<E:Clone, V:Visitor<E>>(visitor: &mut V, expression: @Expr, env:
             visitor.visit_expr(count, env.clone())
         }
         ExprStruct(ref path, ref fields, optional_base) => {
-            walk_path(visitor, path, env.clone());
+            visitor.visit_path(path, expression.id, env.clone());
             for field in fields.iter() {
                 visitor.visit_expr(field.expr, env.clone())
             }
@@ -711,7 +716,9 @@ pub fn walk_expr<E:Clone, V:Visitor<E>>(visitor: &mut V, expression: @Expr, env:
             visitor.visit_expr(main_expression, env.clone());
             visitor.visit_expr(index_expression, env.clone())
         }
-        ExprPath(ref path) => walk_path(visitor, path, env.clone()),
+        ExprPath(ref path) => {
+            visitor.visit_path(path, expression.id, env.clone())
+        }
         ExprSelf | ExprBreak(_) | ExprAgain(_) => {}
         ExprRet(optional_expression) => {
             walk_expr_opt(visitor, optional_expression, env.clone())
diff --git a/src/test/compile-fail/privacy1.rs b/src/test/compile-fail/privacy1.rs
index a2f8e78590f..fdc681e1da0 100644
--- a/src/test/compile-fail/privacy1.rs
+++ b/src/test/compile-fail/privacy1.rs
@@ -162,6 +162,9 @@ mod foo {
         bar::foo();
         bar::bar();
     }
+
+    impl ::bar::B for f32 { fn foo() -> f32 { 1.0 } }
+    //~^ ERROR: trait `B` is private
 }
 
 pub mod mytest {