Skip to content

Commit f7c4829

Browse files
borsbors
committed
Auto merge of #88681 - ehuss:duplicate-attributes, r=petrochenkov
Check for duplicate attributes. This adds some checks for duplicate attributes. In many cases, the duplicates were being ignored without error or warning. This adds several kinds of checks (see `AttributeDuplicates` enum). The motivation here is to issue unused warnings with similar reasoning for any unused lint, and to error for cases where there are conflicts. This also adds a check for empty attribute lists in a few attributes where this causes the attribute to be ignored. Closes #55112.
2 parents cebd2dd + 36dcd4c commit f7c4829

15 files changed

+1052
-302
lines changed

Diff for: ‎compiler/rustc_feature/src/builtin_attrs.rs

+272-155
Large diffs are not rendered by default.

Diff for: ‎compiler/rustc_feature/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
//! even if it is stabilized or removed, *do not remove it*. Instead, move the
1212
//! symbol to the `accepted` or `removed` modules respectively.
1313
14+
#![feature(derive_default_enum)]
1415
#![feature(once_cell)]
1516

1617
mod accepted;
@@ -146,6 +147,7 @@ pub fn find_feature_issue(feature: Symbol, issue: GateIssue) -> Option<NonZeroU3
146147

147148
pub use accepted::ACCEPTED_FEATURES;
148149
pub use active::{Features, ACTIVE_FEATURES, INCOMPATIBLE_FEATURES};
150+
pub use builtin_attrs::AttributeDuplicates;
149151
pub use builtin_attrs::{
150152
deprecated_attributes, find_gated_cfg, is_builtin_attr_name, AttributeGate, AttributeTemplate,
151153
AttributeType, BuiltinAttribute, GatedCfg, BUILTIN_ATTRIBUTES, BUILTIN_ATTRIBUTE_MAP,

Diff for: ‎compiler/rustc_passes/src/check_attr.rs

+110-17
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ use rustc_middle::ty::query::Providers;
99
use rustc_middle::ty::TyCtxt;
1010

1111
use rustc_ast::{ast, AttrStyle, Attribute, Lit, LitKind, NestedMetaItem};
12-
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
12+
use rustc_data_structures::fx::FxHashMap;
1313
use rustc_errors::{pluralize, struct_span_err, Applicability};
14-
use rustc_feature::{AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
14+
use rustc_feature::{AttributeDuplicates, AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
1515
use rustc_hir as hir;
1616
use rustc_hir::def_id::LocalDefId;
1717
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
@@ -23,6 +23,7 @@ use rustc_session::lint::builtin::{
2323
use rustc_session::parse::feature_err;
2424
use rustc_span::symbol::{sym, Symbol};
2525
use rustc_span::{MultiSpan, Span, DUMMY_SP};
26+
use std::collections::hash_map::Entry;
2627

2728
pub(crate) fn target_from_impl_item<'tcx>(
2829
tcx: TyCtxt<'tcx>,
@@ -69,7 +70,7 @@ impl CheckAttrVisitor<'tcx> {
6970
let mut doc_aliases = FxHashMap::default();
7071
let mut is_valid = true;
7172
let mut specified_inline = None;
72-
let mut seen = FxHashSet::default();
73+
let mut seen = FxHashMap::default();
7374
let attrs = self.tcx.hir().attrs(hir_id);
7475
for attr in attrs {
7576
let attr_is_valid = match attr.name_or_empty() {
@@ -148,6 +149,8 @@ impl CheckAttrVisitor<'tcx> {
148149
_ => {}
149150
}
150151

152+
let builtin = attr.ident().and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name));
153+
151154
if hir_id != CRATE_HIR_ID {
152155
if let Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) =
153156
attr.ident().and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name))
@@ -165,21 +168,37 @@ impl CheckAttrVisitor<'tcx> {
165168
}
166169
}
167170

