Move some other checks to AST sanity pass

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) {

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! {

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" }

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:

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,

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");
 

src/libsyntax/ast.rs

@@ -1907,6 +1907,16 @@ pub enum ViewPath_ {
     ViewPathList(Path, Vec<PathListItem>)
 }
 
+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<Attribute_>;
 

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)
     }
 }
 

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`
 }

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;
         }
     }

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() {}

src/test/compile-fail/privacy/restricted/ty-params.rs

@@ -16,9 +16,11 @@
 
 struct S<T>(T);
 m!{ S<u8> } //~ ERROR type or lifetime parameters in visibility path
+//~^ ERROR failed to resolve module path. Not a module `S`
 
 mod foo {
     struct S(pub(foo<T>) ()); //~ ERROR type or lifetime parameters in visibility path
+    //~^ ERROR type name `T` is undefined or not in scope
 }
 
 fn main() {}

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();
     }

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() {}