Skip to content

Commit c2cab1f

Browse files
committed
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
2 parents 806a553 + 731144b commit c2cab1f

File tree

21 files changed

+312
-214
lines changed

21 files changed

+312
-214
lines changed

src/librustc/lint/builtin.rs

+8-1
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,12 @@ declare_lint! {
204204
"object-unsafe non-principal fragments in object types were erroneously allowed"
205205
}
206206

207+
declare_lint! {
208+
pub LIFETIME_UNDERSCORE,
209+
Warn,
210+
"lifetimes or labels named `'_` were erroneously allowed"
211+
}
212+
207213
/// Does nothing as a lint pass, but registers some `Lint`s
208214
/// which are used by other parts of the compiler.
209215
#[derive(Copy, Clone)]
@@ -242,7 +248,8 @@ impl LintPass for HardwiredLints {
242248
SUPER_OR_SELF_IN_GLOBAL_PATH,
243249
UNSIZED_IN_TUPLE,
244250
OBJECT_UNSAFE_FRAGMENT,
245-
HR_LIFETIME_IN_ASSOC_TYPE
251+
HR_LIFETIME_IN_ASSOC_TYPE,
252+
LIFETIME_UNDERSCORE
246253
)
247254
}
248255
}

src/librustc_driver/driver.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ use rustc_privacy;
3838
use rustc_plugin::registry::Registry;
3939
use rustc_plugin as plugin;
4040
use rustc::hir::lowering::lower_crate;
41-
use rustc_passes::{no_asm, loops, consts, rvalues, static_recursion};
41+
use rustc_passes::{ast_validation, no_asm, loops, consts, rvalues, static_recursion};
4242
use rustc_const_eval::check_match;
4343
use super::Compilation;
4444

@@ -165,6 +165,10 @@ pub fn compile_input(sess: &Session,
165165
"early lint checks",
166166
|| lint::check_ast_crate(sess, &expanded_crate));
167167

168+
time(sess.time_passes(),
169+
"AST validation",
170+
|| ast_validation::check_crate(sess, &expanded_crate));
171+
168172
let (analysis, resolutions, mut hir_forest) = {
169173
lower_and_resolve(sess, &id, &mut defs, &expanded_crate,
170174
&sess.dep_graph, control.make_glob_map)

src/librustc_lint/lib.rs

+4
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
202202
id: LintId::of(HR_LIFETIME_IN_ASSOC_TYPE),
203203
reference: "issue #33685 <https://github.com/rust-lang/rust/issues/33685>",
204204
},
205+
FutureIncompatibleInfo {
206+
id: LintId::of(LIFETIME_UNDERSCORE),
207+
reference: "RFC 1177 <https://github.com/rust-lang/rfcs/pull/1177>",
208+
},
205209
]);
206210

207211
// We have one lint pass defined specially

src/librustc_passes/ast_validation.rs

