Move some other checks to AST sanity pass

src/librustc_passes/ast_sanity.rs

@@ -21,7 +21,7 @@ use rustc::session::Session;
 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 @@ impl<'a> SanityChecker<'a> {
             );
         }
     }
+
+    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 @@ impl<'a, 'v> Visitor<'v> for SanityChecker<'a> {
 
         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,25 @@
 #![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::hir::{self, PatKind};
-use rustc::hir::intravisit::{self, Visitor};
-
 use rustc::dep_graph::DepNode;
-use rustc::lint;
+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::lint;
 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 +421,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
             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(..) => {
@@ -521,74 +508,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
     }
 }
 
-////////////////////////////////////////////////////////////////////////////////
-/// 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
@@ -629,7 +548,7 @@ impl<'a, 'tcx> ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx> {
             // .. 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 {
@@ -863,7 +782,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx>
         // 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);
     }
 
@@ -895,7 +813,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx>
     }
 
     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);
@@ -1180,10 +1097,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,