Auto merge of rust-lang#12486 - J-ZhengLi:issue12435, r=y21

don't lint [`mixed_attributes_style`] when mixing docs and other attrs

fixes: rust-lang#12435
fixes: rust-lang#12436
fixes: rust-lang#12530

---

changelog: don't lint [`mixed_attributes_style`] when mixing different kind of attrs; and move it to late pass;

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<Symbol>),
+}
+
+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::<Vec<_>>();
+                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<SimpleAttrKind> = FxHashSet::default();
+    let mut outer_attr_kind: FxHashSet<SimpleAttrKind> = FxHashSet::default();
+
+    let source_map = cx.sess().source_map();
+    let item_src = source_map.lookup_source_file(item_span.lo());
 
-    for attr in &item.attrs {
-        if attr.span.from_expansion() {
+    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<SourceFile>, attr_span: Span) -> bool {
+    let attr_src = source_map.lookup_source_file(attr_span.lo());
+    Arc::ptr_eq(item_src, &attr_src)
+}

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);
     }
 

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]
+    }
+}

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
 

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)]
+}

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() {}

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;

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
+