Skip to content

Commit c02c6e8

Browse files
petrochenkovpetrochenkov
committed
Move some other checks to AST sanity pass
1 parent 2abdf96 commit c02c6e8

File tree

14 files changed

+169
-212
lines changed

14 files changed

+169
-212
lines changed

‎src/librustc_passes/ast_sanity.rs

+95-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use rustc::session::Session;
2121
use syntax::ast::*;
2222
use syntax::codemap::Span;
2323
use syntax::errors;
24-
use syntax::parse::token::keywords;
24+
use syntax::parse::token::{self, keywords};
2525
use syntax::visit::{self, Visitor};
2626

2727
struct SanityChecker<'a> {
@@ -44,6 +44,17 @@ impl<'a> SanityChecker<'a> {
4444
);
4545
}
4646
}
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+
}
4758
}
4859

4960
impl<'a, 'v> Visitor<'v> for SanityChecker<'a> {
@@ -72,6 +83,89 @@ impl<'a, 'v> Visitor<'v> for SanityChecker<'a> {
7283

7384
visit::walk_expr(self, expr)
7485
}
86+
87+
fn visit_path(&mut self, path: &Path, id: NodeId) {
88+
if path.global && path.segments.len() > 0 {
89+
let ident = path.segments[0].identifier;
90+
if token::Ident(ident).is_path_segment_keyword() {
91+
self.session.add_lint(
92+
lint::builtin::SUPER_OR_SELF_IN_GLOBAL_PATH, id, path.span,
93+
format!("global paths cannot start with `{}`", ident)
94+
);
95+
}
96+
}
97+
98+
visit::walk_path(self, path)
99+
}
100+
101+
fn visit_item(&mut self, item: &Item) {
102+
match item.node {
103+
ItemKind::Use(ref view_path) => {
104+
let path = view_path.node.path();
105+
if !path.segments.iter().all(|segment| segment.parameters.is_empty()) {
106+
self.err_handler().span_err(path.span, "type or lifetime parameters \
107+
in import path");
108+
}
109+
}
110+
ItemKind::Impl(_, _, _, Some(..), _, ref impl_items) => {
111+
self.invalid_visibility(&item.vis, item.span, None);
112+
for impl_item in impl_items {
113+
self.invalid_visibility(&impl_item.vis, impl_item.span, None);
114+
}
115+
}
116+
ItemKind::Impl(_, _, _, None, _, _) => {
117+
self.invalid_visibility(&item.vis, item.span, Some("place qualifiers on individual \
118+
impl items instead"));
119+
}
120+
ItemKind::DefaultImpl(..) => {
121+
self.invalid_visibility(&item.vis, item.span, None);
122+
}
123+
ItemKind::ForeignMod(..) => {
124+
self.invalid_visibility(&item.vis, item.span, Some("place qualifiers on individual \
125+
foreign items instead"));
126+
}
127+
ItemKind::Enum(ref def, _) => {
128+
for variant in &def.variants {
129+
for field in variant.node.data.fields() {
130+
self.invalid_visibility(&field.vis, field.span, None);
131+
}
132+
}
133+
}
134+
_ => {}
135+
}
136+
137+
visit::walk_item(self, item)
138+
}
139+
140+
fn visit_variant_data(&mut self, vdata: &VariantData, _: Ident,
141+
_: &Generics, _: NodeId, span: Span) {
142+
if vdata.fields().is_empty() {
143+
if vdata.is_tuple() {
144+
self.err_handler().struct_span_err(span, "empty tuple structs and enum variants \
145+
are not allowed, use unit structs and \
146+
enum variants instead")
147+
.span_help(span, "remove trailing `()` to make a unit \
148+
struct or unit enum variant")
149+
.emit();
150+
}
151+
}
152+
153+
visit::walk_struct_def(self, vdata)
154+
}
155+
156+
fn visit_vis(&mut self, vis: &Visibility) {
157+
match *vis {
158+
Visibility::Restricted{ref path, ..} => {
159+
if !path.segments.iter().all(|segment| segment.parameters.is_empty()) {
160+
self.err_handler().span_err(path.span, "type or lifetime parameters \
161+
in visibility path");
162+
}
163+
}
164+
_ => {}
165+
}
166+
167+
visit::walk_vis(self, vis)
168+
}
75169
}
76170

77171
pub fn check_crate(session: &Session, krate: &Crate) {

‎src/librustc_passes/diagnostics.rs

+40
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,46 @@ fn some_func() {
118118
```
119119
"##,
120120

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

123163
register_diagnostics! {

‎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
@@ -111,46 +111,6 @@ pub enum Foo {
111111
```
112112
"##,
113113

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

‎src/librustc_privacy/lib.rs

+8-94
Original file line numberDiff line numberDiff line change
@@ -21,37 +21,26 @@
2121
#![feature(rustc_private)]
2222
#![feature(staged_api)]
2323

24-
#[macro_use] extern crate log;
24+
extern crate rustc;
2525
#[macro_use] extern crate syntax;
2626

27-
#[macro_use] extern crate rustc;
28-
29-
use std::cmp;
30-
use std::mem::replace;
31-
27+
use rustc::dep_graph::DepNode;
3228
use rustc::hir::{self, PatKind};
29+
use rustc::hir::def::{self, Def};
30+
use rustc::hir::def_id::DefId;
3331
use rustc::hir::intravisit::{self, Visitor};
3432
use rustc::hir::pat_util::EnumerateAndAdjustIterator;
35-
use rustc::dep_graph::DepNode;
3633
use rustc::lint;
37-
use rustc::hir::def::{self, Def};
38-
use rustc::hir::def_id::DefId;
3934
use rustc::middle::privacy::{AccessLevel, AccessLevels};
4035
use rustc::ty::{self, TyCtxt};
4136
use rustc::util::nodemap::NodeSet;
42-
use rustc::hir::map as ast_map;
43-
4437
use syntax::ast;
4538
use syntax::codemap::Span;
4639

47-
pub mod diagnostics;
48-
49-
type Context<'a, 'tcx> = (&'a ty::MethodMap<'tcx>, &'a def::ExportMap);
40+
use std::cmp;
41+
use std::mem::replace;
5042

51-
/// Result of a checking operation - None => no errors were found. Some => an
52-
/// error and contains the span and message for reporting that error and
53-
/// optionally the same for a note about the error.
54-
type CheckResult = Option<(Span, String, Option<(Span, String)>)>;
43+
pub mod diagnostics;
5544

5645
////////////////////////////////////////////////////////////////////////////////
5746
/// The embargo visitor, used to determine the exports of the ast
@@ -433,7 +422,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
433422
hir::ExprMethodCall(..) => {
434423
let method_call = ty::MethodCall::expr(expr.id);
435424
let method = self.tcx.tables.borrow().method_map[&method_call];
436-
debug!("(privacy checking) checking impl method");
437425
self.check_method(expr.span, method.def_id);
438426
}
439427
hir::ExprStruct(..) => {
@@ -518,74 +506,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
518506
}
519507
}
520508

521-
////////////////////////////////////////////////////////////////////////////////
522-
/// The privacy sanity check visitor, ensures unnecessary visibility isn't here
523-
////////////////////////////////////////////////////////////////////////////////
524-
525-
struct SanePrivacyVisitor<'a, 'tcx: 'a> {
526-
tcx: TyCtxt<'a, 'tcx, 'tcx>,
527-
}
528-
529-
impl<'a, 'tcx, 'v> Visitor<'v> for SanePrivacyVisitor<'a, 'tcx> {
530-
fn visit_item(&mut self, item: &hir::Item) {
531-
self.check_sane_privacy(item);
532-
intravisit::walk_item(self, item);
533-
}
534-
}
535-
536-
impl<'a, 'tcx> SanePrivacyVisitor<'a, 'tcx> {
537-
/// Validate that items that shouldn't have visibility qualifiers don't have them.
538-
/// Such qualifiers can be set by syntax extensions even if the parser doesn't allow them,
539-
/// so we check things like variant fields too.
540-
fn check_sane_privacy(&self, item: &hir::Item) {
541-
let check_inherited = |sp, vis: &hir::Visibility, note: &str| {
542-
if *vis != hir::Inherited {
543-
let mut err = struct_span_err!(self.tcx.sess, sp, E0449,
544-
"unnecessary visibility qualifier");
545-
if !note.is_empty() {
546-
err.span_note(sp, note);
547-
}
548-
err.emit();
549-
}
550-
};
551-
552-
match item.node {
553-
hir::ItemImpl(_, _, _, Some(..), _, ref impl_items) => {
554-
check_inherited(item.span, &item.vis,
555-
"visibility qualifiers have no effect on trait impls");
556-
for impl_item in impl_items {
557-
check_inherited(impl_item.span, &impl_item.vis,
558-
"visibility qualifiers have no effect on trait impl items");
559-
}
560-
}
561-
hir::ItemImpl(_, _, _, None, _, _) => {
562-
check_inherited(item.span, &item.vis,
563-
"place qualifiers on individual methods instead");
564-
}
565-
hir::ItemDefaultImpl(..) => {
566-
check_inherited(item.span, &item.vis,
567-
"visibility qualifiers have no effect on trait impls");
568-
}
569-
hir::ItemForeignMod(..) => {
570-
check_inherited(item.span, &item.vis,
571-
"place qualifiers on individual functions instead");
572-
}
573-
hir::ItemEnum(ref def, _) => {
574-
for variant in &def.variants {
575-
for field in variant.node.data.fields() {
576-
check_inherited(field.span, &field.vis,
577-
"visibility qualifiers have no effect on variant fields");
578-
}
579-
}
580-
}
581-
hir::ItemStruct(..) | hir::ItemTrait(..) |
582-
hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
583-
hir::ItemMod(..) | hir::ItemExternCrate(..) |
584-
hir::ItemUse(..) | hir::ItemTy(..) => {}
585-
}
586-
}
587-
}
588-
589509
///////////////////////////////////////////////////////////////////////////////
590510
/// Obsolete visitors for checking for private items in public interfaces.
591511
/// These visitors are supposed to be kept in frozen state and produce an
@@ -626,7 +546,7 @@ impl<'a, 'tcx> ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx> {
626546
// .. and it corresponds to a private type in the AST (this returns
627547
// None for type parameters)
628548
match self.tcx.map.find(node_id) {
629-
Some(ast_map::NodeItem(ref item)) => item.vis != hir::Public,
549+
Some(hir::map::NodeItem(ref item)) => item.vis != hir::Public,
630550
Some(_) | None => false,
631551
}
632552
} else {
@@ -860,7 +780,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx>
860780
// any `visit_ty`'s will be called on things that are in
861781
// public signatures, i.e. things that we're interested in for
862782
// this visitor.
863-
debug!("VisiblePrivateTypesVisitor entering item {:?}", item);
864783
intravisit::walk_item(self, item);
865784
}
866785

@@ -892,7 +811,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx>
892811
}
893812

894813
fn visit_ty(&mut self, t: &hir::Ty) {
895-
debug!("VisiblePrivateTypesVisitor checking ty {:?}", t);
896814
if let hir::TyPath(..) = t.node {
897815
if self.path_is_private_type(t.id) {
898816
self.old_error_set.insert(t.id);
@@ -1177,10 +1095,6 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
11771095

11781096
let krate = tcx.map.krate();
11791097

1180-
// Sanity check to make sure that all privacy usage is reasonable.
1181-
let mut visitor = SanePrivacyVisitor { tcx: tcx };
1182-
krate.visit_all_items(&mut visitor);
1183-
11841098
// Use the parent map to check the privacy of everything
11851099
let mut visitor = PrivacyVisitor {
11861100
curitem: ast::DUMMY_NODE_ID,

0 commit comments

Comments
 (0)