Auto merge of #33794 - petrochenkov:sanity, r=nrc

Add AST validation pass and move some checks to it

The purpose of this pass is to catch constructions that fit into AST data structures, but not permitted by the language. As an example, `impl`s don't have visibilities, but for convenience and uniformity with other items they are represented with a structure `Item` which has `Visibility` field.

This pass is intended to run after expansion of macros and syntax extensions (and before lowering to HIR), so it can catch erroneous constructions that were generated by them. This pass allows to remove ad hoc semantic checks from the parser, which can be overruled by syntax extensions and occasionally macros.

The checks can be put here if they are simple, local, don't require results of any complex analysis like name resolution or type checking and maybe don't logically fall into other passes. I expect most of errors generated by this pass to be non-fatal and allowing the compilation to proceed.

I intend to move some more checks to this pass later and maybe extend it with new checks, like, for example, identifier validity. Given that syntax extensions are going to be stabilized in the measurable future, it's important that they would not be able to subvert usual language rules.

In this patch I've added two new checks - a check for labels named `'static` and a check for lifetimes and labels named `'_`. The first one gives a hard error, the second one - a future compatibility warning.
Fixes #33059 ([breaking-change])
cc rust-lang/rfcs#1177

r? @nrc

src/librustc/lint/builtin.rs

@@ -204,6 +204,12 @@ declare_lint! {
     "object-unsafe non-principal fragments in object types were erroneously allowed"
 }
 
+declare_lint! {
+    pub LIFETIME_UNDERSCORE,
+    Warn,
+    "lifetimes or labels named `'_` were erroneously allowed"
+}
+
 /// Does nothing as a lint pass, but registers some `Lint`s
 /// which are used by other parts of the compiler.
 #[derive(Copy, Clone)]
@@ -242,7 +248,8 @@ impl LintPass for HardwiredLints {
             SUPER_OR_SELF_IN_GLOBAL_PATH,
             UNSIZED_IN_TUPLE,
             OBJECT_UNSAFE_FRAGMENT,
-            HR_LIFETIME_IN_ASSOC_TYPE
+            HR_LIFETIME_IN_ASSOC_TYPE,
+            LIFETIME_UNDERSCORE
         )
     }
 }

src/librustc_driver/driver.rs

@@ -38,7 +38,7 @@ use rustc_privacy;
 use rustc_plugin::registry::Registry;
 use rustc_plugin as plugin;
 use rustc::hir::lowering::lower_crate;
-use rustc_passes::{no_asm, loops, consts, rvalues, static_recursion};
+use rustc_passes::{ast_validation, no_asm, loops, consts, rvalues, static_recursion};
 use rustc_const_eval::check_match;
 use super::Compilation;
 
@@ -166,6 +166,10 @@ pub fn compile_input(sess: &Session,
              "early lint checks",
              || lint::check_ast_crate(sess, &expanded_crate));
 
+        time(sess.time_passes(),
+             "AST validation",
+             || ast_validation::check_crate(sess, &expanded_crate));
+
         let (analysis, resolutions, mut hir_forest) = {
             lower_and_resolve(sess, &id, &mut defs, &expanded_crate,
                               &sess.dep_graph, control.make_glob_map)

src/librustc_lint/lib.rs

@@ -202,6 +202,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             id: LintId::of(HR_LIFETIME_IN_ASSOC_TYPE),
             reference: "issue #33685 <https://github.com/rust-lang/rust/issues/33685>",
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(LIFETIME_UNDERSCORE),
+            reference: "RFC 1177 <https://github.com/rust-lang/rfcs/pull/1177>",
+        },
         ]);
 
     // We have one lint pass defined specially

src/librustc_passes/ast_validation.rs

@@ -0,0 +1,183 @@
+// Copyright 2016 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.
+
+// Validate AST before lowering it to HIR
+//
+// This pass is supposed to catch things that fit into AST data structures,
+// but not permitted by the language. It runs after expansion when AST is frozen,
+// so it can check for erroneous constructions produced by syntax extensions.
+// This pass is supposed to perform only simple checks not requiring name resolution
+// or type checking or some other kind of complex analysis.
+
+use rustc::lint;
+use rustc::session::Session;
+use syntax::ast::*;
+use syntax::codemap::Span;
+use syntax::errors;
+use syntax::parse::token::{self, keywords};
+use syntax::visit::{self, Visitor};
+
+struct AstValidator<'a> {
+    session: &'a Session,
+}
+
+impl<'a> AstValidator<'a> {
+    fn err_handler(&self) -> &errors::Handler {
+        &self.session.parse_sess.span_diagnostic
+    }
+
+    fn check_label(&self, label: Ident, span: Span, id: NodeId) {
+        if label.name == keywords::StaticLifetime.name() {
+            self.err_handler().span_err(span, &format!("invalid label name `{}`", label.name));
+        }
+        if label.name.as_str() == "'_" {
+            self.session.add_lint(
+                lint::builtin::LIFETIME_UNDERSCORE, id, span,
+                format!("invalid label name `{}`", label.name)
+            );
+        }
+    }
+
+    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();
+        }
+    }
+
+    fn check_path(&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)
+                );
+            }
+        }
+    }
+}
+
+impl<'a, 'v> Visitor<'v> for AstValidator<'a> {
+    fn visit_lifetime(&mut self, lt: &Lifetime) {
+        if lt.name.as_str() == "'_" {
+            self.session.add_lint(
+                lint::builtin::LIFETIME_UNDERSCORE, lt.id, lt.span,
+                format!("invalid lifetime name `{}`", lt.name)
+            );
+        }
+
+        visit::walk_lifetime(self, lt)
+    }
+
+    fn visit_expr(&mut self, expr: &Expr) {
+        match expr.node {
+            ExprKind::While(_, _, Some(ident)) | ExprKind::Loop(_, Some(ident)) |
+            ExprKind::WhileLet(_, _, _, Some(ident)) | ExprKind::ForLoop(_, _, _, Some(ident)) => {
+                self.check_label(ident, expr.span, expr.id);
+            }
+            ExprKind::Break(Some(ident)) | ExprKind::Again(Some(ident)) => {
+                self.check_label(ident.node, ident.span, expr.id);
+            }
+            _ => {}
+        }
+
+        visit::walk_expr(self, expr)
+    }
+
+    fn visit_path(&mut self, path: &Path, id: NodeId) {
+        self.check_path(path, id);
+
+        visit::walk_path(self, path)
+    }
+
+    fn visit_path_list_item(&mut self, prefix: &Path, item: &PathListItem) {
+        self.check_path(prefix, item.node.id());
+
+        visit::walk_path_list_item(self, prefix, item)
+    }
+
+    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) {
+    visit::walk_crate(&mut AstValidator { session: session }, krate)
+}

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_passes/lib.rs

@@ -37,6 +37,7 @@ extern crate rustc_const_math;
 
 pub mod diagnostics;
 
+pub mod ast_validation;
 pub mod consts;
 pub mod loops;
 pub mod no_asm;

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: