From dab8fec4af85c94b65d7036129f89a7e7bf6cbac Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 5 Nov 2013 18:06:27 -0800 Subject: [PATCH] Forbid privacy in inner functions Closes #10111 --- doc/rust.md | 1 + doc/tutorial-container.md | 2 + doc/tutorial-macros.md | 6 +- doc/tutorial.md | 7 +- src/libextra/sort.rs | 4 +- src/librustc/middle/privacy.rs | 82 +++++++++++++++++++- src/librustc/middle/ty.rs | 2 +- src/libsyntax/ext/expand.rs | 2 +- src/test/compile-fail/unnecessary-private.rs | 26 +++++++ src/test/run-pass/nested-class.rs | 2 +- 10 files changed, 121 insertions(+), 13 deletions(-) create mode 100644 src/test/compile-fail/unnecessary-private.rs diff --git a/doc/rust.md b/doc/rust.md index 389ea58ee15..c0104e0f756 100644 --- a/doc/rust.md +++ b/doc/rust.md @@ -1548,6 +1548,7 @@ keyword for struct fields and enum variants). When an item is declared as `pub`, it can be thought of as being accessible to the outside world. For example: ~~~~ +# fn main() {} // Declare a private struct struct Foo; diff --git a/doc/tutorial-container.md b/doc/tutorial-container.md index bd0510c4fb3..65f536fa9f2 100644 --- a/doc/tutorial-container.md +++ b/doc/tutorial-container.md @@ -87,6 +87,7 @@ Reaching the end of the iterator is signalled by returning `None` instead of `Some(item)`: ~~~ +# fn main() {} /// A stream of N zeroes struct ZeroStream { priv remaining: uint @@ -301,6 +302,7 @@ the iterator can provide better information. The `ZeroStream` from earlier can provide an exact lower and upper bound: ~~~ +# fn main() {} /// A stream of N zeroes struct ZeroStream { priv remaining: uint diff --git a/doc/tutorial-macros.md b/doc/tutorial-macros.md index 4117faa1a6b..0ad8adf3cc7 100644 --- a/doc/tutorial-macros.md +++ b/doc/tutorial-macros.md @@ -216,7 +216,7 @@ Now consider code like the following: ~~~~ # enum t1 { good_1(t2, uint), bad_1 }; -# pub struct t2 { body: t3 } +# struct t2 { body: t3 } # enum t3 { good_2(uint), bad_2}; # fn f(x: t1) -> uint { match x { @@ -262,7 +262,7 @@ macro_rules! biased_match ( ) # enum t1 { good_1(t2, uint), bad_1 }; -# pub struct t2 { body: t3 } +# struct t2 { body: t3 } # enum t3 { good_2(uint), bad_2}; # fn f(x: t1) -> uint { biased_match!((x) ~ (good_1(g1, val)) else { return 0 }; @@ -364,7 +364,7 @@ macro_rules! biased_match ( # enum t1 { good_1(t2, uint), bad_1 }; -# pub struct t2 { body: t3 } +# struct t2 { body: t3 } # enum t3 { good_2(uint), bad_2}; # fn f(x: t1) -> uint { biased_match!( diff --git a/doc/tutorial.md b/doc/tutorial.md index 685c9f72217..1b414c40834 100644 --- a/doc/tutorial.md +++ b/doc/tutorial.md @@ -2568,6 +2568,7 @@ pub fn foo() { bar(); } ~~~ // c.rs pub fn bar() { println("Baz!"); } +# fn main() {} ~~~ There also exist two short forms for importing multiple names at once: @@ -2743,7 +2744,7 @@ Therefore, if you plan to compile your crate as a library, you should annotate i #[link(name = "farm", vers = "2.5")]; // ... -# pub fn farm() {} +# fn farm() {} ~~~~ You can also in turn require in a `extern mod` statement that certain link metadata items match some criteria. @@ -2769,7 +2770,7 @@ or setting the crate type (library or executable) explicitly: // Turn on a warning #[warn(non_camel_case_types)] -# pub fn farm() {} +# fn farm() {} ~~~~ If you're compiling your crate with `rustpkg`, @@ -2790,7 +2791,9 @@ We define two crates, and use one of them as a library in the other. ~~~~ // world.rs #[link(name = "world", vers = "0.42")]; +# extern mod extra; pub fn explore() -> &'static str { "world" } +# fn main() {} ~~~~ ~~~~ {.xfail-test} diff --git a/src/libextra/sort.rs b/src/libextra/sort.rs index 2de7c1ba6dc..60c4a75104b 100644 --- a/src/libextra/sort.rs +++ b/src/libextra/sort.rs @@ -846,7 +846,7 @@ mod tests { fn check_sort(v1: &[int], v2: &[int]) { let len = v1.len(); - pub fn le(a: &int, b: &int) -> bool { *a <= *b } + fn le(a: &int, b: &int) -> bool { *a <= *b } let f = le; let v3 = merge_sort::(v1, f); let mut i = 0u; @@ -876,7 +876,7 @@ mod tests { #[test] fn test_merge_sort_mutable() { - pub fn le(a: &int, b: &int) -> bool { *a <= *b } + fn le(a: &int, b: &int) -> bool { *a <= *b } let v1 = ~[3, 2, 1]; let v2 = merge_sort(v1, le); assert_eq!(v2, ~[1, 2, 3]); diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs index d516483dec3..416dc9d7237 100644 --- a/src/librustc/middle/privacy.rs +++ b/src/librustc/middle/privacy.rs @@ -13,6 +13,7 @@ //! which are available for use externally when compiled as a library. use std::hashmap::{HashSet, HashMap}; +use std::util; use middle::resolve; use middle::ty; @@ -275,6 +276,7 @@ impl<'self> Visitor<()> for EmbargoVisitor<'self> { struct PrivacyVisitor<'self> { tcx: ty::ctxt, curitem: ast::NodeId, + in_fn: bool, // See comments on the same field in `EmbargoVisitor`. path_all_public_items: &'self ExportedItems, @@ -688,6 +690,63 @@ impl<'self> PrivacyVisitor<'self> { } } } + + /// When inside of something like a function or a method, visibility has no + /// control over anything so this forbids any mention of any visibility + fn check_all_inherited(&self, item: @ast::item) { + let tcx = self.tcx; + let check_inherited = |sp: Span, vis: ast::visibility| { + if vis != ast::inherited { + tcx.sess.span_err(sp, "visibility has no effect inside functions"); + } + }; + let check_struct = |def: &@ast::struct_def| { + for f in def.fields.iter() { + match f.node.kind { + ast::named_field(_, p) => check_inherited(f.span, p), + ast::unnamed_field => {} + } + } + }; + check_inherited(item.span, item.vis); + match item.node { + ast::item_impl(_, _, _, ref methods) => { + for m in methods.iter() { + check_inherited(m.span, m.vis); + } + } + ast::item_foreign_mod(ref fm) => { + for i in fm.items.iter() { + check_inherited(i.span, i.vis); + } + } + ast::item_enum(ref def, _) => { + for v in def.variants.iter() { + check_inherited(v.span, v.node.vis); + + match v.node.kind { + ast::struct_variant_kind(ref s) => check_struct(s), + ast::tuple_variant_kind(*) => {} + } + } + } + + ast::item_struct(ref def, _) => check_struct(def), + + ast::item_trait(_, _, ref methods) => { + for m in methods.iter() { + match *m { + ast::required(*) => {} + ast::provided(ref m) => check_inherited(m.span, m.vis), + } + } + } + + ast::item_static(*) | + ast::item_fn(*) | ast::item_mod(*) | ast::item_ty(*) | + ast::item_mac(*) => {} + } + } } impl<'self> Visitor<()> for PrivacyVisitor<'self> { @@ -699,12 +758,28 @@ impl<'self> Visitor<()> for PrivacyVisitor<'self> { } // Disallow unnecessary visibility qualifiers - self.check_sane_privacy(item); + if self.in_fn { + self.check_all_inherited(item); + } else { + self.check_sane_privacy(item); + } - let orig_curitem = self.curitem; - self.curitem = item.id; + let orig_curitem = util::replace(&mut self.curitem, item.id); + let orig_in_fn = util::replace(&mut self.in_fn, match item.node { + ast::item_mod(*) => false, // modules turn privacy back on + _ => self.in_fn, // otherwise we inherit + }); visit::walk_item(self, item, ()); self.curitem = orig_curitem; + self.in_fn = orig_in_fn; + } + + fn visit_fn(&mut self, fk: &visit::fn_kind, fd: &ast::fn_decl, + b: &ast::Block, s: Span, n: ast::NodeId, _: ()) { + // This catches both functions and methods + let orig_in_fn = util::replace(&mut self.in_fn, true); + visit::walk_fn(self, fk, fd, b, s, n, ()); + self.in_fn = orig_in_fn; } fn visit_expr(&mut self, expr: @ast::Expr, _: ()) { @@ -907,6 +982,7 @@ pub fn check_crate(tcx: ty::ctxt, { let mut visitor = PrivacyVisitor { curitem: ast::DUMMY_NODE_ID, + in_fn: false, tcx: tcx, path_all_public_items: &path_all_public_items, parents: &parents, diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index bb92ececcbf..10a02e1e8be 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -1409,7 +1409,7 @@ pub fn subst_tps(tcx: ctxt, tps: &[t], self_ty_opt: Option, typ: t) -> t { let mut subst = TpsSubst { tcx: tcx, self_ty_opt: self_ty_opt, tps: tps }; return subst.fold_ty(typ); - pub struct TpsSubst<'self> { + struct TpsSubst<'self> { tcx: ctxt, self_ty_opt: Option, tps: &'self [t], diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index b70cb59caaf..f69a0433347 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -138,7 +138,7 @@ pub fn expand_expr(extsbox: @mut SyntaxEnv, let span = e.span; - pub fn mk_expr(_: @ExtCtxt, span: Span, node: Expr_) + fn mk_expr(_: @ExtCtxt, span: Span, node: Expr_) -> @ast::Expr { @ast::Expr { id: ast::DUMMY_NODE_ID, diff --git a/src/test/compile-fail/unnecessary-private.rs b/src/test/compile-fail/unnecessary-private.rs new file mode 100644 index 00000000000..69a33922776 --- /dev/null +++ b/src/test/compile-fail/unnecessary-private.rs @@ -0,0 +1,26 @@ +// Copyright 2013 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn main() { + pub struct A; //~ ERROR: visibility has no effect + pub enum B {} //~ ERROR: visibility has no effect + pub trait C { //~ ERROR: visibility has no effect + pub fn foo() {} //~ ERROR: visibility has no effect + } + impl A { + pub fn foo() {} //~ ERROR: visibility has no effect + } + + struct D { + priv foo: int, //~ ERROR: visibility has no effect + } + pub fn foo() {} //~ ERROR: visibility has no effect + pub mod bar {} //~ ERROR: visibility has no effect +} diff --git a/src/test/run-pass/nested-class.rs b/src/test/run-pass/nested-class.rs index 6ab9856c070..927f8160f7e 100644 --- a/src/test/run-pass/nested-class.rs +++ b/src/test/run-pass/nested-class.rs @@ -14,7 +14,7 @@ pub fn main() { } impl b { - pub fn do_stuff(&self) -> int { return 37; } + fn do_stuff(&self) -> int { return 37; } } fn b(i:int) -> b {