Skip to content

Commit e9f25b3

Browse files
J-ZhengLiJ-ZhengLi
committed
add test cases for rust-lang#12435
don't lint [`mixed_attributes_style`] when mixing docs and other attrs add test files for issue rust-lang#12436 move [`mixed_attributes_style`] to `LateLintPass` to enable global `allow` stop [`mixed_attributes_style`] from linting on different attributes add `@compile-flags` to [`mixed_attributes_style`]'s test; turns out not linting in test mod is not a FN. Apply suggestions from code review Co-authored-by: Timo <[email protected]> move [`mixed_attributes_style`] to late pass and stop it from linting on different kind of attributes
1 parent 99e8000 commit e9f25b3

File tree

8 files changed

+214
-23
lines changed

8 files changed

+214
-23
lines changed

‎clippy_lints/src/attrs/mixed_attributes_style.rs‎

Lines changed: 70 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,85 @@
11
use super::MIXED_ATTRIBUTES_STYLE;
22
use clippy_utils::diagnostics::span_lint;
3-
use rustc_ast::AttrStyle;
4-
use rustc_lint::EarlyContext;
3+
use rustc_ast::{AttrKind, AttrStyle, Attribute};
4+
use rustc_data_structures::fx::FxHashSet;
5+
use rustc_lint::{LateContext, LintContext};
6+
use rustc_span::source_map::SourceMap;
7+
use rustc_span::{SourceFile, Span, Symbol};
8+
use std::sync::Arc;
59

