Skip to content

Commit 121e65f

Browse files
committed
Fix inconsistencies in handling of inert attributes on statements
When the 'early' and 'late' visitors visit an attribute target, they activate any lint attributes (e.g. `#[allow]`) that apply to it. This can affect warnings emitted on sibiling attributes. For example, the following code does not produce an `unused_attributes` for `#[inline]`, since the sibiling `#[allow(unused_attributes)]` suppressed the warning. ```rust trait Foo { #[allow(unused_attributes)] #[inline] fn first(); #[inline] #[allow(unused_attributes)] fn second(); } ``` However, we do not do this for statements - instead, the lint attributes only become active when we visit the struct nested inside `StmtKind` (e.g. `Item`). Currently, this is difficult to observe due to another issue - the `HasAttrs` impl for `StmtKind` ignores attributes for `StmtKind::Item`. As a result, the `unused_doc_comments` lint will never see attributes on item statements. This commit makes two interrelated fixes to the handling of inert (non-proc-macro) attributes on statements: * The `HasAttr` impl for `StmtKind` now returns attributes for `StmtKind::Item`, treating it just like every other `StmtKind` variant. The only place relying on the old behavior was macro which has been updated to explicitly ignore attributes on item statements. This allows the `unused_doc_comments` lint to fire for item statements. * The `early` and `late` lint visitors now activate lint attributes when invoking the callback for `Stmt`. This ensures that a lint attribute (e.g. `#[allow(unused_doc_comments)]`) can be applied to sibiling attributes on an item statement. The `unused_doc_comments` lint now fires on a number of items in the stnadard library. I've added `#[allow(unused_doc_comments)]` to these items.
1 parent 07a63e6 commit 121e65f

File tree

16 files changed

+140
-22
lines changed

16 files changed

+140
-22
lines changed

