Skip to content

Commit 9522a08

Browse files
alexcrichtonalexcrichton
committed
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
1 parent 29ca435 commit 9522a08

File tree

7 files changed

+63
-35
lines changed

7 files changed

+63
-35
lines changed

‎src/librustc/middle/borrowck/check_loans.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ impl<'self> Visitor<()> for CheckLoanCtxt<'self> {
5858
b:ast::P<ast::Block>, s:Span, n:ast::NodeId, _:()) {
5959
check_loans_in_fn(self, fk, fd, b, s, n);
6060
}
61+
62+
// FIXME(#10894) should continue recursing
63+
fn visit_ty(&mut self, _t: &ast::Ty, _: ()) {}
6164
}
6265

6366
pub fn check_loans(bccx: &BorrowckCtxt,

‎src/librustc/middle/lint.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1364,6 +1364,9 @@ impl<'self> Visitor<()> for Context<'self> {
13641364
visit::walk_variant(cx, v, g, ());
13651365
})
13661366
}
1367+
1368+
// FIXME(#10894) should continue recursing
1369+
fn visit_ty(&mut self, _t: &ast::Ty, _: ()) {}
13671370
}
13681371

13691372
impl<'self> IdVisitingOperation for Context<'self> {

‎src/librustc/middle/moves.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,8 @@ impl visit::Visitor<()> for VisitContext {
202202
fn visit_local(&mut self, l:@Local, _:()) {
203203
compute_modes_for_local(self, l);
204204
}
205+
// FIXME(#10894) should continue recursing
206+
fn visit_ty(&mut self, _t: &Ty, _: ()) {}
205207
}
206208

207209
pub fn compute_moves(tcx: ty::ctxt,

‎src/librustc/middle/privacy.rs

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,7 @@ struct PrivacyVisitor<'self> {
312312
tcx: ty::ctxt,
313313
curitem: ast::NodeId,
314314
in_fn: bool,
315+
in_foreign: bool,
315316
method_map: &'self method_map,
316317
parents: HashMap<ast::NodeId, ast::NodeId>,
317318
external_exports: resolve::ExternalExports,
@@ -625,7 +626,9 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
625626
let t = ty::type_autoderef(self.tcx,
626627
ty::expr_ty(self.tcx, base));
627628
match ty::get(t).sty {
628-
ty::ty_struct(id, _) => self.check_field(expr.span, id, ident),
629+
ty::ty_struct(id, _) => {
630+
self.check_field(expr.span, id, ident);
631+
}
629632
_ => {}
630633
}
631634
}
@@ -649,9 +652,6 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
649652
_ => {}
650653
}
651654
}
652-
ast::ExprPath(ref path) => {
653-
self.check_path(expr.span, expr.id, path);
654-
}
655655
ast::ExprStruct(_, ref fields, _) => {
656656
match ty::get(ty::expr_ty(self.tcx, expr)).sty {
657657
ty::ty_struct(id, _) => {
@@ -697,25 +697,14 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
697697
visit::walk_expr(self, expr, ());
698698
}
699699

700-
fn visit_ty(&mut self, t: &ast::Ty, _: ()) {
701-
match t.node {
702-
ast::ty_path(ref path, _, id) => self.check_path(t.span, id, path),
703-
_ => {}
704-
}
705-
visit::walk_ty(self, t, ());
706-
}
707-
708700
fn visit_view_item(&mut self, a: &ast::view_item, _: ()) {
709701
match a.node {
710702
ast::view_item_extern_mod(..) => {}
711703
ast::view_item_use(ref uses) => {
712704
for vpath in uses.iter() {
713705
match vpath.node {
714-
ast::view_path_simple(_, ref path, id) |
715-
ast::view_path_glob(ref path, id) => {
716-
debug!("privacy - glob/simple {}", id);
717-
self.check_path(vpath.span, id, path);
718-
}
706+
ast::view_path_simple(..) |
707+
ast::view_path_glob(..) => {}
719708
ast::view_path_list(_, ref list, _) => {
720709
for pid in list.iter() {
721710
debug!("privacy - list {}", pid.node.id);
@@ -737,9 +726,16 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
737726
}
738727
}
739728
}
729+
visit::walk_view_item(self, a, ());
740730
}
741731

742732
fn visit_pat(&mut self, pattern: &ast::Pat, _: ()) {
733+
// Foreign functions do not have their patterns mapped in the def_map,
734+
// and there's nothing really relevant there anyway, so don't bother
735+
// checking privacy. If you can name the type then you can pass it to an
736+
// external C function anyway.
737+
if self.in_foreign { return }
738+
743739
match pattern.node {
744740
ast::PatStruct(_, ref fields, _) => {
745741
match ty::get(ty::pat_ty(self.tcx, pattern)).sty {
@@ -773,6 +769,17 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> {
773769

774770
visit::walk_pat(self, pattern, ());
775771
}
772+
773+
fn visit_foreign_item(&mut self, fi: @ast::foreign_item, _: ()) {
774+
self.in_foreign = true;
775+
visit::walk_foreign_item(self, fi, ());
776+
self.in_foreign = false;
777+
}
778+
779+
fn visit_path(&mut self, path: &ast::Path, id: ast::NodeId, _: ()) {
780+
self.check_path(path.span, id, path);
781+
visit::walk_path(self, path, ());
782+
}
776783
}
777784

778785
////////////////////////////////////////////////////////////////////////////////
@@ -999,6 +1006,7 @@ pub fn check_crate(tcx: ty::ctxt,
9991006
let mut visitor = PrivacyVisitor {
10001007
curitem: ast::DUMMY_NODE_ID,
10011008
in_fn: false,
1009+
in_foreign: false,
10021010
tcx: tcx,
10031011
parents: visitor.parents,
10041012
method_map: method_map,

‎src/librustc/middle/typeck/check/writeback.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,8 @@ impl Visitor<()> for WbCtxt {
325325
fn visit_block(&mut self, b:ast::P<ast::Block>, _:()) { visit_block(b, self); }
326326
fn visit_pat(&mut self, p:&ast::Pat, _:()) { visit_pat(p, self); }
327327
fn visit_local(&mut self, l:@ast::Local, _:()) { visit_local(l, self); }
328+
// FIXME(#10894) should continue recursing
329+
fn visit_ty(&mut self, _t: &ast::Ty, _:()) {}
328330
}
329331

330332
pub fn resolve_type_vars_in_expr(fcx: @mut FnCtxt, e: @ast::Expr) -> bool {

‎src/libsyntax/visit.rs

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ pub trait Visitor<E:Clone> {
8282
fn visit_decl(&mut self, d:@Decl, e:E) { walk_decl(self, d, e) }
8383
fn visit_expr(&mut self, ex:@Expr, e:E) { walk_expr(self, ex, e) }
8484
fn visit_expr_post(&mut self, _ex:@Expr, _e:E) { }
85-
fn visit_ty(&mut self, _t:&Ty, _e:E) { }
85+
fn visit_ty(&mut self, t:&Ty, e:E) { walk_ty(self, t, e) }
8686
fn visit_generics(&mut self, g:&Generics, e:E) { walk_generics(self, g, e) }
8787
fn visit_fn(&mut self, fk:&fn_kind, fd:&fn_decl, b:P<Block>, s:Span, n:NodeId, e:E) {
8888
walk_fn(self, fk, fd, b, s, n , e)
@@ -120,6 +120,9 @@ pub trait Visitor<E:Clone> {
120120
fn visit_mac(&mut self, macro:&mac, e:E) {
121121
walk_mac(self, macro, e)
122122
}
123+
fn visit_path(&mut self, path: &Path, _id: ast::NodeId, e: E) {
124+
walk_path(self, path, e)
125+
}
123126
}
124127

125128
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
143146
}
144147
view_item_use(ref paths) => {
145148
for vp in paths.iter() {
146-
let path = match vp.node {
147-
view_path_simple(ident, ref path, _) => {
149+
match vp.node {
150+
view_path_simple(ident, ref path, id) => {
148151
visitor.visit_ident(vp.span, ident, env.clone());
149-
path
152+
visitor.visit_path(path, id, env.clone());
153+
}
154+
view_path_glob(ref path, id) => {
155+
visitor.visit_path(path, id, env.clone());
150156
}
151-
view_path_glob(ref path, _) => path,
152157
view_path_list(ref path, ref list, _) => {
153158
for id in list.iter() {
154159
visitor.visit_ident(id.span, id.node.name, env.clone())
155160
}
156-
path
161+
walk_path(visitor, path, env.clone());
157162
}
158-
};
159-
160-
walk_path(visitor, path, env.clone());
163+
}
161164
}
162165
}
163166
}
@@ -187,7 +190,7 @@ fn walk_explicit_self<E:Clone, V:Visitor<E>>(visitor: &mut V,
187190
fn walk_trait_ref<E:Clone, V:Visitor<E>>(visitor: &mut V,
188191
trait_ref: &ast::trait_ref,
189192
env: E) {
190-
walk_path(visitor, &trait_ref.path, env)
193+
visitor.visit_path(&trait_ref.path, trait_ref.ref_id, env)
191194
}
192195

193196
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) {
248251
item_trait(ref generics, ref trait_paths, ref methods) => {
249252
visitor.visit_generics(generics, env.clone());
250253
for trait_path in trait_paths.iter() {
251-
walk_path(visitor, &trait_path.path, env.clone())
254+
visitor.visit_path(&trait_path.path,
255+
trait_path.ref_id,
256+
env.clone())
252257
}
253258
for method in methods.iter() {
254259
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) {
331336
walk_lifetime_decls(visitor, &function_declaration.lifetimes,
332337
env.clone());
333338
}
334-
ty_path(ref path, ref bounds, _) => {
335-
walk_path(visitor, path, env.clone());
339+
ty_path(ref path, ref bounds, id) => {
340+
visitor.visit_path(path, id, env.clone());
336341
for bounds in bounds.iter() {
337342
walk_ty_param_bounds(visitor, bounds, env.clone())
338343
}
@@ -372,15 +377,15 @@ pub fn walk_path<E:Clone, V:Visitor<E>>(visitor: &mut V, path: &Path, env: E) {
372377
pub fn walk_pat<E:Clone, V:Visitor<E>>(visitor: &mut V, pattern: &Pat, env: E) {
373378
match pattern.node {
374379
PatEnum(ref path, ref children) => {
375-
walk_path(visitor, path, env.clone());
380+
visitor.visit_path(path, pattern.id, env.clone());
376381
for children in children.iter() {
377382
for child in children.iter() {
378383
visitor.visit_pat(*child, env.clone())
379384
}
380385
}
381386
}
382387
PatStruct(ref path, ref fields, _) => {
383-
walk_path(visitor, path, env.clone());
388+
visitor.visit_path(path, pattern.id, env.clone());
384389
for field in fields.iter() {
385390
visitor.visit_pat(field.pat, env.clone())
386391
}
@@ -396,7 +401,7 @@ pub fn walk_pat<E:Clone, V:Visitor<E>>(visitor: &mut V, pattern: &Pat, env: E) {
396401
visitor.visit_pat(subpattern, env)
397402
}
398403
PatIdent(_, ref path, ref optional_subpattern) => {
399-
walk_path(visitor, path, env.clone());
404+
visitor.visit_path(path, pattern.id, env.clone());
400405
match *optional_subpattern {
401406
None => {}
402407
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:
617622
visitor.visit_expr(count, env.clone())
618623
}
619624
ExprStruct(ref path, ref fields, optional_base) => {
620-
walk_path(visitor, path, env.clone());
625+
visitor.visit_path(path, expression.id, env.clone());
621626
for field in fields.iter() {
622627
visitor.visit_expr(field.expr, env.clone())
623628
}
@@ -711,7 +716,9 @@ pub fn walk_expr<E:Clone, V:Visitor<E>>(visitor: &mut V, expression: @Expr, env:
711716
visitor.visit_expr(main_expression, env.clone());
712717
visitor.visit_expr(index_expression, env.clone())
713718
}
714-
ExprPath(ref path) => walk_path(visitor, path, env.clone()),
719+
ExprPath(ref path) => {
720+
visitor.visit_path(path, expression.id, env.clone())
721+
}
715722
ExprSelf | ExprBreak(_) | ExprAgain(_) => {}
716723
ExprRet(optional_expression) => {
717724
walk_expr_opt(visitor, optional_expression, env.clone())

‎src/test/compile-fail/privacy1.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,9 @@ mod foo {
162162
bar::foo();
163163
bar::bar();
164164
}
165+
166+
impl ::bar::B for f32 { fn foo() -> f32 { 1.0 } }
167+
//~^ ERROR: trait `B` is private
165168
}
166169

167170
pub mod mytest {

0 commit comments

Comments
 (0)