168-
// Duplicate attributes
169-
match attr.name_or_empty() {
170-
name @ sym::macro_use => {
171-
let args = attr.meta_item_list().unwrap_or_else(Vec::new);
172-
let args: Vec<_> = args.iter().map(|arg| arg.name_or_empty()).collect();
173-
if !seen.insert((name, args)) {
174-
self.tcx.struct_span_lint_hir(
175-
UNUSED_ATTRIBUTES,
176-
hir_id,
171+
if let Some(BuiltinAttribute { duplicates, .. }) = builtin {
172+
check_duplicates(self.tcx, attr, hir_id, *duplicates, &mut seen);
173+
}
174+
175+
// Warn on useless empty attributes.
176+
if matches!(
177+
attr.name_or_empty(),
178+
sym::macro_use
179+
| sym::allow
180+
| sym::warn
181+
| sym::deny
182+
| sym::forbid
183+
| sym::feature
184+
| sym::repr
185+
| sym::target_feature
186+
) && attr.meta_item_list().map_or(false, |list| list.is_empty())
187+
{
188+
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
189+
lint.build("unused attribute")
190+
.span_suggestion(
177191
attr.span,
178-
|lint| lint.build("unused attribute").emit(),
179-
);
180-
}
181-
}
182-
_ => {}
192+
"remove this attribute",
193+
String::new(),
194+
Applicability::MachineApplicable,
195+
)
196+
.note(&format!(
197+
"attribute `{}` with an empty list has no effect",
198+
attr.name_or_empty()
199+
))
200+
.emit();
201+
});
183202
}
184203
}
185204

@@ -1990,3 +2009,77 @@ fn check_mod_attrs(tcx: TyCtxt<'_>, module_def_id: LocalDefId) {
19902009
pub(crate) fn provide(providers: &mut Providers) {
19912010
*providers = Providers { check_mod_attrs, ..*providers };
19922011
}
2012+
2013+
fn check_duplicates(
2014+
tcx: TyCtxt<'_>,
2015+
attr: &Attribute,
2016+
hir_id: HirId,
2017+
duplicates: AttributeDuplicates,
2018+
seen: &mut FxHashMap<Symbol, Span>,
2019+
) {
2020+
use AttributeDuplicates::*;
2021+
if matches!(duplicates, WarnFollowingWordOnly) && !attr.is_word() {
2022+
return;
2023+
}
2024+
match duplicates {
2025+
DuplicatesOk => {}
2026+
WarnFollowing | FutureWarnFollowing | WarnFollowingWordOnly | FutureWarnPreceding => {
2027+
match seen.entry(attr.name_or_empty()) {
2028+
Entry::Occupied(mut entry) => {
2029+
let (this, other) = if matches!(duplicates, FutureWarnPreceding) {
2030+
let to_remove = entry.insert(attr.span);
2031+
(to_remove, attr.span)
2032+
} else {
2033+
(attr.span, *entry.get())
2034+
};
2035+
tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, this, |lint| {
2036+
let mut db = lint.build("unused attribute");
2037+
db.span_note(other, "attribute also specified here").span_suggestion(
2038+
this,
2039+
"remove this attribute",
2040+
String::new(),
2041+
Applicability::MachineApplicable,
2042+
);
2043+
if matches!(duplicates, FutureWarnFollowing | FutureWarnPreceding) {
2044+
db.warn(
2045+
"this was previously accepted by the compiler but is \
2046+
being phased out; it will become a hard error in \
2047+
a future release!",
2048+
);
2049+
}
2050+
db.emit();
2051+
});
2052+
}
2053+
Entry::Vacant(entry) => {
2054+
entry.insert(attr.span);
2055+
}
2056+
}
2057+
}
2058+
ErrorFollowing | ErrorPreceding => match seen.entry(attr.name_or_empty()) {
2059+
Entry::Occupied(mut entry) => {
2060+
let (this, other) = if matches!(duplicates, ErrorPreceding) {
2061+
let to_remove = entry.insert(attr.span);
2062+
(to_remove, attr.span)
2063+
} else {
2064+
(attr.span, *entry.get())
2065+
};
2066+
tcx.sess
2067+
.struct_span_err(
2068+
this,
2069+
&format!("multiple `{}` attributes", attr.name_or_empty()),
2070+
)
2071+
.span_note(other, "attribute also specified here")
2072+
.span_suggestion(
2073+
this,
2074+
"remove this attribute",
2075+
String::new(),
2076+
Applicability::MachineApplicable,
2077+
)
2078+
.emit();
2079+
}
2080+
Entry::Vacant(entry) => {
2081+
entry.insert(attr.span);
2082+
}
2083+
},
2084+
}
2085+
}

