Auto merge of #10869 - Centri3:allow_attributes, r=Manishearth

[`allow_attributes`, `allow_attributes_without_reason`]: Ignore attributes from procedural macros

I use `lint_reasons` and `clap`, which is a bit overzealous when it comes to preventing warnings in its macros; it uses a ton of allow attributes on everything to, as ironic as it is, silence warnings. These two now ignore anything from procedural macros.

PS, I think `allow_attributes.rs` should be merged with `attrs.rs` in the future.

fixes #10377

changelog: [`allow_attributes`, `allow_attributes_without_reason`]: Ignore attributes from procedural macros

clippy_lints/src/allow_attributes.rs

@@ -1,5 +1,5 @@
-use ast::AttrStyle;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use ast::{AttrStyle, Attribute};
+use clippy_utils::{diagnostics::span_lint_and_sugg, is_from_proc_macro};
 use rustc_ast as ast;
 use rustc_errors::Applicability;
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -50,13 +50,14 @@
 
 impl LateLintPass<'_> for AllowAttribute {
     // Separate each crate's features.
-    fn check_attribute(&mut self, cx: &LateContext<'_>, attr: &ast::Attribute) {
+    fn check_attribute<'cx>(&mut self, cx: &LateContext<'cx>, attr: &'cx Attribute) {
         if_chain! {
             if !in_external_macro(cx.sess(), attr.span);
             if cx.tcx.features().lint_reasons;
             if let AttrStyle::Outer = attr.style;
             if let Some(ident) = attr.ident();
             if ident.name == rustc_span::symbol::sym::allow;
+            if !is_from_proc_macro(cx, &attr);
             then {
                 span_lint_and_sugg(
                     cx,

clippy_lints/src/attrs.rs

@@ -1,9 +1,12 @@
 //! checks for attributes
 
-use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::macros::{is_panic, macro_backtrace};
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::{first_line_of_span, is_present_in_source, snippet_opt, without_block_comments};
+use clippy_utils::{
+    diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then},
+    is_from_proc_macro,
+};
 use if_chain::if_chain;
 use rustc_ast::{AttrKind, AttrStyle, Attribute, LitKind, MetaItemKind, MetaItemLit, NestedMetaItem};
 use rustc_errors::Applicability;
@@ -566,7 +569,7 @@ fn check_clippy_lint_names(cx: &LateContext<'_>, name: Symbol, items: &[NestedMe
     }
 }
 
-fn check_lint_reason(cx: &LateContext<'_>, name: Symbol, items: &[NestedMetaItem], attr: &'_ Attribute) {
+fn check_lint_reason<'cx>(cx: &LateContext<'cx>, name: Symbol, items: &[NestedMetaItem], attr: &'cx Attribute) {
     // Check for the feature
     if !cx.tcx.features().lint_reasons {
         return;
@@ -581,7 +584,7 @@ fn check_lint_reason(cx: &LateContext<'_>, name: Symbol, items: &[NestedMetaItem
     }
 
     // Check if the attribute is in an external macro and therefore out of the developer's control
-    if in_external_macro(cx.sess(), attr.span) {
+    if in_external_macro(cx.sess(), attr.span) || is_from_proc_macro(cx, &attr) {
         return;
     }
 

clippy_utils/src/check_proc_macro.rs

@@ -12,7 +12,11 @@
 //! code was written, and check if the span contains that text. Note this will only work correctly
 //! if the span is not from a `macro_rules` based macro.
 
-use rustc_ast::ast::{IntTy, LitIntType, LitKind, StrStyle, UintTy};
+use rustc_ast::{
+    ast::{AttrKind, Attribute, IntTy, LitIntType, LitKind, StrStyle, UintTy},
+    token::CommentKind,
+    AttrStyle,
+};
 use rustc_hir::{
     intravisit::FnKind, Block, BlockCheckMode, Body, Closure, Destination, Expr, ExprKind, FieldDef, FnHeader, HirId,
     Impl, ImplItem, ImplItemKind, IsAuto, Item, ItemKind, LoopSource, MatchSource, Node, QPath, TraitItem,
@@ -25,12 +29,16 @@
 use rustc_target::spec::abi::Abi;
 
 /// The search pattern to look for. Used by `span_matches_pat`
-#[derive(Clone, Copy)]
+#[derive(Clone)]
 pub enum Pat {
     /// A single string.
     Str(&'static str),
+    /// A single string.
+    OwnedStr(String),
     /// Any of the given strings.
     MultiStr(&'static [&'static str]),
+    /// Any of the given strings.
+    OwnedMultiStr(Vec<String>),
     /// The string representation of the symbol.
     Sym(Symbol),
     /// Any decimal or hexadecimal digit depending on the location.
@@ -51,12 +59,16 @@ fn span_matches_pat(sess: &Session, span: Span, start_pat: Pat, end_pat: Pat) ->
         let end_str = s.trim_end_matches(|c: char| c.is_whitespace() || c == ')' || c == ',');
         (match start_pat {
             Pat::Str(text) => start_str.starts_with(text),
+            Pat::OwnedStr(text) => start_str.starts_with(&text),
             Pat::MultiStr(texts) => texts.iter().any(|s| start_str.starts_with(s)),
+            Pat::OwnedMultiStr(texts) => texts.iter().any(|s| start_str.starts_with(s)),
             Pat::Sym(sym) => start_str.starts_with(sym.as_str()),
             Pat::Num => start_str.as_bytes().first().map_or(false, u8::is_ascii_digit),
         } && match end_pat {
             Pat::Str(text) => end_str.ends_with(text),
+            Pat::OwnedStr(text) => end_str.starts_with(&text),
             Pat::MultiStr(texts) => texts.iter().any(|s| start_str.ends_with(s)),
+            Pat::OwnedMultiStr(texts) => texts.iter().any(|s| start_str.starts_with(s)),
             Pat::Sym(sym) => end_str.ends_with(sym.as_str()),
             Pat::Num => end_str.as_bytes().last().map_or(false, u8::is_ascii_hexdigit),
         })
@@ -271,6 +283,42 @@ fn fn_kind_pat(tcx: TyCtxt<'_>, kind: &FnKind<'_>, body: &Body<'_>, hir_id: HirI
     (start_pat, end_pat)
 }
 
+fn attr_search_pat(attr: &Attribute) -> (Pat, Pat) {
+    match attr.kind {
+        AttrKind::Normal(..) => {
+            let mut pat = if matches!(attr.style, AttrStyle::Outer) {
+                (Pat::Str("#["), Pat::Str("]"))
+            } else {
+                (Pat::Str("#!["), Pat::Str("]"))
+            };
+
+            if let Some(ident) = attr.ident() && let Pat::Str(old_pat) = pat.0 {
+                // TODO: I feel like it's likely we can use `Cow` instead but this will require quite a bit of
+                // refactoring
+                // NOTE: This will likely have false positives, like `allow = 1`
+                pat.0 = Pat::OwnedMultiStr(vec![ident.to_string(), old_pat.to_owned()]);
+                pat.1 = Pat::Str("");
+            }
+
+            pat
+        },
+        AttrKind::DocComment(_kind @ CommentKind::Line, ..) => {
+            if matches!(attr.style, AttrStyle::Outer) {
+                (Pat::Str("///"), Pat::Str(""))
+            } else {
+                (Pat::Str("//!"), Pat::Str(""))
+            }
+        },
+        AttrKind::DocComment(_kind @ CommentKind::Block, ..) => {
+            if matches!(attr.style, AttrStyle::Outer) {
+                (Pat::Str("/**"), Pat::Str("*/"))
+            } else {
+                (Pat::Str("/*!"), Pat::Str("*/"))
+            }
+        },
+    }
+}
+
 pub trait WithSearchPat {
     type Context: LintContext;
     fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat);
@@ -310,6 +358,19 @@ fn span(&self) -> Span {
     }
 }
 
+// `Attribute` does not have the `hir` associated lifetime, so we cannot use the macro
+impl<'cx> WithSearchPat for &'cx Attribute {
+    type Context = LateContext<'cx>;
+
+    fn search_pat(&self, _cx: &Self::Context) -> (Pat, Pat) {
+        attr_search_pat(self)
+    }
+
+    fn span(&self) -> Span {
+        self.span
+    }
+}
+
 /// Checks if the item likely came from a proc-macro.
 ///
 /// This should be called after `in_external_macro` and the initial pattern matching of the ast as

tests/ui/allow_attributes.fixed

@@ -1,9 +1,12 @@
 //@run-rustfix
+//@aux-build:proc_macros.rs
 #![allow(unused)]
 #![warn(clippy::allow_attributes)]
 #![feature(lint_reasons)]
+#![no_main]
 
-fn main() {}
+extern crate proc_macros;
+use proc_macros::{external, with_span};
 
 // Using clippy::needless_borrow just as a placeholder, it isn't relevant.
 
@@ -20,6 +23,21 @@ struct T4;
 #[cfg_attr(panic = "unwind", expect(dead_code))]
 struct CfgT;
 
+fn ignore_external() {
+    external! {
+        #[allow(clippy::needless_borrow)] // Should not lint
+        fn a() {}
+    }
+}
+
+fn ignore_proc_macro() {
+    with_span! {
+        span
+        #[allow(clippy::needless_borrow)] // Should not lint
+        fn a() {}
+    }
+}
+
 fn ignore_inner_attr() {
     #![allow(unused)] // Should not lint
 }

tests/ui/allow_attributes.rs

@@ -1,9 +1,12 @@
 //@run-rustfix
+//@aux-build:proc_macros.rs
 #![allow(unused)]
 #![warn(clippy::allow_attributes)]
 #![feature(lint_reasons)]
+#![no_main]
 
-fn main() {}
+extern crate proc_macros;
+use proc_macros::{external, with_span};
 
 // Using clippy::needless_borrow just as a placeholder, it isn't relevant.
 
@@ -20,6 +23,21 @@ fn main() {}
 #[cfg_attr(panic = "unwind", allow(dead_code))]
 struct CfgT;
 
+fn ignore_external() {
+    external! {
+        #[allow(clippy::needless_borrow)] // Should not lint
+        fn a() {}
+    }
+}
+
+fn ignore_proc_macro() {
+    with_span! {
+        span
+        #[allow(clippy::needless_borrow)] // Should not lint
+        fn a() {}
+    }
+}
+
 fn ignore_inner_attr() {
     #![allow(unused)] // Should not lint
 }

tests/ui/allow_attributes.stderr

@@ -1,5 +1,5 @@
 error: #[allow] attribute found
-  --> $DIR/allow_attributes.rs:11:3
+  --> $DIR/allow_attributes.rs:14:3
    |
 LL | #[allow(dead_code)]
    |   ^^^^^ help: replace it with: `expect`
@@ -7,7 +7,7 @@ LL | #[allow(dead_code)]
    = note: `-D clippy::allow-attributes` implied by `-D warnings`
 
 error: #[allow] attribute found
-  --> $DIR/allow_attributes.rs:20:30
+  --> $DIR/allow_attributes.rs:23:30
    |
 LL | #[cfg_attr(panic = "unwind", allow(dead_code))]
    |                              ^^^^^ help: replace it with: `expect`

tests/ui/allow_attributes_false_positive.rs

@@ -1,5 +0,0 @@
-#![warn(clippy::allow_attributes)]
-#![feature(lint_reasons)]
-#![crate_type = "proc-macro"]
-
-fn main() {}

tests/ui/allow_attributes_without_reason.rs

@@ -1,9 +1,15 @@
+//@aux-build:proc_macros.rs
 #![feature(lint_reasons)]
 #![deny(clippy::allow_attributes_without_reason)]
+#![allow(unfulfilled_lint_expectations)]
+
+extern crate proc_macros;
+use proc_macros::{external, with_span};
 
 // These should trigger the lint
 #[allow(dead_code)]
 #[allow(dead_code, deprecated)]
+#[expect(dead_code)]
 // These should be fine
 #[allow(dead_code, reason = "This should be allowed")]
 #[warn(dyn_drop, reason = "Warnings can also have reasons")]
@@ -11,4 +17,28 @@
 #[deny(deref_nullptr)]
 #[forbid(deref_nullptr)]
 
-fn main() {}
+fn main() {
+    external! {
+        #[allow(dead_code)]
+        fn a() {}
+    }
+    with_span! {
+        span
+        #[allow(dead_code)]
+        fn b() {}
+    }
+}
+
+// Make sure this is not triggered on `?` desugaring
+
+pub fn trigger_fp_option() -> Option<()> {
+    Some(())?;
+    None?;
+    Some(())
+}
+
+pub fn trigger_fp_result() -> Result<(), &'static str> {
+    Ok(())?;
+    Err("asdf")?;
+    Ok(())
+}

tests/ui/allow_attributes_without_reason.stderr

@@ -1,23 +1,39 @@
 error: `allow` attribute without specifying a reason
-  --> $DIR/allow_attributes_without_reason.rs:5:1
+  --> $DIR/allow_attributes_without_reason.rs:4:1
    |
-LL | #[allow(dead_code)]
-   | ^^^^^^^^^^^^^^^^^^^
+LL | #![allow(unfulfilled_lint_expectations)]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: try adding a reason at the end with `, reason = ".."`
 note: the lint level is defined here
-  --> $DIR/allow_attributes_without_reason.rs:2:9
+  --> $DIR/allow_attributes_without_reason.rs:3:9
    |
 LL | #![deny(clippy::allow_attributes_without_reason)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: `allow` attribute without specifying a reason
-  --> $DIR/allow_attributes_without_reason.rs:6:1
+  --> $DIR/allow_attributes_without_reason.rs:10:1
+   |
+LL | #[allow(dead_code)]
+   | ^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try adding a reason at the end with `, reason = ".."`
+
+error: `allow` attribute without specifying a reason
+  --> $DIR/allow_attributes_without_reason.rs:11:1
    |
 LL | #[allow(dead_code, deprecated)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: try adding a reason at the end with `, reason = ".."`
 
-error: aborting due to 2 previous errors
+error: `expect` attribute without specifying a reason
+  --> $DIR/allow_attributes_without_reason.rs:12:1
+   |
+LL | #[expect(dead_code)]
+   | ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try adding a reason at the end with `, reason = ".."`
+
+error: aborting due to 4 previous errors