Diff for: compiler/rustc_ast/src/attr/mod.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,8 @@ impl HasAttrs for StmtKind {
623623
match *self {
624624
StmtKind::Local(ref local) => local.attrs(),
625625
StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => expr.attrs(),
626-
StmtKind::Empty | StmtKind::Item(..) => &[],
626+
StmtKind::Item(ref item) => item.attrs(),
627+
StmtKind::Empty => &[],
627628
StmtKind::MacCall(ref mac) => mac.attrs.attrs(),
628629
}
629630
}
@@ -632,7 +633,8 @@ impl HasAttrs for StmtKind {
632633
match self {
633634
StmtKind::Local(local) => local.visit_attrs(f),
634635
StmtKind::Expr(expr) | StmtKind::Semi(expr) => expr.visit_attrs(f),
635-
StmtKind::Empty | StmtKind::Item(..) => {}
636+
StmtKind::Item(item) => item.visit_attrs(f),
637+
StmtKind::Empty => {}
636638
StmtKind::MacCall(mac) => {
637639
mac.attrs.visit_attrs(f);
638640
}

Diff for: compiler/rustc_expand/src/expand.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -1357,7 +1357,8 @@ impl<'a, 'b> MutVisitor for InvocationCollector<'a, 'b> {
13571357
// we'll expand attributes on expressions separately
13581358
if !stmt.is_expr() {
13591359
let (attr, derives, after_derive) = if stmt.is_item() {
1360-
self.classify_item(&mut stmt)
1360+
// FIXME: Handle custom attributes on statements (#15701)
1361+
(None, vec![], false)
13611362
} else {
13621363
// ignore derives on non-item statements so it falls through
13631364
// to the unused-attributes lint

Diff for: compiler/rustc_hir/src/hir.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -1099,11 +1099,11 @@ pub enum StmtKind<'hir> {
10991099
Semi(&'hir Expr<'hir>),
11001100
}
11011101

1102-
impl StmtKind<'hir> {
1103-
pub fn attrs(&self) -> &'hir [Attribute] {
1102+
impl<'hir> StmtKind<'hir> {
1103+
pub fn attrs(&self, get_item: impl FnOnce(ItemId) -> &'hir Item<'hir>) -> &'hir [Attribute] {
11041104
match *self {
11051105
StmtKind::Local(ref l) => &l.attrs,
1106-
StmtKind::Item(_) => &[],
1106+
StmtKind::Item(ref item_id) => &get_item(*item_id).attrs,
11071107
StmtKind::Expr(ref e) | StmtKind::Semi(ref e) => &e.attrs,
11081108
}
11091109
}

Diff for: compiler/rustc_lint/src/early.rs

+17-2
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ use crate::context::{EarlyContext, LintContext, LintStore};
1818
use crate::passes::{EarlyLintPass, EarlyLintPassObject};
1919
use rustc_ast as ast;
2020
use rustc_ast::visit as ast_visit;
21+
use rustc_attr::HasAttrs;
2122
use rustc_session::lint::{BufferedEarlyLint, LintBuffer, LintPass};
2223
use rustc_session::Session;
2324
use rustc_span::symbol::Ident;
@@ -119,8 +120,22 @@ impl<'a, T: EarlyLintPass> ast_visit::Visitor<'a> for EarlyContextAndPass<'a, T>
119120
}
120121

121122
fn visit_stmt(&mut self, s: &'a ast::Stmt) {
122-
run_early_pass!(self, check_stmt, s);
123-
self.check_id(s.id);
123+
// Add the statement's lint attributes to our
124+
// current state when checking the statement itself.
125+
// This allows us to handle attributes like
126+
// `#[allow(unused_doc_comments)]`, which apply to
127+
// sibling attributes on the same target
128+
//
129+
// Note that statements get their attributes from
130+
// the AST struct that they wrap (e.g. an item)
131+
self.with_lint_attrs(s.id, s.attrs(), |cx| {
132+
run_early_pass!(cx, check_stmt, s);
133+
cx.check_id(s.id);
134+
});
135+
// The visitor for the AST struct wrapped
136+
// by the statement (e.g. `Item`) will call
137+
// `with_lint_attrs`, so do this walk
138+
// outside of the above `with_lint_attrs` call
124139
ast_visit::walk_stmt(self, s);
125140
}
126141

Diff for: compiler/rustc_lint/src/late.rs

+7-6
Original file line numberDiff line numberDiff line change
@@ -174,12 +174,13 @@ impl<'tcx, T: LateLintPass<'tcx>> hir_visit::Visitor<'tcx> for LateContextAndPas
174174
}
175175

176176
fn visit_stmt(&mut self, s: &'tcx hir::Stmt<'tcx>) {
177-
// statement attributes are actually just attributes on one of
178-
// - item
179-
// - local
180-
// - expression
181-
// so we keep track of lint levels there
182-
lint_callback!(self, check_stmt, s);
177+
let get_item = |id: hir::ItemId| self.context.tcx.hir().item(id.id);
178+
let attrs = &s.kind.attrs(get_item);
179+
// See `EarlyContextAndPass::visit_stmt` for an explanation
180+
// of why we call `walk_stmt` outside of `with_lint_attrs`
181+
self.with_lint_attrs(s.hir_id, attrs, |cx| {
182+
lint_callback!(cx, check_stmt, s);
183+
});
183184
hir_visit::walk_stmt(self, s);
184185
}
185186

Diff for: compiler/rustc_lint/src/levels.rs

+7
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,13 @@ impl<'tcx> intravisit::Visitor<'tcx> for LintLevelMapBuilder<'_, 'tcx> {
562562
})
563563
}
564564