Diff for: ‎compiler/rustc_typeck/src/collect.rs

-8
Original file line numberDiff line numberDiff line change
@@ -2861,14 +2861,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
28612861
} else if attr.has_name(sym::link_name) {
28622862
codegen_fn_attrs.link_name = attr.value_str();
28632863
} else if attr.has_name(sym::link_ordinal) {
2864-
if link_ordinal_span.is_some() {
2865-
tcx.sess
2866-
.struct_span_err(
2867-
attr.span,
2868-
"multiple `link_ordinal` attributes on a single definition",
2869-
)
2870-
.emit();
2871-
}
28722864
link_ordinal_span = Some(attr.span);
28732865
if let ordinal @ Some(_) = check_link_ordinal(tcx, attr) {
28742866
codegen_fn_attrs.link_ordinal = ordinal;

Diff for: ‎src/test/ui/empty/empty-attributes.rs

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
#![deny(unused_attributes)]
2+
#![allow()] //~ ERROR unused attribute
3+
#![warn()] //~ ERROR unused attribute
4+
#![deny()] //~ ERROR unused attribute
5+
#![forbid()] //~ ERROR unused attribute
6+
#![feature()] //~ ERROR unused attribute
7+
8+
#[repr()] //~ ERROR unused attribute
9+
pub struct S;
10+
11+
#[target_feature()] //~ ERROR unused attribute
12+
pub unsafe fn foo() {}
13+
14+
fn main() {}

Diff for: ‎src/test/ui/empty/empty-attributes.stderr

+63
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
error: unused attribute
2+
--> $DIR/empty-attributes.rs:8:1
3+
|
4+
LL | #[repr()]
5+
| ^^^^^^^^^ help: remove this attribute
6+
|
7+
note: the lint level is defined here
8+
--> $DIR/empty-attributes.rs:1:9
9+
|
10+
LL | #![deny(unused_attributes)]
11+
| ^^^^^^^^^^^^^^^^^
12+
= note: attribute `repr` with an empty list has no effect
13+
14+
error: unused attribute
15+
--> $DIR/empty-attributes.rs:11:1
16+
|
17+
LL | #[target_feature()]
18+
| ^^^^^^^^^^^^^^^^^^^ help: remove this attribute
19+
|
20+
= note: attribute `target_feature` with an empty list has no effect
21+
22+
error: unused attribute
23+
--> $DIR/empty-attributes.rs:2:1
24+
|
25+
LL | #![allow()]
26+
| ^^^^^^^^^^^ help: remove this attribute
27+
|
28+
= note: attribute `allow` with an empty list has no effect
29+
30+
error: unused attribute
31+
--> $DIR/empty-attributes.rs:3:1
32+
|
33+
LL | #![warn()]
34+
| ^^^^^^^^^^ help: remove this attribute
35+
|
36+
= note: attribute `warn` with an empty list has no effect
37+
38+
error: unused attribute
39+
--> $DIR/empty-attributes.rs:4:1
40+
|
41+
LL | #![deny()]
42+
| ^^^^^^^^^^ help: remove this attribute
43+
|
44+
= note: attribute `deny` with an empty list has no effect
45+
46+
error: unused attribute
47+
--> $DIR/empty-attributes.rs:5:1
48+
|
49+
LL | #![forbid()]
50+
| ^^^^^^^^^^^^ help: remove this attribute
51+
|
52+
= note: attribute `forbid` with an empty list has no effect
53+
54+
error: unused attribute
55+
--> $DIR/empty-attributes.rs:6:1
56+
|
57+
LL | #![feature()]
58+
| ^^^^^^^^^^^^^ help: remove this attribute
59+
|
60+
= note: attribute `feature` with an empty list has no effect
61+
62+
error: aborting due to 7 previous errors
63+

Diff for: ‎src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.rs

+23
Original file line numberDiff line numberDiff line change
@@ -134,4 +134,27 @@ mod start {
134134
//~^ ERROR: `start` attribute can only be used on functions
135135
}
136136

137+
#[repr(C)]
138+
//~^ ERROR: attribute should be applied to a struct, enum, or union
139+
mod repr {
140+
//~^ NOTE not a struct, enum, or union
141+
mod inner { #![repr(C)] }
142+
//~^ ERROR: attribute should be applied to a struct, enum, or union
143+
//~| NOTE not a struct, enum, or union
144+
145+
#[repr(C)] fn f() { }
146+
//~^ ERROR: attribute should be applied to a struct, enum, or union
147+
//~| NOTE not a struct, enum, or union
148+
149+
struct S;
150+
151+
#[repr(C)] type T = S;
152+
//~^ ERROR: attribute should be applied to a struct, enum, or union
153+
//~| NOTE not a struct, enum, or union
154+
155+
#[repr(C)] impl S { }
156+
//~^ ERROR: attribute should be applied to a struct, enum, or union
157+
//~| NOTE not a struct, enum, or union
158+
}
159+
137160
fn main() {}

Diff for: ‎src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.stderr

+42-3
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,21 @@ LL | | }
9191
LL | | }
9292
| |_- not a free function, impl method or static
9393

94+
error[E0517]: attribute should be applied to a struct, enum, or union
95+
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:137:8
96+
|
97+
LL | #[repr(C)]
98+
| ^
99+
LL |
100+
LL | / mod repr {
101+
LL | |
102+
LL | | mod inner { #![repr(C)] }
103+
LL | |
104+
... |
105+
LL | |
106+
LL | | }
107+
| |_- not a struct, enum, or union
108+
94109
error: attribute should be applied to an `extern crate` item
95110
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:25:1
96111
|
@@ -235,7 +250,31 @@ error: attribute should be applied to a free function, impl method or static
235250
LL | #[export_name = "2200"] fn bar() {}
236251
| ^^^^^^^^^^^^^^^^^^^^^^^ ----------- not a free function, impl method or static
237252

238-
error: aborting due to 34 previous errors
253+
error[E0517]: attribute should be applied to a struct, enum, or union
254+
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:141:25
255+
|
256+
LL | mod inner { #![repr(C)] }
257+
| --------------------^---- not a struct, enum, or union
258+
259+
error[E0517]: attribute should be applied to a struct, enum, or union
260+
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:145:12
261+
|
262+
LL | #[repr(C)] fn f() { }
263+
| ^ ---------- not a struct, enum, or union
264+
265+
error[E0517]: attribute should be applied to a struct, enum, or union
266+
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:151:12
267+
|
268+
LL | #[repr(C)] type T = S;
269+
| ^ ----------- not a struct, enum, or union
270+
271+
error[E0517]: attribute should be applied to a struct, enum, or union
272+
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:155:12
273+
|
274+
LL | #[repr(C)] impl S { }
275+
| ^ ---------- not a struct, enum, or union
276+
277+
error: aborting due to 39 previous errors
239278

240-
Some errors have detailed explanations: E0518, E0658.
241-
For more information about an error, try `rustc --explain E0518`.
279+
Some errors have detailed explanations: E0517, E0518, E0658.
280+
For more information about an error, try `rustc --explain E0517`.

Diff for: ‎src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs.rs

-13
Original file line numberDiff line numberDiff line change
@@ -245,19 +245,6 @@ mod bench {
245245
impl S { }
246246
}
247247

248-
#[repr()]
249-
mod repr {
250-
mod inner { #![repr()] }
251-
252-
#[repr()] fn f() { }
253-
254-
struct S;
255-
256-
#[repr()] type T = S;
257-
258-
#[repr()] impl S { }
259-
}
260-
261248
#[path = "3800"]
262249
mod path {
263250
mod inner { #![path="3800"] }

0 commit comments

Comments
 (0)