Auto merge of #29726 - petrochenkov:privsan, r=alexcrichton

- Check privacy sanity in all blocks, not only function bodies
- Check all fields, not only named
- Check all impl items, not only methods
- Check default impls
- Move the sanity check in the beginning of privacy checking, so others could rely on it

Technically it's a [breaking-change], but I expect no breakage because, well, it's *sane* privacy visitor, if code is broken it must be insane by definition!

Author: bors

src/doc/trpl/associated-constants.md

@@ -74,6 +74,6 @@ for a `struct` or an `enum` works fine too:
 struct Foo;
 
 impl Foo {
-    pub const FOO: u32 = 3;
+    const FOO: u32 = 3;
 }
 ```

src/librustc_privacy/lib.rs

@@ -1031,32 +1031,29 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
 
 struct SanePrivacyVisitor<'a, 'tcx: 'a> {
     tcx: &'a ty::ctxt<'tcx>,
-    in_fn: bool,
+    in_block: bool,
 }
 
 impl<'a, 'tcx, 'v> Visitor<'v> for SanePrivacyVisitor<'a, 'tcx> {
     fn visit_item(&mut self, item: &hir::Item) {
-        if self.in_fn {
+        self.check_sane_privacy(item);
+        if self.in_block {
             self.check_all_inherited(item);
-        } else {
-            self.check_sane_privacy(item);
         }
 
-        let in_fn = self.in_fn;
-        let orig_in_fn = replace(&mut self.in_fn, match item.node {
-            hir::ItemMod(..) => false, // modules turn privacy back on
-            _ => in_fn,           // otherwise we inherit
-        });
+        let orig_in_block = self.in_block;
+
+        // Modules turn privacy back on, otherwise we inherit
+        self.in_block = if let hir::ItemMod(..) = item.node { false } else { orig_in_block };
+
         visit::walk_item(self, item);
-        self.in_fn = orig_in_fn;
+        self.in_block = orig_in_block;
     }
 
-    fn visit_fn(&mut self, fk: visit::FnKind<'v>, fd: &'v hir::FnDecl,
-                b: &'v hir::Block, s: Span, _: ast::NodeId) {
-        // This catches both functions and methods
-        let orig_in_fn = replace(&mut self.in_fn, true);
-        visit::walk_fn(self, fk, fd, b, s);
-        self.in_fn = orig_in_fn;
+    fn visit_block(&mut self, b: &'v hir::Block) {
+        let orig_in_block = replace(&mut self.in_block, true);
+        visit::walk_block(self, b);
+        self.in_block = orig_in_block;
     }
 }
 
@@ -1066,89 +1063,75 @@ impl<'a, 'tcx> SanePrivacyVisitor<'a, 'tcx> {
     /// anything. In theory these qualifiers wouldn't parse, but that may happen
     /// later on down the road...
     fn check_sane_privacy(&self, item: &hir::Item) {
-        let tcx = self.tcx;
-        let check_inherited = |sp: Span, vis: hir::Visibility, note: &str| {
+        let check_inherited = |sp, vis, note: &str| {
             if vis != hir::Inherited {
-                span_err!(tcx.sess, sp, E0449,
-                          "unnecessary visibility qualifier");
+                span_err!(self.tcx.sess, sp, E0449, "unnecessary visibility qualifier");
                 if !note.is_empty() {
-                    tcx.sess.span_note(sp, note);
+                    self.tcx.sess.span_note(sp, note);
                 }
             }
         };
+
         match item.node {
             // implementations of traits don't need visibility qualifiers because
             // that's controlled by having the trait in scope.
             hir::ItemImpl(_, _, _, Some(..), _, ref impl_items) => {
                 check_inherited(item.span, item.vis,
-                                "visibility qualifiers have no effect on trait \
-                                 impls");
+                                "visibility qualifiers have no effect on trait impls");
                 for impl_item in impl_items {
                     check_inherited(impl_item.span, impl_item.vis, "");
                 }
             }
-
-            hir::ItemImpl(..) => {
+            hir::ItemImpl(_, _, _, None, _, _) => {
                 check_inherited(item.span, item.vis,
                                 "place qualifiers on individual methods instead");
             }
+            hir::ItemDefaultImpl(..) => {
+                check_inherited(item.span, item.vis,
+                                "visibility qualifiers have no effect on trait impls");
+            }
             hir::ItemForeignMod(..) => {
                 check_inherited(item.span, item.vis,
-                                "place qualifiers on individual functions \
-                                 instead");
+                                "place qualifiers on individual functions instead");
             }
-
-            hir::ItemEnum(..) |
-            hir::ItemTrait(..) | hir::ItemDefaultImpl(..) |
-            hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemStruct(..) |
-            hir::ItemFn(..) | hir::ItemMod(..) | hir::ItemTy(..) |
-            hir::ItemExternCrate(_) | hir::ItemUse(_) => {}
+            hir::ItemStruct(..) | hir::ItemEnum(..) | hir::ItemTrait(..) |
+            hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
+            hir::ItemMod(..) | hir::ItemExternCrate(..) |
+            hir::ItemUse(..) | hir::ItemTy(..) => {}
         }
     }
 
     /// 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: &hir::Item) {
-        let tcx = self.tcx;
-        fn check_inherited(tcx: &ty::ctxt, sp: Span, vis: hir::Visibility) {
+        let check_inherited = |sp, vis| {
             if vis != hir::Inherited {
-                span_err!(tcx.sess, sp, E0447,
-                          "visibility has no effect inside functions");
-            }
-        }
-        let check_struct = |def: &hir::VariantData| {
-            for f in def.fields() {
-               match f.node.kind {
-                    hir::NamedField(_, p) => check_inherited(tcx, f.span, p),
-                    hir::UnnamedField(..) => {}
-                }
+                span_err!(self.tcx.sess, sp, E0447,
+                          "visibility has no effect inside functions or block expressions");
             }
         };
-        check_inherited(tcx, item.span, item.vis);
+
+        check_inherited(item.span, item.vis);
         match item.node {
             hir::ItemImpl(_, _, _, _, _, ref impl_items) => {
                 for impl_item in impl_items {
-                    match impl_item.node {
-                        hir::MethodImplItem(..) => {
-                            check_inherited(tcx, impl_item.span, impl_item.vis);
-                        }
-                        _ => {}
-                    }
+                    check_inherited(impl_item.span, impl_item.vis);
                 }
             }
             hir::ItemForeignMod(ref fm) => {
-                for i in &fm.items {
-                    check_inherited(tcx, i.span, i.vis);
+                for fi in &fm.items {
+                    check_inherited(fi.span, fi.vis);
                 }
             }
-
-            hir::ItemStruct(ref def, _) => check_struct(def),
-
-            hir::ItemEnum(..) |
-            hir::ItemExternCrate(_) | hir::ItemUse(_) |
-            hir::ItemTrait(..) | hir::ItemDefaultImpl(..) |
-            hir::ItemStatic(..) | hir::ItemConst(..) |
-            hir::ItemFn(..) | hir::ItemMod(..) | hir::ItemTy(..) => {}
+            hir::ItemStruct(ref vdata, _) => {
+                for f in vdata.fields() {
+                    check_inherited(f.span, f.node.kind.visibility());
+                }
+            }
+            hir::ItemDefaultImpl(..) | hir::ItemEnum(..) | hir::ItemTrait(..) |
+            hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
+            hir::ItemMod(..) | hir::ItemExternCrate(..) |
+            hir::ItemUse(..) | hir::ItemTy(..) => {}
         }
     }
 }
@@ -1492,6 +1475,14 @@ pub fn check_crate(tcx: &ty::ctxt,
                    -> (ExportedItems, PublicItems) {
     let krate = tcx.map.krate();
 
+    // Sanity check to make sure that all privacy usage and controls are
+    // reasonable.
+    let mut visitor = SanePrivacyVisitor {
+        tcx: tcx,
+        in_block: false,
+    };
+    visit::walk_crate(&mut visitor, krate);
+
     // Figure out who everyone's parent is
     let mut visitor = ParentVisitor {
         parents: NodeMap(),
@@ -1509,14 +1500,6 @@ pub fn check_crate(tcx: &ty::ctxt,
     };
     visit::walk_crate(&mut visitor, krate);
 
-    // Sanity check to make sure that all privacy usage and controls are
-    // reasonable.
-    let mut visitor = SanePrivacyVisitor {
-        in_fn: false,
-        tcx: tcx,
-    };
-    visit::walk_crate(&mut visitor, krate);
-
     tcx.sess.abort_if_errors();
 
     // Build up a set of all exported items in the AST. This is a set of all

src/test/compile-fail/privacy-sanity.rs

@@ -0,0 +1,113 @@
+// Copyright 2015 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(associated_consts)]
+#![feature(optin_builtin_traits)]
+
+trait MarkerTr {}
+pub trait Tr {
+    fn f();
+    const C: u8;
+    type T;
+}
+pub struct S {
+    pub a: u8
+}
+struct Ts(pub u8);
+
+pub impl MarkerTr for .. {} //~ ERROR unnecessary visibility qualifier
+pub impl Tr for S {  //~ ERROR unnecessary visibility qualifier
+    pub fn f() {} //~ ERROR unnecessary visibility qualifier
+    pub const C: u8 = 0; //~ ERROR unnecessary visibility qualifier
+    pub type T = u8; //~ ERROR unnecessary visibility qualifier
+}
+pub impl S { //~ ERROR unnecessary visibility qualifier
+    pub fn f() {}
+    pub const C: u8 = 0;
+    // pub type T = u8;
+}
+pub extern "C" { //~ ERROR unnecessary visibility qualifier
+    pub fn f();
+    pub static St: u8;
+}
+
+const MAIN: u8 = {
+    trait MarkerTr {}
+    pub trait Tr { //~ ERROR visibility has no effect inside functions or block
+        fn f();
+        const C: u8;
+        type T;
+    }
+    pub struct S { //~ ERROR visibility has no effect inside functions or block
+        pub a: u8 //~ ERROR visibility has no effect inside functions or block
+    }
+    struct Ts(pub u8); //~ ERROR visibility has no effect inside functions or block
+
+    pub impl MarkerTr for .. {} //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+    pub impl Tr for S {  //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+        pub fn f() {} //~ ERROR unnecessary visibility qualifier
+        //~^ ERROR visibility has no effect inside functions or block
+        pub const C: u8 = 0; //~ ERROR unnecessary visibility qualifier
+        //~^ ERROR visibility has no effect inside functions or block
+        pub type T = u8; //~ ERROR unnecessary visibility qualifier
+        //~^ ERROR visibility has no effect inside functions or block
+    }
+    pub impl S { //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+        pub fn f() {} //~ ERROR visibility has no effect inside functions or block
+        pub const C: u8 = 0; //~ ERROR visibility has no effect inside functions or block
+        // pub type T = u8; // ERROR visibility has no effect inside functions or block
+    }
+    pub extern "C" { //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+        pub fn f(); //~ ERROR visibility has no effect inside functions or block
+        pub static St: u8; //~ ERROR visibility has no effect inside functions or block
+    }
+
+    0
+};
+
+fn main() {
+    trait MarkerTr {}
+    pub trait Tr { //~ ERROR visibility has no effect inside functions or block
+        fn f();
+        const C: u8;
+        type T;
+    }
+    pub struct S { //~ ERROR visibility has no effect inside functions or block
+        pub a: u8 //~ ERROR visibility has no effect inside functions or block
+    }
+    struct Ts(pub u8); //~ ERROR visibility has no effect inside functions or block
+
+    pub impl MarkerTr for .. {} //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+    pub impl Tr for S {  //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+        pub fn f() {} //~ ERROR unnecessary visibility qualifier
+        //~^ ERROR visibility has no effect inside functions or block
+        pub const C: u8 = 0; //~ ERROR unnecessary visibility qualifier
+        //~^ ERROR visibility has no effect inside functions or block
+        pub type T = u8; //~ ERROR unnecessary visibility qualifier
+        //~^ ERROR visibility has no effect inside functions or block
+    }
+    pub impl S { //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+        pub fn f() {} //~ ERROR visibility has no effect inside functions or block
+        pub const C: u8 = 0; //~ ERROR visibility has no effect inside functions or block
+        // pub type T = u8; // ERROR visibility has no effect inside functions or block
+    }
+    pub extern "C" { //~ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility has no effect inside functions or block
+        pub fn f(); //~ ERROR visibility has no effect inside functions or block
+        pub static St: u8; //~ ERROR visibility has no effect inside functions or block
+    }
+}

src/test/run-pass/const-block-item.rs

@@ -20,11 +20,6 @@ static BLOCK_USE: usize = {
     100
 };
 
-static BLOCK_PUB_USE: usize = {
-    pub use foo::Value;
-    200
-};
-
 static BLOCK_STRUCT_DEF: usize = {
     struct Foo {
         a: usize
@@ -48,7 +43,6 @@ static BLOCK_MACRO_RULES: usize = {
 
 pub fn main() {
     assert_eq!(BLOCK_USE, 100);
-    assert_eq!(BLOCK_PUB_USE, 200);
     assert_eq!(BLOCK_STRUCT_DEF, 300);
     assert_eq!(BLOCK_FN_DEF(390), 400);
     assert_eq!(BLOCK_MACRO_RULES, 412);