From c02c6e88e67e7c23a037f90569cc0072c37d12df Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sun, 22 May 2016 18:07:28 +0300 Subject: [PATCH] Move some other checks to AST sanity pass --- src/librustc_passes/ast_sanity.rs | 96 ++++++++++++++++- src/librustc_passes/diagnostics.rs | 40 +++++++ src/librustc_privacy/Cargo.toml | 1 - src/librustc_privacy/diagnostics.rs | 40 ------- src/librustc_privacy/lib.rs | 102 ++---------------- src/librustc_resolve/build_reduced_graph.rs | 39 +------ src/libsyntax/ast.rs | 10 ++ src/libsyntax/feature_gate.rs | 32 +----- src/test/compile-fail/issue-12560-1.rs | 2 +- src/test/compile-fail/issue-29161.rs | 2 +- .../compile-fail/priv-in-bad-locations.rs | 9 +- .../privacy/restricted/ty-params.rs | 2 + .../compile-fail/use-super-global-path.rs | 2 +- src/test/compile-fail/useless-pub.rs | 4 +- 14 files changed, 169 insertions(+), 212 deletions(-) diff --git a/src/librustc_passes/ast_sanity.rs b/src/librustc_passes/ast_sanity.rs index 22f73896f09..e22b3dfb81c 100644 --- a/src/librustc_passes/ast_sanity.rs +++ b/src/librustc_passes/ast_sanity.rs @@ -21,7 +21,7 @@ use syntax::ast::*; use syntax::codemap::Span; use syntax::errors; -use syntax::parse::token::keywords; +use syntax::parse::token::{self, keywords}; use syntax::visit::{self, Visitor}; struct SanityChecker<'a> { @@ -44,6 +44,17 @@ fn check_label(&self, label: Ident, span: Span, id: NodeId) { ); } } + + fn invalid_visibility(&self, vis: &Visibility, span: Span, note: Option<&str>) { + if vis != &Visibility::Inherited { + let mut err = struct_span_err!(self.session, span, E0449, + "unnecessary visibility qualifier"); + if let Some(note) = note { + err.span_note(span, note); + } + err.emit(); + } + } } impl<'a, 'v> Visitor<'v> for SanityChecker<'a> { @@ -72,6 +83,89 @@ fn visit_expr(&mut self, expr: &Expr) { visit::walk_expr(self, expr) } + + fn visit_path(&mut self, path: &Path, id: NodeId) { + if path.global && path.segments.len() > 0 { + let ident = path.segments[0].identifier; + if token::Ident(ident).is_path_segment_keyword() { + self.session.add_lint( + lint::builtin::SUPER_OR_SELF_IN_GLOBAL_PATH, id, path.span, + format!("global paths cannot start with `{}`", ident) + ); + } + } + + visit::walk_path(self, path) + } + + fn visit_item(&mut self, item: &Item) { + match item.node { + ItemKind::Use(ref view_path) => { + let path = view_path.node.path(); + if !path.segments.iter().all(|segment| segment.parameters.is_empty()) { + self.err_handler().span_err(path.span, "type or lifetime parameters \ + in import path"); + } + } + ItemKind::Impl(_, _, _, Some(..), _, ref impl_items) => { + self.invalid_visibility(&item.vis, item.span, None); + for impl_item in impl_items { + self.invalid_visibility(&impl_item.vis, impl_item.span, None); + } + } + ItemKind::Impl(_, _, _, None, _, _) => { + self.invalid_visibility(&item.vis, item.span, Some("place qualifiers on individual \ + impl items instead")); + } + ItemKind::DefaultImpl(..) => { + self.invalid_visibility(&item.vis, item.span, None); + } + ItemKind::ForeignMod(..) => { + self.invalid_visibility(&item.vis, item.span, Some("place qualifiers on individual \ + foreign items instead")); + } + ItemKind::Enum(ref def, _) => { + for variant in &def.variants { + for field in variant.node.data.fields() { + self.invalid_visibility(&field.vis, field.span, None); + } + } + } + _ => {} + } + + visit::walk_item(self, item) + } + + fn visit_variant_data(&mut self, vdata: &VariantData, _: Ident, + _: &Generics, _: NodeId, span: Span) { + if vdata.fields().is_empty() { + if vdata.is_tuple() { + self.err_handler().struct_span_err(span, "empty tuple structs and enum variants \ + are not allowed, use unit structs and \ + enum variants instead") + .span_help(span, "remove trailing `()` to make a unit \ + struct or unit enum variant") + .emit(); + } + } + + visit::walk_struct_def(self, vdata) + } + + fn visit_vis(&mut self, vis: &Visibility) { + match *vis { + Visibility::Restricted{ref path, ..} => { + if !path.segments.iter().all(|segment| segment.parameters.is_empty()) { + self.err_handler().span_err(path.span, "type or lifetime parameters \ + in visibility path"); + } + } + _ => {} + } + + visit::walk_vis(self, vis) + } } pub fn check_crate(session: &Session, krate: &Crate) { diff --git a/src/librustc_passes/diagnostics.rs b/src/librustc_passes/diagnostics.rs index 77f896e011b..ebbbc89e57e 100644 --- a/src/librustc_passes/diagnostics.rs +++ b/src/librustc_passes/diagnostics.rs @@ -118,6 +118,46 @@ fn some_func() { ``` "##, +E0449: r##" +A visibility qualifier was used when it was unnecessary. Erroneous code +examples: + +```compile_fail +struct Bar; + +trait Foo { + fn foo(); +} + +pub impl Bar {} // error: unnecessary visibility qualifier + +pub impl Foo for Bar { // error: unnecessary visibility qualifier + pub fn foo() {} // error: unnecessary visibility qualifier +} +``` + +To fix this error, please remove the visibility qualifier when it is not +required. Example: + +```ignore +struct Bar; + +trait Foo { + fn foo(); +} + +// Directly implemented methods share the visibility of the type itself, +// so `pub` is unnecessary here +impl Bar {} + +// Trait methods share the visibility of the trait, so `pub` is +// unnecessary in either case +pub impl Foo for Bar { + pub fn foo() {} +} +``` +"##, + } register_diagnostics! { diff --git a/src/librustc_privacy/Cargo.toml b/src/librustc_privacy/Cargo.toml index 0553e54e3aa..ac33c23f023 100644 --- a/src/librustc_privacy/Cargo.toml +++ b/src/librustc_privacy/Cargo.toml @@ -9,6 +9,5 @@ path = "lib.rs" crate-type = ["dylib"] [dependencies] -log = { path = "../liblog" } rustc = { path = "../librustc" } syntax = { path = "../libsyntax" } diff --git a/src/librustc_privacy/diagnostics.rs b/src/librustc_privacy/diagnostics.rs index 1b49409970d..bc84827ddf4 100644 --- a/src/librustc_privacy/diagnostics.rs +++ b/src/librustc_privacy/diagnostics.rs @@ -111,46 +111,6 @@ pub enum Foo { ``` "##, -E0449: r##" -A visibility qualifier was used when it was unnecessary. Erroneous code -examples: - -```compile_fail -struct Bar; - -trait Foo { - fn foo(); -} - -pub impl Bar {} // error: unnecessary visibility qualifier - -pub impl Foo for Bar { // error: unnecessary visibility qualifier - pub fn foo() {} // error: unnecessary visibility qualifier -} -``` - -To fix this error, please remove the visibility qualifier when it is not -required. Example: - -```ignore -struct Bar; - -trait Foo { - fn foo(); -} - -// Directly implemented methods share the visibility of the type itself, -// so `pub` is unnecessary here -impl Bar {} - -// Trait methods share the visibility of the trait, so `pub` is -// unnecessary in either case -pub impl Foo for Bar { - pub fn foo() {} -} -``` -"##, - E0450: r##" A tuple constructor was invoked while some of its fields are private. Erroneous code example: diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index c90d152e3c3..7e76842a9f4 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -21,37 +21,26 @@ #![feature(rustc_private)] #![feature(staged_api)] -#[macro_use] extern crate log; +extern crate rustc; #[macro_use] extern crate syntax; -#[macro_use] extern crate rustc; - -use std::cmp; -use std::mem::replace; - +use rustc::dep_graph::DepNode; use rustc::hir::{self, PatKind}; +use rustc::hir::def::{self, Def}; +use rustc::hir::def_id::DefId; use rustc::hir::intravisit::{self, Visitor}; use rustc::hir::pat_util::EnumerateAndAdjustIterator; -use rustc::dep_graph::DepNode; use rustc::lint; -use rustc::hir::def::{self, Def}; -use rustc::hir::def_id::DefId; use rustc::middle::privacy::{AccessLevel, AccessLevels}; use rustc::ty::{self, TyCtxt}; use rustc::util::nodemap::NodeSet; -use rustc::hir::map as ast_map; - use syntax::ast; use syntax::codemap::Span; -pub mod diagnostics; - -type Context<'a, 'tcx> = (&'a ty::MethodMap<'tcx>, &'a def::ExportMap); +use std::cmp; +use std::mem::replace; -/// Result of a checking operation - None => no errors were found. Some => an -/// error and contains the span and message for reporting that error and -/// optionally the same for a note about the error. -type CheckResult = Option<(Span, String, Option<(Span, String)>)>; +pub mod diagnostics; //////////////////////////////////////////////////////////////////////////////// /// The embargo visitor, used to determine the exports of the ast @@ -433,7 +422,6 @@ fn visit_expr(&mut self, expr: &hir::Expr) { hir::ExprMethodCall(..) => { let method_call = ty::MethodCall::expr(expr.id); let method = self.tcx.tables.borrow().method_map[&method_call]; - debug!("(privacy checking) checking impl method"); self.check_method(expr.span, method.def_id); } hir::ExprStruct(..) => { @@ -518,74 +506,6 @@ fn visit_foreign_item(&mut self, fi: &hir::ForeignItem) { } } -//////////////////////////////////////////////////////////////////////////////// -/// The privacy sanity check visitor, ensures unnecessary visibility isn't here -//////////////////////////////////////////////////////////////////////////////// - -struct SanePrivacyVisitor<'a, 'tcx: 'a> { - tcx: TyCtxt<'a, 'tcx, 'tcx>, -} - -impl<'a, 'tcx, 'v> Visitor<'v> for SanePrivacyVisitor<'a, 'tcx> { - fn visit_item(&mut self, item: &hir::Item) { - self.check_sane_privacy(item); - intravisit::walk_item(self, item); - } -} - -impl<'a, 'tcx> SanePrivacyVisitor<'a, 'tcx> { - /// Validate that items that shouldn't have visibility qualifiers don't have them. - /// Such qualifiers can be set by syntax extensions even if the parser doesn't allow them, - /// so we check things like variant fields too. - fn check_sane_privacy(&self, item: &hir::Item) { - let check_inherited = |sp, vis: &hir::Visibility, note: &str| { - if *vis != hir::Inherited { - let mut err = struct_span_err!(self.tcx.sess, sp, E0449, - "unnecessary visibility qualifier"); - if !note.is_empty() { - err.span_note(sp, note); - } - err.emit(); - } - }; - - match item.node { - hir::ItemImpl(_, _, _, Some(..), _, ref impl_items) => { - check_inherited(item.span, &item.vis, - "visibility qualifiers have no effect on trait impls"); - for impl_item in impl_items { - check_inherited(impl_item.span, &impl_item.vis, - "visibility qualifiers have no effect on trait impl items"); - } - } - 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"); - } - hir::ItemEnum(ref def, _) => { - for variant in &def.variants { - for field in variant.node.data.fields() { - check_inherited(field.span, &field.vis, - "visibility qualifiers have no effect on variant fields"); - } - } - } - hir::ItemStruct(..) | hir::ItemTrait(..) | - hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) | - hir::ItemMod(..) | hir::ItemExternCrate(..) | - hir::ItemUse(..) | hir::ItemTy(..) => {} - } - } -} - /////////////////////////////////////////////////////////////////////////////// /// Obsolete visitors for checking for private items in public interfaces. /// These visitors are supposed to be kept in frozen state and produce an @@ -626,7 +546,7 @@ fn path_is_private_type(&self, path_id: ast::NodeId) -> bool { // .. and it corresponds to a private type in the AST (this returns // None for type parameters) match self.tcx.map.find(node_id) { - Some(ast_map::NodeItem(ref item)) => item.vis != hir::Public, + Some(hir::map::NodeItem(ref item)) => item.vis != hir::Public, Some(_) | None => false, } } else { @@ -860,7 +780,6 @@ fn visit_item(&mut self, item: &hir::Item) { // any `visit_ty`'s will be called on things that are in // public signatures, i.e. things that we're interested in for // this visitor. - debug!("VisiblePrivateTypesVisitor entering item {:?}", item); intravisit::walk_item(self, item); } @@ -892,7 +811,6 @@ fn visit_foreign_item(&mut self, item: &hir::ForeignItem) { } fn visit_ty(&mut self, t: &hir::Ty) { - debug!("VisiblePrivateTypesVisitor checking ty {:?}", t); if let hir::TyPath(..) = t.node { if self.path_is_private_type(t.id) { self.old_error_set.insert(t.id); @@ -1177,10 +1095,6 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let krate = tcx.map.krate(); - // Sanity check to make sure that all privacy usage is reasonable. - let mut visitor = SanePrivacyVisitor { tcx: tcx }; - krate.visit_all_items(&mut visitor); - // Use the parent map to check the privacy of everything let mut visitor = PrivacyVisitor { curitem: ast::DUMMY_NODE_ID, diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index e0243bf4fa6..c7b113689fd 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -22,21 +22,20 @@ use {resolve_error, resolve_struct_error, ResolutionError}; use rustc::middle::cstore::{ChildItem, DlDef}; -use rustc::lint; use rustc::hir::def::*; use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId}; use rustc::ty::{self, VariantKind}; -use syntax::ast::{Name, NodeId}; +use syntax::ast::Name; use syntax::attr::AttrMetaMethods; -use syntax::parse::token::{self, keywords}; +use syntax::parse::token; use syntax::codemap::{Span, DUMMY_SP}; use syntax::ast::{Block, Crate, DeclKind}; use syntax::ast::{ForeignItem, ForeignItemKind, Item, ItemKind}; use syntax::ast::{Mutability, PathListItemKind}; use syntax::ast::{Stmt, StmtKind, TraitItemKind}; -use syntax::ast::{Variant, ViewPath, ViewPathGlob, ViewPathList, ViewPathSimple}; +use syntax::ast::{Variant, ViewPathGlob, ViewPathList, ViewPathSimple}; use syntax::visit::{self, Visitor}; trait ToNameBinding<'a> { @@ -95,36 +94,6 @@ fn is_item(statement: &Stmt) -> bool { block.stmts.iter().any(is_item) } - fn sanity_check_import(&self, view_path: &ViewPath, id: NodeId) { - let path = match view_path.node { - ViewPathSimple(_, ref path) | - ViewPathGlob (ref path) | - ViewPathList(ref path, _) => path - }; - - // Check for type parameters - let found_param = path.segments.iter().any(|segment| { - !segment.parameters.types().is_empty() || - !segment.parameters.lifetimes().is_empty() || - !segment.parameters.bindings().is_empty() - }); - if found_param { - self.session.span_err(path.span, "type or lifetime parameters in import path"); - } - - // Checking for special identifiers in path - // prevent `self` or `super` at beginning of global path - if path.global && path.segments.len() > 0 { - let first = path.segments[0].identifier.name; - if first == keywords::Super.name() || first == keywords::SelfValue.name() { - self.session.add_lint( - lint::builtin::SUPER_OR_SELF_IN_GLOBAL_PATH, id, path.span, - format!("expected identifier, found keyword `{}`", first) - ); - } - } - } - /// Constructs the reduced graph for one item. fn build_reduced_graph_for_item(&mut self, item: &Item, parent_ref: &mut Module<'b>) { let parent = *parent_ref; @@ -158,8 +127,6 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent_ref: &mut Module< } }; - self.sanity_check_import(view_path, item.id); - // Build up the import directives. let is_prelude = item.attrs.iter().any(|attr| attr.name() == "prelude_import"); diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index c8ded115db8..cd735be9fdf 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -1907,6 +1907,16 @@ pub enum ViewPath_ { ViewPathList(Path, Vec) } +impl ViewPath_ { + pub fn path(&self) -> &Path { + match *self { + ViewPathSimple(_, ref path) | + ViewPathGlob (ref path) | + ViewPathList(ref path, _) => path + } + } +} + /// Meta-data associated with an item pub type Attribute = Spanned; diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 5687099b27c..ed9c49445a6 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -952,22 +952,6 @@ fn visit_item(&mut self, i: &ast::Item) { visit::walk_item(self, i); } - fn visit_variant_data(&mut self, s: &'v ast::VariantData, _: ast::Ident, - _: &'v ast::Generics, _: ast::NodeId, span: Span) { - if s.fields().is_empty() { - if s.is_tuple() { - self.context.span_handler.struct_span_err(span, "empty tuple structs and enum \ - variants are not allowed, use \ - unit structs and enum variants \ - instead") - .span_help(span, "remove trailing `()` to make a unit \ - struct or unit enum variant") - .emit(); - } - } - visit::walk_struct_def(self, s) - } - fn visit_foreign_item(&mut self, i: &ast::ForeignItem) { let links_to_llvm = match attr::first_attr_value_str_by_name(&i.attrs, "link_name") { @@ -1138,22 +1122,12 @@ fn visit_impl_item(&mut self, ii: &'v ast::ImplItem) { fn visit_vis(&mut self, vis: &'v ast::Visibility) { let span = match *vis { ast::Visibility::Crate(span) => span, - ast::Visibility::Restricted { ref path, .. } => { - // Check for type parameters - let found_param = path.segments.iter().any(|segment| { - !segment.parameters.types().is_empty() || - !segment.parameters.lifetimes().is_empty() || - !segment.parameters.bindings().is_empty() - }); - if found_param { - self.context.span_handler.span_err(path.span, "type or lifetime parameters \ - in visibility path"); - } - path.span - } + ast::Visibility::Restricted { ref path, .. } => path.span, _ => return, }; gate_feature_post!(&self, pub_restricted, span, "`pub(restricted)` syntax is experimental"); + + visit::walk_vis(self, vis) } } diff --git a/src/test/compile-fail/issue-12560-1.rs b/src/test/compile-fail/issue-12560-1.rs index e005de01649..80f551ebd1f 100644 --- a/src/test/compile-fail/issue-12560-1.rs +++ b/src/test/compile-fail/issue-12560-1.rs @@ -21,5 +21,5 @@ enum Foo { } fn main() { - println!("{}", match Bar { Bar => 1, Baz => 2, Bazar => 3 }) + println!("{}", match Bar { Bar => 1, Baz => 2, Bazar => 3 }) //~ ERROR unresolved name `Bar` } diff --git a/src/test/compile-fail/issue-29161.rs b/src/test/compile-fail/issue-29161.rs index f7453c45be6..bc09f61a754 100644 --- a/src/test/compile-fail/issue-29161.rs +++ b/src/test/compile-fail/issue-29161.rs @@ -12,7 +12,7 @@ mod a { struct A; impl Default for A { - pub fn default() -> A { + pub fn default() -> A { //~ ERROR unnecessary visibility qualifier A; } } diff --git a/src/test/compile-fail/priv-in-bad-locations.rs b/src/test/compile-fail/priv-in-bad-locations.rs index 43d112b8aa0..8901d8c08e5 100644 --- a/src/test/compile-fail/priv-in-bad-locations.rs +++ b/src/test/compile-fail/priv-in-bad-locations.rs @@ -8,8 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -pub extern { - //~^ ERROR unnecessary visibility +pub extern { //~ ERROR unnecessary visibility qualifier pub fn bar(); } @@ -19,10 +18,10 @@ fn foo(&self) {} struct B; -pub impl B {} //~ ERROR: unnecessary visibility +pub impl B {} //~ ERROR unnecessary visibility qualifier -pub impl A for B { //~ ERROR: unnecessary visibility - pub fn foo(&self) {} //~ ERROR: unnecessary visibility +pub impl A for B { //~ ERROR unnecessary visibility qualifier + pub fn foo(&self) {} //~ ERROR unnecessary visibility qualifier } pub fn main() {} diff --git a/src/test/compile-fail/privacy/restricted/ty-params.rs b/src/test/compile-fail/privacy/restricted/ty-params.rs index ab423620d68..ae60c4366ee 100644 --- a/src/test/compile-fail/privacy/restricted/ty-params.rs +++ b/src/test/compile-fail/privacy/restricted/ty-params.rs @@ -16,9 +16,11 @@ struct S(T); m!{ S } //~ ERROR type or lifetime parameters in visibility path +//~^ ERROR failed to resolve module path. Not a module `S` mod foo { struct S(pub(foo) ()); //~ ERROR type or lifetime parameters in visibility path + //~^ ERROR type name `T` is undefined or not in scope } fn main() {} diff --git a/src/test/compile-fail/use-super-global-path.rs b/src/test/compile-fail/use-super-global-path.rs index d721d428f29..c4d18a94eb2 100644 --- a/src/test/compile-fail/use-super-global-path.rs +++ b/src/test/compile-fail/use-super-global-path.rs @@ -12,7 +12,7 @@ mod foo { pub fn g() { - use ::super::main; //~ WARN expected identifier, found keyword `super` + use ::super::main; //~ WARN global paths cannot start with `super` //~^ WARN this was previously accepted by the compiler but is being phased out main(); } diff --git a/src/test/compile-fail/useless-pub.rs b/src/test/compile-fail/useless-pub.rs index 268b937c291..064062df753 100644 --- a/src/test/compile-fail/useless-pub.rs +++ b/src/test/compile-fail/useless-pub.rs @@ -15,14 +15,12 @@ pub trait E { } impl E for A { - pub fn foo(&self) {} //~ ERROR: unnecessary visibility + pub fn foo(&self) {} //~ ERROR: unnecessary visibility qualifier } enum Foo { V1 { pub f: i32 }, //~ ERROR unnecessary visibility qualifier - //| NOTE visibility qualifiers have no effect on variant fields V2(pub i32), //~ ERROR unnecessary visibility qualifier - //| NOTE visibility qualifiers have no effect on variant fields } fn main() {} -- GitLab