+171
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// Validate AST before lowering it to HIR
12+
//
13+
// This pass is supposed to catch things that fit into AST data structures,
14+
// but not permitted by the language. It runs after expansion when AST is frozen,
15+
// so it can check for erroneous constructions produced by syntax extensions.
16+
// This pass is supposed to perform only simple checks not requiring name resolution
17+
// or type checking or some other kind of complex analysis.
18+
19+
use rustc::lint;
20+
use rustc::session::Session;
21+
use syntax::ast::*;
22+
use syntax::codemap::Span;
23+
use syntax::errors;
24+
use syntax::parse::token::{self, keywords};
25+
use syntax::visit::{self, Visitor};
26+
27+
struct AstValidator<'a> {
28+
session: &'a Session,
29+
}
30+
31+
impl<'a> AstValidator<'a> {
32+
fn err_handler(&self) -> &errors::Handler {
33+
&self.session.parse_sess.span_diagnostic
34+
}
35+
36+
fn check_label(&self, label: Ident, span: Span, id: NodeId) {
37+
if label.name == keywords::StaticLifetime.name() {
38+
self.err_handler().span_err(span, &format!("invalid label name `{}`", label.name));
39+
}
40+
if label.name.as_str() == "'_" {
41+
self.session.add_lint(
42+
lint::builtin::LIFETIME_UNDERSCORE, id, span,
43+
format!("invalid label name `{}`", label.name)
44+
);
45+
}
46+
}
47+
48+
fn invalid_visibility(&self, vis: &Visibility, span: Span, note: Option<&str>) {
49+
if vis != &Visibility::Inherited {
50+
let mut err = struct_span_err!(self.session, span, E0449,
51+
"unnecessary visibility qualifier");
52+
if let Some(note) = note {
53+
err.span_note(span, note);
54+
}
55+
err.emit();
56+
}
57+
}
58+
}
59+
60+
impl<'a, 'v> Visitor<'v> for AstValidator<'a> {
61+
fn visit_lifetime(&mut self, lt: &Lifetime) {
62+
if lt.name.as_str() == "'_" {
63+
self.session.add_lint(
64+
lint::builtin::LIFETIME_UNDERSCORE, lt.id, lt.span,
65+
format!("invalid lifetime name `{}`", lt.name)
66+
);
67+
}
68+
69+
visit::walk_lifetime(self, lt)
70+
}
71+
72+
fn visit_expr(&mut self, expr: &Expr) {
73+
match expr.node {
74+
ExprKind::While(_, _, Some(ident)) | ExprKind::Loop(_, Some(ident)) |
75+
ExprKind::WhileLet(_, _, _, Some(ident)) | ExprKind::ForLoop(_, _, _, Some(ident)) |
76+
ExprKind::Break(Some(ident)) | ExprKind::Again(Some(ident)) => {
77+
self.check_label(ident.node, ident.span, expr.id);
78+
}
79+
_ => {}
80+
}
81+
82+
visit::walk_expr(self, expr)
83+
}
84+
85+
fn visit_path(&mut self, path: &Path, id: NodeId) {
86+
if path.global && path.segments.len() > 0 {
87+
let ident = path.segments[0].identifier;
88+
if token::Ident(ident).is_path_segment_keyword() {
89+
self.session.add_lint(
90+
lint::builtin::SUPER_OR_SELF_IN_GLOBAL_PATH, id, path.span,
91+
format!("global paths cannot start with `{}`", ident)
92+
);
93+
}
94+
}
95+
96+
visit::walk_path(self, path)
97+
}
98+
99+
fn visit_item(&mut self, item: &Item) {
100+
match item.node {
101+
ItemKind::Use(ref view_path) => {
102+
let path = view_path.node.path();
103+
if !path.segments.iter().all(|segment| segment.parameters.is_empty()) {
104+
self.err_handler().span_err(path.span, "type or lifetime parameters \
105+
in import path");
106+
}
107+
}
108+
ItemKind::Impl(_, _, _, Some(..), _, ref impl_items) => {
109+
self.invalid_visibility(&item.vis, item.span, None);
110+
for impl_item in impl_items {
111+
self.invalid_visibility(&impl_item.vis, impl_item.span, None);
112+
}
113+
}
114+
ItemKind::Impl(_, _, _, None, _, _) => {
115+
self.invalid_visibility(&item.vis, item.span, Some("place qualifiers on individual \
116+
impl items instead"));
117+
}
118+
ItemKind::DefaultImpl(..) => {
119+
self.invalid_visibility(&item.vis, item.span, None);
120+
}
121+
ItemKind::ForeignMod(..) => {
122+
self.invalid_visibility(&item.vis, item.span, Some("place qualifiers on individual \
123+
foreign items instead"));
124+
}
125+
ItemKind::Enum(ref def, _) => {
126+
for variant in &def.variants {
127+
for field in variant.node.data.fields() {
128+
self.invalid_visibility(&field.vis, field.span, None);
129+
}
130+
}
131+
}
132+
_ => {}
133+
}
134+
135+
visit::walk_item(self, item)
136+
}
137+
138+
fn visit_variant_data(&mut self, vdata: &VariantData, _: Ident,
139+
_: &Generics, _: NodeId, span: Span) {
140+
if vdata.fields().is_empty() {
141+
if vdata.is_tuple() {
142+
self.err_handler().struct_span_err(span, "empty tuple structs and enum variants \
143+
are not allowed, use unit structs and \
144+
enum variants instead")
145+
.span_help(span, "remove trailing `()` to make a unit \
146+
struct or unit enum variant")
147+
.emit();
148+
}
149+
}
150+
151+
visit::walk_struct_def(self, vdata)
152+
}
153+
154+
fn visit_vis(&mut self, vis: &Visibility) {
155+
match *vis {
156+
Visibility::Restricted{ref path, ..} => {
157+
if !path.segments.iter().all(|segment| segment.parameters.is_empty()) {
158+
self.err_handler().span_err(path.span, "type or lifetime parameters \
159+
in visibility path");
160+
}
161+
}
162+
_ => {}
163+
}
164+
165+
visit::walk_vis(self, vis)
166+
}
167+
}
168+
169+
pub fn check_crate(session: &Session, krate: &Crate) {
170+
visit::walk_crate(&mut AstValidator { session: session }, krate)
171+
}

src/librustc_passes/diagnostics.rs

+40
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,46 @@ fn some_func() {
143143
```
144144
"##,
145145

146+
E0449: r##"
147+
A visibility qualifier was used when it was unnecessary. Erroneous code
148+
examples:
149+
150+
```compile_fail
151+
struct Bar;
152+
153+
trait Foo {
154+
fn foo();
155+
}
156+
157+
pub impl Bar {} // error: unnecessary visibility qualifier
158+
159+
pub impl Foo for Bar { // error: unnecessary visibility qualifier
160+
pub fn foo() {} // error: unnecessary visibility qualifier
161+
}
162+
```
163+
164+
To fix this error, please remove the visibility qualifier when it is not
165+
required. Example:
166+
167+
```ignore
168+
struct Bar;
169+
170+
trait Foo {
171+
fn foo();
172+
}
173+
174+
// Directly implemented methods share the visibility of the type itself,
175+
// so `pub` is unnecessary here
176+
impl Bar {}
177+
178+
// Trait methods share the visibility of the trait, so `pub` is
179+
// unnecessary in either case
180+
pub impl Foo for Bar {
181+
pub fn foo() {}
182+
}
183+
```
184+
"##,
185+
146186
}
147187

148188
register_diagnostics! {

src/librustc_passes/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ extern crate rustc_const_math;
3737

3838
pub mod diagnostics;
3939

40+
pub mod ast_validation;
4041
pub mod consts;
4142
pub mod loops;
4243
pub mod no_asm;

src/librustc_privacy/Cargo.toml

-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,5 @@ path = "lib.rs"
99
crate-type = ["dylib"]
1010

1111
[dependencies]
12-
log = { path = "../liblog" }
1312
rustc = { path = "../librustc" }
1413
syntax = { path = "../libsyntax" }

src/librustc_privacy/diagnostics.rs

-40
Original file line numberDiff line numberDiff line change
@@ -115,46 +115,6 @@ pub enum Foo {
115115
```
116116
"##,
117117

118-
E0449: r##"
119-
A visibility qualifier was used when it was unnecessary. Erroneous code
120-
examples:
121-
122-
```compile_fail
123-
struct Bar;
124-
125-
trait Foo {
126-
fn foo();
127-
}
128-
129-
pub impl Bar {} // error: unnecessary visibility qualifier
130-
131-
pub impl Foo for Bar { // error: unnecessary visibility qualifier
132-
pub fn foo() {} // error: unnecessary visibility qualifier
133-
}
134-
```
135-
136-
To fix this error, please remove the visibility qualifier when it is not
137-
required. Example:
138-
139-
```ignore
140-
struct Bar;
141-
142-
trait Foo {
143-
fn foo();
144-
}
145-
146-
// Directly implemented methods share the visibility of the type itself,
147-
// so `pub` is unnecessary here
148-
impl Bar {}
149-
150-
// Trait methods share the visibility of the trait, so `pub` is
151-
// unnecessary in either case
152-
pub impl Foo for Bar {
153-
pub fn foo() {}
154-
}
155-
```
156-
"##,
157-
158118
E0450: r##"
159119
A tuple constructor was invoked while some of its fields are private. Erroneous
160120
code example:

0 commit comments

Comments
 (0)