6-
pub(super) fn check(cx: &EarlyContext<'_>, item: &rustc_ast::Item) {
7-
let mut has_outer = false;
8-
let mut has_inner = false;
10+
#[derive(Hash, PartialEq, Eq)]
11+
enum SimpleAttrKind {
12+
Doc,
13+
/// A normal attribute, with its name symbols.
14+
Normal(Vec<Symbol>),
15+
}
16+
17+
impl From<&AttrKind> for SimpleAttrKind {
18+
fn from(value: &AttrKind) -> Self {
19+
match value {
20+
AttrKind::Normal(attr) => {
21+
let path_symbols = attr
22+
.item
23+
.path
24+
.segments
25+
.iter()
26+
.map(|seg| seg.ident.name)
27+
.collect::<Vec<_>>();
28+
Self::Normal(path_symbols)
29+
},
30+
AttrKind::DocComment(..) => Self::Doc,
31+
}
32+
}
33+
}
34+
35+
pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute]) {
36+
let mut inner_attr_kind: FxHashSet<SimpleAttrKind> = FxHashSet::default();
37+
let mut outer_attr_kind: FxHashSet<SimpleAttrKind> = FxHashSet::default();
38+
39+
let source_map = cx.sess().source_map();
40+
let item_src = source_map.lookup_source_file(item_span.lo());
941

10-
for attr in &item.attrs {
11-
if attr.span.from_expansion() {
42+
for attr in attrs {
43+
if attr.span.from_expansion() || !attr_in_same_src_as_item(source_map, &item_src, attr.span) {
1244
continue;
1345
}
46+
47+
let kind: SimpleAttrKind = (&attr.kind).into();
1448
match attr.style {
15-
AttrStyle::Inner => has_inner = true,
16-
AttrStyle::Outer => has_outer = true,
17-
}
49+
AttrStyle::Inner => {
50+
if outer_attr_kind.contains(&kind) {
51+
lint_mixed_attrs(cx, attrs);
52+
return;
53+
}
54+
inner_attr_kind.insert(kind);
55+
},
56+
AttrStyle::Outer => {
57+
if inner_attr_kind.contains(&kind) {
58+
lint_mixed_attrs(cx, attrs);
59+
return;
60+
}
61+
outer_attr_kind.insert(kind);
62+
},
63+
};
1864
}
19-
if !has_outer || !has_inner {
65+
}
66+
67+
fn lint_mixed_attrs(cx: &LateContext<'_>, attrs: &[Attribute]) {
68+
let mut attrs_iter = attrs.iter().filter(|attr| !attr.span.from_expansion());
69+
let span = if let (Some(first), Some(last)) = (attrs_iter.next(), attrs_iter.last()) {
70+
first.span.with_hi(last.span.hi())
71+
} else {
2072
return;
21-
}
22-
let mut attrs_iter = item.attrs.iter().filter(|attr| !attr.span.from_expansion());
23-
let span = attrs_iter.next().unwrap().span;
73+
};
2474
span_lint(
2575
cx,
2676
MIXED_ATTRIBUTES_STYLE,
27-
span.with_hi(attrs_iter.last().unwrap().span.hi()),
77+
span,
2878
"item has both inner and outer attributes",
2979
);
3080
}
81+
82+
fn attr_in_same_src_as_item(source_map: &SourceMap, item_src: &Arc<SourceFile>, attr_span: Span) -> bool {
83+
let attr_src = source_map.lookup_source_file(attr_span.lo());
84+
Arc::ptr_eq(item_src, &attr_src)
85+
}

‎clippy_lints/src/attrs/mod.rs‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,10 +465,20 @@ declare_clippy_lint! {
465465

466466
declare_clippy_lint! {
467467
/// ### What it does
468-
/// Checks that an item has only one kind of attributes.
468+
/// Checks for items that have the same kind of attributes with mixed styles (inner/outer).
469469
///
470470
/// ### Why is this bad?
471-
/// Having both kinds of attributes makes it more complicated to read code.
471+
/// Having both style of said attributes makes it more complicated to read code.
472+
///
473+
/// ### Known problems
474+
/// This lint currently has false-negatives when mixing same attributes
475+
/// but they have different path symbols, for example:
476+
/// ```ignore
477+
/// #[custom_attribute]
478+
/// pub fn foo() {
479+
/// #![my_crate::custom_attribute]
480+
/// }
481+
/// ```
472482
///
473483
/// ### Example
474484
/// ```no_run
@@ -523,6 +533,7 @@ declare_lint_pass!(Attributes => [
523533
USELESS_ATTRIBUTE,
524534
BLANKET_CLIPPY_RESTRICTION_LINTS,
525535
SHOULD_PANIC_WITHOUT_EXPECT,
536+
MIXED_ATTRIBUTES_STYLE,
526537
]);
527538

528539
impl<'tcx> LateLintPass<'tcx> for Attributes {
@@ -566,6 +577,7 @@ impl<'tcx> LateLintPass<'tcx> for Attributes {
566577
ItemKind::ExternCrate(..) | ItemKind::Use(..) => useless_attribute::check(cx, item, attrs),
567578
_ => {},
568579
}
580+
mixed_attributes_style::check(cx, item.span, attrs);
569581
}
570582

571583
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {
@@ -594,7 +606,6 @@ impl_lint_pass!(EarlyAttributes => [
594606
MAYBE_MISUSED_CFG,
595607
DEPRECATED_CLIPPY_CFG_ATTR,
596608
UNNECESSARY_CLIPPY_CFG,
597-
MIXED_ATTRIBUTES_STYLE,
598609
DUPLICATED_ATTRIBUTES,
599610
]);
600611

@@ -605,7 +616,6 @@ impl EarlyLintPass for EarlyAttributes {
605616

606617
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &rustc_ast::Item) {
607618
empty_line_after::check(cx, item);
608-
mixed_attributes_style::check(cx, item);
609619
duplicated_attributes::check(cx, &item.attrs);
610620
}
611621

‎tests/ui/mixed_attributes_style.rs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
1+
//@aux-build:proc_macro_attr.rs
2+
//@compile-flags: --test --cfg dummy_cfg
3+
#![feature(custom_inner_attributes)]
14
#![warn(clippy::mixed_attributes_style)]
25
#![allow(clippy::duplicated_attributes)]
36

7+
#[macro_use]
8+
extern crate proc_macro_attr;
9+
410
#[allow(unused)] //~ ERROR: item has both inner and outer attributes
511
fn foo1() {
612
#![allow(unused)]
@@ -38,3 +44,57 @@ mod bar {
3844
fn main() {
3945
// test code goes here
4046
}
47+
48+
// issue #12435
49+
#[cfg(test)]
50+
mod tests {
51+
//! Module doc, don't lint
52+
}
53+
#[allow(unused)]
54+
mod baz {
55+
//! Module doc, don't lint
56+
const FOO: u8 = 0;
57+
}
58+
/// Module doc, don't lint
59+
mod quz {
60+
#![allow(unused)]
61+
}
62+
63+
mod issue_12530 {
64+
// don't lint different attributes entirely
65+
#[cfg(test)]
66+
mod tests {
67+
#![allow(clippy::unreadable_literal)]
68+
69+
#[allow(dead_code)] //~ ERROR: item has both inner and outer attributes
70+
mod inner_mod {
71+
#![allow(dead_code)]
72+
}
73+
}
74+
#[cfg(dummy_cfg)]
75+
mod another_mod {
76+
#![allow(clippy::question_mark)]
77+
}
78+
/// Nested mod
79+
mod nested_mod {
80+
#[allow(dead_code)] //~ ERROR: item has both inner and outer attributes
81+
mod inner_mod {
82+
#![allow(dead_code)]
83+
}
84+
}
85+
/// Nested mod //~ ERROR: item has both inner and outer attributes
86+
#[allow(unused)]
87+
mod nest_mod_2 {
88+
#![allow(unused)]
89+
90+
#[allow(dead_code)] //~ ERROR: item has both inner and outer attributes
91+
mod inner_mod {
92+
#![allow(dead_code)]
93+
}
94+
}
95+
// Different path symbols - Known FN
96+
#[dummy]
97+
fn use_dummy() {
98+
#![proc_macro_attr::dummy]
99+
}
100+
}

‎tests/ui/mixed_attributes_style.stderr‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: item has both inner and outer attributes
2-
--> tests/ui/mixed_attributes_style.rs:4:1
2+
--> tests/ui/mixed_attributes_style.rs:10:1
33
|
44
LL | / #[allow(unused)]
55
LL | | fn foo1() {
@@ -10,7 +10,7 @@ LL | | #![allow(unused)]
1010
= help: to override `-D warnings` add `#[allow(clippy::mixed_attributes_style)]`
1111

1212
error: item has both inner and outer attributes
13-
--> tests/ui/mixed_attributes_style.rs:18:1
13+
--> tests/ui/mixed_attributes_style.rs:24:1
1414
|
1515
LL | / /// linux
1616
LL | |
@@ -19,12 +19,45 @@ LL | | //! windows
1919
| |_______________^
2020

2121
error: item has both inner and outer attributes
22-
--> tests/ui/mixed_attributes_style.rs:33:1
22+
--> tests/ui/mixed_attributes_style.rs:39:1
2323
|
2424
LL | / #[allow(unused)]
2525
LL | | mod bar {
2626
LL | | #![allow(unused)]
2727
| |_____________________^
2828

29-
error: aborting due to 3 previous errors
29+
error: item has both inner and outer attributes
30+
--> tests/ui/mixed_attributes_style.rs:69:9
31+
|
32+
LL | / #[allow(dead_code)]
33+
LL | | mod inner_mod {
34+
LL | | #![allow(dead_code)]
35+
| |________________________________^
36+
37+
error: item has both inner and outer attributes
38+
--> tests/ui/mixed_attributes_style.rs:80:9
39+
|
40+
LL | / #[allow(dead_code)]
41+
LL | | mod inner_mod {
42+
LL | | #![allow(dead_code)]
43+
| |________________________________^
44+
45+
error: item has both inner and outer attributes
46+
--> tests/ui/mixed_attributes_style.rs:85:5
47+
|
48+
LL | / /// Nested mod
49+
LL | | #[allow(unused)]
50+
LL | | mod nest_mod_2 {
51+
LL | | #![allow(unused)]
52+
| |_________________________^
53+
54+
error: item has both inner and outer attributes
55+
--> tests/ui/mixed_attributes_style.rs:90:9
56+
|
57+
LL | / #[allow(dead_code)]
58+
LL | | mod inner_mod {
59+
LL | | #![allow(dead_code)]
60+
| |________________________________^
61+
62+
error: aborting due to 7 previous errors
3063

‎tests/ui/mixed_attributes_style/auxiliary/submodule.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
//! Module level doc
2+
3+
#![allow(dead_code)]
4+
5+
#[allow(unused)]
6+
//~^ ERROR: item has both inner and outer attributes
7+
mod foo {
8+
#![allow(dead_code)]
9+
}

‎tests/ui/mixed_attributes_style/global_allow.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// issue 12436
2+
#![allow(clippy::mixed_attributes_style)]
3+
4+
#[path = "auxiliary/submodule.rs"]
5+
mod submodule;
6+
7+
fn main() {}

‎tests/ui/mixed_attributes_style/mod_declaration.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
#[path = "auxiliary/submodule.rs"] // don't lint.
2+
/// This doc comment should not lint, it could be used to add context to the original module doc
3+
mod submodule;

‎tests/ui/mixed_attributes_style/mod_declaration.stderr‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
error: item has both inner and outer attributes
2+
--> tests/ui/mixed_attributes_style/auxiliary/submodule.rs:5:1
3+
|
4+
LL | / #[allow(unused)]
5+
LL | |
6+
LL | | mod foo {
7+
LL | | #![allow(dead_code)]
8+
| |________________________^
9+
|
10+
= note: `-D clippy::mixed-attributes-style` implied by `-D warnings`
11+
= help: to override `-D warnings` add `#[allow(clippy::mixed_attributes_style)]`
12+
13+
error: aborting due to 1 previous error
14+

0 commit comments

Comments
 (0)