Forbid privacy in inner functions

Closes #10111

10 files changed, 121 insertions, 13 deletions

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::<int>(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<t>, 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<t>,
         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 <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.
+
+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 {