565+
fn visit_stmt(&mut self, e: &'tcx hir::Stmt<'tcx>) {
566+
// We will call `with_lint_attrs` when we walk
567+
// the `StmtKind`. The outer statement itself doesn't
568+
// define the lint levels.
569+
intravisit::walk_stmt(self, e);
570+
}
571+
565572
fn visit_expr(&mut self, e: &'tcx hir::Expr<'tcx>) {
566573
self.with_lint_attrs(e.hir_id, &e.attrs, |builder| {
567574
intravisit::walk_expr(builder, e);

Diff for: compiler/rustc_middle/src/hir/map/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -816,7 +816,7 @@ impl<'hir> Map<'hir> {
816816
Some(Node::Variant(ref v)) => Some(&v.attrs[..]),
817817
Some(Node::Field(ref f)) => Some(&f.attrs[..]),
818818
Some(Node::Expr(ref e)) => Some(&*e.attrs),
819-
Some(Node::Stmt(ref s)) => Some(s.kind.attrs()),
819+
Some(Node::Stmt(ref s)) => Some(s.kind.attrs(|id| self.item(id.id))),
820820
Some(Node::Arm(ref a)) => Some(&*a.attrs),
821821
Some(Node::GenericParam(param)) => Some(&param.attrs[..]),
822822
// Unit/tuple structs/variants take the attributes straight from

Diff for: src/test/ui/allow-doc-comments.rs

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// check-pass
2+
3+
fn main() {
4+
5+
#[allow(unused_doc_comments)]
6+
/// A doc comment
7+
fn bar() {}
8+
9+
/// Another doc comment
10+
#[allow(unused_doc_comments)]
11+
struct Foo {}
12+
}

Diff for: src/test/ui/lint/reasons-forbidden.rs

+12
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
//~^ NOTE `forbid` level set here
66
//~| NOTE `forbid` level set here
77
//~| NOTE `forbid` level set here
8+
//~| NOTE `forbid` level set here
9+
//~| NOTE `forbid` level set here
10+
//~| NOTE `forbid` level set here
811
reason = "our errors & omissions insurance policy doesn't cover unsafe Rust"
912
)]
1013

@@ -17,9 +20,18 @@ fn main() {
1720
//~^ ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
1821
//~| ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
1922
//~| ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
23+
//~| ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
24+
//~| ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
25+
//~| ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
2026
//~| NOTE overruled by previous forbid
2127
//~| NOTE overruled by previous forbid
2228
//~| NOTE overruled by previous forbid
29+
//~| NOTE overruled by previous forbid
30+
//~| NOTE overruled by previous forbid
31+
//~| NOTE overruled by previous forbid
32+
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust
33+
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust
34+
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust
2335
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust
2436
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust
2537
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust

Diff for: src/test/ui/lint/reasons-forbidden.stderr

+37-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
2-
--> $DIR/reasons-forbidden.rs:16:13
2+
--> $DIR/reasons-forbidden.rs:19:13
33
|
44
LL | unsafe_code,
55
| ----------- `forbid` level set here
@@ -10,7 +10,7 @@ LL | #[allow(unsafe_code)]
1010
= note: our errors & omissions insurance policy doesn't cover unsafe Rust
1111

1212
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
13-
--> $DIR/reasons-forbidden.rs:16:13
13+
--> $DIR/reasons-forbidden.rs:19:13
1414
|
1515
LL | unsafe_code,
1616
| ----------- `forbid` level set here
@@ -21,7 +21,7 @@ LL | #[allow(unsafe_code)]
2121
= note: our errors & omissions insurance policy doesn't cover unsafe Rust
2222

2323
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
24-
--> $DIR/reasons-forbidden.rs:16:13
24+
--> $DIR/reasons-forbidden.rs:19:13
2525
|
2626
LL | unsafe_code,
2727
| ----------- `forbid` level set here
@@ -31,6 +31,39 @@ LL | #[allow(unsafe_code)]
3131
|
3232
= note: our errors & omissions insurance policy doesn't cover unsafe Rust
3333

34-
error: aborting due to 3 previous errors
34+
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
35+
--> $DIR/reasons-forbidden.rs:19:13
36+
|
37+
LL | unsafe_code,
38+
| ----------- `forbid` level set here
39+
...
40+
LL | #[allow(unsafe_code)]
41+
| ^^^^^^^^^^^ overruled by previous forbid
42+
|
43+
= note: our errors & omissions insurance policy doesn't cover unsafe Rust
44+
45+
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
46+
--> $DIR/reasons-forbidden.rs:19:13
47+
|
48+
LL | unsafe_code,
49+
| ----------- `forbid` level set here
50+
...
51+
LL | #[allow(unsafe_code)]
52+
| ^^^^^^^^^^^ overruled by previous forbid
53+
|
54+
= note: our errors & omissions insurance policy doesn't cover unsafe Rust
55+
56+
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
57+
--> $DIR/reasons-forbidden.rs:19:13
58+
|
59+
LL | unsafe_code,
60+
| ----------- `forbid` level set here
61+
...
62+
LL | #[allow(unsafe_code)]
63+
| ^^^^^^^^^^^ overruled by previous forbid
64+
|
65+
= note: our errors & omissions insurance policy doesn't cover unsafe Rust
66+
67+
error: aborting due to 6 previous errors
3568

3669
For more information about this error, try `rustc --explain E0453`.

Diff for: src/test/ui/macros/macro-literal.rs

+1
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ macro_rules! match_produced_attr {
6767
($lit: literal) => {
6868
// Struct with doc comment passed via $literal
6969
#[doc = $lit]
70+
#[allow(unused_doc_comments)]
7071
struct LiteralProduced;
7172
};
7273
($expr: expr) => {

Diff for: src/test/ui/macros/macro-nested_expr.rs

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
pub macro m($inner_str:expr) {
88
#[doc = $inner_str]
9+
#[allow(unused_doc_comments)]
910
struct S;
1011
}
1112

Diff for: src/test/ui/useless-comment.rs

+9
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,15 @@ fn foo() {
3838
{
3939

4040
}
41+
42+
/// foo //~ ERROR unused doc comment
43+
struct Foo {}
44+
45+
/// bar //~ ERROR unused doc comment
46+
struct Bar {};
47+
48+
/// my_fn //~ ERROR unused_doc_comment
49+
fn my_fn() {}
4150
}
4251

4352
fn main() {

Diff for: src/test/ui/useless-comment.stderr

+25-1
Original file line numberDiff line numberDiff line change
@@ -90,5 +90,29 @@ LL | |
9090
LL | | }
9191
| |_____- rustdoc does not generate documentation for expressions
9292

93-
error: aborting due to 10 previous errors
93+
error: unused doc comment
94+
--> $DIR/useless-comment.rs:42:5
95+
|
96+
LL | /// foo
97+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
98+
LL | struct Foo {}
99+
| ------------- rustdoc does not generate documentation for inner items
100+
101+
error: unused doc comment
102+
--> $DIR/useless-comment.rs:45:5
103+
|
104+
LL | /// bar
105+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
106+
LL | struct Bar {};
107+
| -------------- rustdoc does not generate documentation for inner items
108+
109+
error: unused doc comment
110+
--> $DIR/useless-comment.rs:48:5
111+
|
112+
LL | /// my_fn
113+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
114+
LL | fn my_fn() {}
115+
| ------------- rustdoc does not generate documentation for inner items
116+
117+
error: aborting due to 13 previous errors
94118

Diff for: src/tools/clippy/clippy_lints/src/utils/author.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ impl<'tcx> LateLintPass<'tcx> for Author {
130130
}
131131

132132
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx hir::Stmt<'_>) {
133-
if !has_attr(cx.sess(), stmt.kind.attrs()) {
133+
if !has_attr(cx.sess(), stmt.kind.attrs(|id| cx.tcx.hir().item(id.id))) {
134134
return;
135135
}
136136
prelude();

Diff for: src/tools/clippy/clippy_lints/src/utils/inspector.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ impl<'tcx> LateLintPass<'tcx> for DeepCodeInspector {
109109
}
110110

111111
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx hir::Stmt<'_>) {
112-
if !has_attr(cx.sess(), stmt.kind.attrs()) {
112+
if !has_attr(cx.sess(), stmt.kind.attrs(|id| cx.tcx.hir().item(id.id))) {
113113
return;
114114
}
115115
match stmt.kind {

0 commit comments

Comments
 (0)