From ef6850643ee934e4404b67ea9697a60d80053eb2 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 9 Nov 2015 21:15:53 +0300 Subject: [PATCH 1/2] rustc_privacy: Fix bugs in SanePrivacyVisitor --- src/doc/trpl/associated-constants.md | 2 +- src/librustc_privacy/lib.rs | 117 ++++++++++-------------- src/test/compile-fail/privacy-sanity.rs | 113 +++++++++++++++++++++++ src/test/run-pass/const-block-item.rs | 6 -- 4 files changed, 161 insertions(+), 77 deletions(-) create mode 100644 src/test/compile-fail/privacy-sanity.rs diff --git a/src/doc/trpl/associated-constants.md b/src/doc/trpl/associated-constants.md index 7d51dd0aa5701..81f7ea104e48f 100644 --- a/src/doc/trpl/associated-constants.md +++ b/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; } ``` diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 491c8a2c4524a..a576c41f4316f 100644 --- a/src/librustc_privacy/lib.rs +++ b/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,69 @@ 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(_) => {} + _ => {} } } /// 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()); + } + } + _ => {} } } } @@ -1492,6 +1469,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 +1494,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 diff --git a/src/test/compile-fail/privacy-sanity.rs b/src/test/compile-fail/privacy-sanity.rs new file mode 100644 index 0000000000000..336913b877271 --- /dev/null +++ b/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 or the MIT license +// , 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 + } +} diff --git a/src/test/run-pass/const-block-item.rs b/src/test/run-pass/const-block-item.rs index b616b1f610325..51ebc240e7288 100644 --- a/src/test/run-pass/const-block-item.rs +++ b/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); From 41ccd44f767ec5f87f0c90d5f8d48c33bb9bddff Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 10 Nov 2015 12:51:56 +0300 Subject: [PATCH 2/2] Use exhaustive matching --- src/librustc_privacy/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index a576c41f4316f..a8600d91a2689 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -1094,7 +1094,10 @@ impl<'a, 'tcx> SanePrivacyVisitor<'a, 'tcx> { check_inherited(item.span, item.vis, "place qualifiers on individual functions instead"); } - _ => {} + hir::ItemStruct(..) | hir::ItemEnum(..) | hir::ItemTrait(..) | + hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) | + hir::ItemMod(..) | hir::ItemExternCrate(..) | + hir::ItemUse(..) | hir::ItemTy(..) => {} } } @@ -1125,7 +1128,10 @@ impl<'a, 'tcx> SanePrivacyVisitor<'a, 'tcx> { 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(..) => {} } } }