Skip to content

Commit

Permalink
Auto merge of #29726 - petrochenkov:privsan, r=alexcrichton
Browse files Browse the repository at this point in the history
- 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!
  • Loading branch information
bors committed Nov 11, 2015
2 parents 5f64201 + 41ccd44 commit ad3bd1b
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 77 deletions.
2 changes: 1 addition & 1 deletion src/doc/trpl/associated-constants.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
```
123 changes: 53 additions & 70 deletions src/librustc_privacy/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand All @@ -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(..) => {}
}
}
}
Expand Down Expand Up @@ -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(),
Expand All @@ -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
Expand Down
113 changes: 113 additions & 0 deletions src/test/compile-fail/privacy-sanity.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
6 changes: 0 additions & 6 deletions src/test/run-pass/const-block-item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down

0 comments on commit ad3bd1b

Please sign in to comment.