From e9f25b3b096a8dba595fee21ba42e255563a55b5 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 14 Mar 2024 17:06:18 +0800 Subject: [PATCH] add test cases for #12435 don't lint [`mixed_attributes_style`] when mixing docs and other attrs add test files for issue #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 <30553356+y21@users.noreply.github.com> move [`mixed_attributes_style`] to late pass and stop it from linting on different kind of attributes --- .../src/attrs/mixed_attributes_style.rs | 85 +++++++++++++++---- clippy_lints/src/attrs/mod.rs | 18 +++- tests/ui/mixed_attributes_style.rs | 60 +++++++++++++ tests/ui/mixed_attributes_style.stderr | 41 ++++++++- .../auxiliary/submodule.rs | 9 ++ .../ui/mixed_attributes_style/global_allow.rs | 7 ++ .../mixed_attributes_style/mod_declaration.rs | 3 + .../mod_declaration.stderr | 14 +++ 8 files changed, 214 insertions(+), 23 deletions(-) create mode 100644 tests/ui/mixed_attributes_style/auxiliary/submodule.rs create mode 100644 tests/ui/mixed_attributes_style/global_allow.rs create mode 100644 tests/ui/mixed_attributes_style/mod_declaration.rs create mode 100644 tests/ui/mixed_attributes_style/mod_declaration.stderr diff --git a/clippy_lints/src/attrs/mixed_attributes_style.rs b/clippy_lints/src/attrs/mixed_attributes_style.rs index c2e21cfd330..75a3d7a9ac3 100644 --- a/clippy_lints/src/attrs/mixed_attributes_style.rs +++ b/clippy_lints/src/attrs/mixed_attributes_style.rs @@ -1,30 +1,85 @@ use super::MIXED_ATTRIBUTES_STYLE; use clippy_utils::diagnostics::span_lint; -use rustc_ast::AttrStyle; -use rustc_lint::EarlyContext; +use rustc_ast::{AttrKind, AttrStyle, Attribute}; +use rustc_data_structures::fx::FxHashSet; +use rustc_lint::{LateContext, LintContext}; +use rustc_span::source_map::SourceMap; +use rustc_span::{SourceFile, Span, Symbol}; +use std::sync::Arc; -pub(super) fn check(cx: &EarlyContext<'_>, item: &rustc_ast::Item) { - let mut has_outer = false; - let mut has_inner = false; +#[derive(Hash, PartialEq, Eq)] +enum SimpleAttrKind { + Doc, + /// A normal attribute, with its name symbols. + Normal(Vec), +} - for attr in &item.attrs { - if attr.span.from_expansion() { +impl From<&AttrKind> for SimpleAttrKind { + fn from(value: &AttrKind) -> Self { + match value { + AttrKind::Normal(attr) => { + let path_symbols = attr + .item + .path + .segments + .iter() + .map(|seg| seg.ident.name) + .collect::>(); + Self::Normal(path_symbols) + }, + AttrKind::DocComment(..) => Self::Doc, + } + } +} + +pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute]) { + let mut inner_attr_kind: FxHashSet = FxHashSet::default(); + let mut outer_attr_kind: FxHashSet = FxHashSet::default(); + + let source_map = cx.sess().source_map(); + let item_src = source_map.lookup_source_file(item_span.lo()); + + for attr in attrs { + if attr.span.from_expansion() || !attr_in_same_src_as_item(source_map, &item_src, attr.span) { continue; } + + let kind: SimpleAttrKind = (&attr.kind).into(); match attr.style { - AttrStyle::Inner => has_inner = true, - AttrStyle::Outer => has_outer = true, - } + AttrStyle::Inner => { + if outer_attr_kind.contains(&kind) { + lint_mixed_attrs(cx, attrs); + return; + } + inner_attr_kind.insert(kind); + }, + AttrStyle::Outer => { + if inner_attr_kind.contains(&kind) { + lint_mixed_attrs(cx, attrs); + return; + } + outer_attr_kind.insert(kind); + }, + }; } - if !has_outer || !has_inner { +} + +fn lint_mixed_attrs(cx: &LateContext<'_>, attrs: &[Attribute]) { + let mut attrs_iter = attrs.iter().filter(|attr| !attr.span.from_expansion()); + let span = if let (Some(first), Some(last)) = (attrs_iter.next(), attrs_iter.last()) { + first.span.with_hi(last.span.hi()) + } else { return; - } - let mut attrs_iter = item.attrs.iter().filter(|attr| !attr.span.from_expansion()); - let span = attrs_iter.next().unwrap().span; + }; span_lint( cx, MIXED_ATTRIBUTES_STYLE, - span.with_hi(attrs_iter.last().unwrap().span.hi()), + span, "item has both inner and outer attributes", ); } + +fn attr_in_same_src_as_item(source_map: &SourceMap, item_src: &Arc, attr_span: Span) -> bool { + let attr_src = source_map.lookup_source_file(attr_span.lo()); + Arc::ptr_eq(item_src, &attr_src) +} diff --git a/clippy_lints/src/attrs/mod.rs b/clippy_lints/src/attrs/mod.rs index 675c428948f..6f4575f1b9a 100644 --- a/clippy_lints/src/attrs/mod.rs +++ b/clippy_lints/src/attrs/mod.rs @@ -465,10 +465,20 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks that an item has only one kind of attributes. + /// Checks for items that have the same kind of attributes with mixed styles (inner/outer). /// /// ### Why is this bad? - /// Having both kinds of attributes makes it more complicated to read code. + /// Having both style of said attributes makes it more complicated to read code. + /// + /// ### Known problems + /// This lint currently has false-negatives when mixing same attributes + /// but they have different path symbols, for example: + /// ```ignore + /// #[custom_attribute] + /// pub fn foo() { + /// #![my_crate::custom_attribute] + /// } + /// ``` /// /// ### Example /// ```no_run @@ -523,6 +533,7 @@ declare_lint_pass!(Attributes => [ USELESS_ATTRIBUTE, BLANKET_CLIPPY_RESTRICTION_LINTS, SHOULD_PANIC_WITHOUT_EXPECT, + MIXED_ATTRIBUTES_STYLE, ]); impl<'tcx> LateLintPass<'tcx> for Attributes { @@ -566,6 +577,7 @@ impl<'tcx> LateLintPass<'tcx> for Attributes { ItemKind::ExternCrate(..) | ItemKind::Use(..) => useless_attribute::check(cx, item, attrs), _ => {}, } + mixed_attributes_style::check(cx, item.span, attrs); } fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) { @@ -594,7 +606,6 @@ impl_lint_pass!(EarlyAttributes => [ MAYBE_MISUSED_CFG, DEPRECATED_CLIPPY_CFG_ATTR, UNNECESSARY_CLIPPY_CFG, - MIXED_ATTRIBUTES_STYLE, DUPLICATED_ATTRIBUTES, ]); @@ -605,7 +616,6 @@ impl EarlyLintPass for EarlyAttributes { fn check_item(&mut self, cx: &EarlyContext<'_>, item: &rustc_ast::Item) { empty_line_after::check(cx, item); - mixed_attributes_style::check(cx, item); duplicated_attributes::check(cx, &item.attrs); } diff --git a/tests/ui/mixed_attributes_style.rs b/tests/ui/mixed_attributes_style.rs index 4f89aa8a5e5..1a646c26522 100644 --- a/tests/ui/mixed_attributes_style.rs +++ b/tests/ui/mixed_attributes_style.rs @@ -1,6 +1,12 @@ +//@aux-build:proc_macro_attr.rs +//@compile-flags: --test --cfg dummy_cfg +#![feature(custom_inner_attributes)] #![warn(clippy::mixed_attributes_style)] #![allow(clippy::duplicated_attributes)] +#[macro_use] +extern crate proc_macro_attr; + #[allow(unused)] //~ ERROR: item has both inner and outer attributes fn foo1() { #![allow(unused)] @@ -38,3 +44,57 @@ mod bar { fn main() { // test code goes here } + +// issue #12435 +#[cfg(test)] +mod tests { + //! Module doc, don't lint +} +#[allow(unused)] +mod baz { + //! Module doc, don't lint + const FOO: u8 = 0; +} +/// Module doc, don't lint +mod quz { + #![allow(unused)] +} + +mod issue_12530 { + // don't lint different attributes entirely + #[cfg(test)] + mod tests { + #![allow(clippy::unreadable_literal)] + + #[allow(dead_code)] //~ ERROR: item has both inner and outer attributes + mod inner_mod { + #![allow(dead_code)] + } + } + #[cfg(dummy_cfg)] + mod another_mod { + #![allow(clippy::question_mark)] + } + /// Nested mod + mod nested_mod { + #[allow(dead_code)] //~ ERROR: item has both inner and outer attributes + mod inner_mod { + #![allow(dead_code)] + } + } + /// Nested mod //~ ERROR: item has both inner and outer attributes + #[allow(unused)] + mod nest_mod_2 { + #![allow(unused)] + + #[allow(dead_code)] //~ ERROR: item has both inner and outer attributes + mod inner_mod { + #![allow(dead_code)] + } + } + // Different path symbols - Known FN + #[dummy] + fn use_dummy() { + #![proc_macro_attr::dummy] + } +} diff --git a/tests/ui/mixed_attributes_style.stderr b/tests/ui/mixed_attributes_style.stderr index ed798073cb7..a1d3fc430f6 100644 --- a/tests/ui/mixed_attributes_style.stderr +++ b/tests/ui/mixed_attributes_style.stderr @@ -1,5 +1,5 @@ error: item has both inner and outer attributes - --> tests/ui/mixed_attributes_style.rs:4:1 + --> tests/ui/mixed_attributes_style.rs:10:1 | LL | / #[allow(unused)] LL | | fn foo1() { @@ -10,7 +10,7 @@ LL | | #![allow(unused)] = help: to override `-D warnings` add `#[allow(clippy::mixed_attributes_style)]` error: item has both inner and outer attributes - --> tests/ui/mixed_attributes_style.rs:18:1 + --> tests/ui/mixed_attributes_style.rs:24:1 | LL | / /// linux LL | | @@ -19,12 +19,45 @@ LL | | //! windows | |_______________^ error: item has both inner and outer attributes - --> tests/ui/mixed_attributes_style.rs:33:1 + --> tests/ui/mixed_attributes_style.rs:39:1 | LL | / #[allow(unused)] LL | | mod bar { LL | | #![allow(unused)] | |_____________________^ -error: aborting due to 3 previous errors +error: item has both inner and outer attributes + --> tests/ui/mixed_attributes_style.rs:69:9 + | +LL | / #[allow(dead_code)] +LL | | mod inner_mod { +LL | | #![allow(dead_code)] + | |________________________________^ + +error: item has both inner and outer attributes + --> tests/ui/mixed_attributes_style.rs:80:9 + | +LL | / #[allow(dead_code)] +LL | | mod inner_mod { +LL | | #![allow(dead_code)] + | |________________________________^ + +error: item has both inner and outer attributes + --> tests/ui/mixed_attributes_style.rs:85:5 + | +LL | / /// Nested mod +LL | | #[allow(unused)] +LL | | mod nest_mod_2 { +LL | | #![allow(unused)] + | |_________________________^ + +error: item has both inner and outer attributes + --> tests/ui/mixed_attributes_style.rs:90:9 + | +LL | / #[allow(dead_code)] +LL | | mod inner_mod { +LL | | #![allow(dead_code)] + | |________________________________^ + +error: aborting due to 7 previous errors diff --git a/tests/ui/mixed_attributes_style/auxiliary/submodule.rs b/tests/ui/mixed_attributes_style/auxiliary/submodule.rs new file mode 100644 index 00000000000..df44b07a694 --- /dev/null +++ b/tests/ui/mixed_attributes_style/auxiliary/submodule.rs @@ -0,0 +1,9 @@ +//! Module level doc + +#![allow(dead_code)] + +#[allow(unused)] +//~^ ERROR: item has both inner and outer attributes +mod foo { + #![allow(dead_code)] +} diff --git a/tests/ui/mixed_attributes_style/global_allow.rs b/tests/ui/mixed_attributes_style/global_allow.rs new file mode 100644 index 00000000000..153262e6557 --- /dev/null +++ b/tests/ui/mixed_attributes_style/global_allow.rs @@ -0,0 +1,7 @@ +// issue 12436 +#![allow(clippy::mixed_attributes_style)] + +#[path = "auxiliary/submodule.rs"] +mod submodule; + +fn main() {} diff --git a/tests/ui/mixed_attributes_style/mod_declaration.rs b/tests/ui/mixed_attributes_style/mod_declaration.rs new file mode 100644 index 00000000000..b0f1f0bda9e --- /dev/null +++ b/tests/ui/mixed_attributes_style/mod_declaration.rs @@ -0,0 +1,3 @@ +#[path = "auxiliary/submodule.rs"] // don't lint. +/// This doc comment should not lint, it could be used to add context to the original module doc +mod submodule; diff --git a/tests/ui/mixed_attributes_style/mod_declaration.stderr b/tests/ui/mixed_attributes_style/mod_declaration.stderr new file mode 100644 index 00000000000..968c537c7e4 --- /dev/null +++ b/tests/ui/mixed_attributes_style/mod_declaration.stderr @@ -0,0 +1,14 @@ +error: item has both inner and outer attributes + --> tests/ui/mixed_attributes_style/auxiliary/submodule.rs:5:1 + | +LL | / #[allow(unused)] +LL | | +LL | | mod foo { +LL | | #![allow(dead_code)] + | |________________________^ + | + = note: `-D clippy::mixed-attributes-style` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::mixed_attributes_style)]` + +error: aborting due to 1 previous error +