Auto merge of #8219 - camsteffen:macro-decoupling, r=llogiq

New macro utils

changelog: none

Sorry, this is a big one. A lot of interrelated changes and I wanted to put the new utils to use to make sure they are somewhat battle-tested. We may want to divide some of the lint-specific refactoring commits into batches for smaller reviewing tasks. I could also split into more PRs.

Introduces a bunch of new utils at `clippy_utils::macros::...`. Please read through the docs and give any feedback! I'm happy to introduce `MacroCall` and various functions to retrieve an instance. It feels like the missing puzzle piece. I'm also introducing `ExpnId` from rustc as "useful for Clippy too". `@rust-lang/clippy`

Fixes #7843 by not parsing every node of macro implementations, at least the major offenders.

I probably want to get rid of `is_expn_of` at some point.

Author: bors

clippy_lints/src/assertions_on_constants.rs

@@ -1,12 +1,10 @@
 use clippy_utils::consts::{constant, Constant};
 use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::higher;
-use clippy_utils::source::snippet_opt;
-use clippy_utils::{is_direct_expn_of, is_expn_of, match_panic_call, peel_blocks};
-use if_chain::if_chain;
-use rustc_hir::{Expr, ExprKind, UnOp};
+use clippy_utils::macros::{find_assert_args, root_macro_call_first_node, PanicExpn};
+use rustc_hir::Expr;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -36,107 +34,39 @@ declare_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]);
 
 impl<'tcx> LateLintPass<'tcx> for AssertionsOnConstants {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
-        let lint_true = |is_debug: bool| {
-            span_lint_and_help(
-                cx,
-                ASSERTIONS_ON_CONSTANTS,
-                e.span,
-                if is_debug {
-                    "`debug_assert!(true)` will be optimized out by the compiler"
-                } else {
-                    "`assert!(true)` will be optimized out by the compiler"
-                },
-                None,
-                "remove it",
-            );
+        let Some(macro_call) = root_macro_call_first_node(cx, e) else { return };
+        let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) {
+            Some(sym::debug_assert_macro) => true,
+            Some(sym::assert_macro) => false,
+            _ => return,
         };
-        let lint_false_without_message = || {
+        let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) else { return };
+        let Some((Constant::Bool(val), _)) = constant(cx, cx.typeck_results(), condition) else { return };
+        if val {
             span_lint_and_help(
                 cx,
                 ASSERTIONS_ON_CONSTANTS,
-                e.span,
-                "`assert!(false)` should probably be replaced",
+                macro_call.span,
+                &format!(
+                    "`{}!(true)` will be optimized out by the compiler",
+                    cx.tcx.item_name(macro_call.def_id)
+                ),
                 None,
-                "use `panic!()` or `unreachable!()`",
+                "remove it",
             );
-        };
-        let lint_false_with_message = |panic_message: String| {
+        } else if !is_debug {
+            let (assert_arg, panic_arg) = match panic_expn {
+                PanicExpn::Empty => ("", ""),
+                _ => (", ..", ".."),
+            };
             span_lint_and_help(
                 cx,
                 ASSERTIONS_ON_CONSTANTS,
-                e.span,
-                &format!("`assert!(false, {})` should probably be replaced", panic_message),
+                macro_call.span,
+                &format!("`assert!(false{})` should probably be replaced", assert_arg),
                 None,
-                &format!("use `panic!({})` or `unreachable!({})`", panic_message, panic_message),
+                &format!("use `panic!({})` or `unreachable!({0})`", panic_arg),
             );
-        };
-
-        if let Some(debug_assert_span) = is_expn_of(e.span, "debug_assert") {
-            if debug_assert_span.from_expansion() {
-                return;
-            }
-            if_chain! {
-                if let ExprKind::Unary(_, lit) = e.kind;
-                if let Some((Constant::Bool(is_true), _)) = constant(cx, cx.typeck_results(), lit);
-                if is_true;
-                then {
-                    lint_true(true);
-                }
-            };
-        } else if let Some(assert_span) = is_direct_expn_of(e.span, "assert") {
-            if assert_span.from_expansion() {
-                return;
-            }
-            if let Some(assert_match) = match_assert_with_message(cx, e) {
-                match assert_match {
-                    // matched assert but not message
-                    AssertKind::WithoutMessage(false) => lint_false_without_message(),
-                    AssertKind::WithoutMessage(true) | AssertKind::WithMessage(_, true) => lint_true(false),
-                    AssertKind::WithMessage(panic_message, false) => lint_false_with_message(panic_message),
-                };
-            }
-        }
-    }
-}
-
-/// Result of calling `match_assert_with_message`.
-enum AssertKind {
-    WithMessage(String, bool),
-    WithoutMessage(bool),
-}
-
-/// Check if the expression matches
-///
-/// ```rust,ignore
-/// if !c {
-///   {
-///     ::std::rt::begin_panic(message, _)
-///   }
-/// }
-/// ```
-///
-/// where `message` is any expression and `c` is a constant bool.
-fn match_assert_with_message<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<AssertKind> {
-    if_chain! {
-        if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr);
-        if let ExprKind::Unary(UnOp::Not, expr) = cond.kind;
-        // bind the first argument of the `assert!` macro
-        if let Some((Constant::Bool(is_true), _)) = constant(cx, cx.typeck_results(), expr);
-        let begin_panic_call = peel_blocks(then);
-        // function call
-        if let Some(arg) = match_panic_call(cx, begin_panic_call);
-        // bind the second argument of the `assert!` macro if it exists
-        if let panic_message = snippet_opt(cx, arg.span);
-        // second argument of begin_panic is irrelevant
-        // as is the second match arm
-        then {
-            // an empty message occurs when it was generated by the macro
-            // (and not passed by the user)
-            return panic_message
-                .filter(|msg| !msg.is_empty())
-                .map(|msg| AssertKind::WithMessage(msg, is_true))
-                .or(Some(AssertKind::WithoutMessage(is_true)));
         }
     }
-    None
 }

clippy_lints/src/attrs.rs

@@ -1,9 +1,10 @@
 //! 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;
 use clippy_utils::source::{first_line_of_span, is_present_in_source, snippet_opt, without_block_comments};
-use clippy_utils::{extract_msrv_attr, match_panic_def_id, meets_msrv};
+use clippy_utils::{extract_msrv_attr, meets_msrv};
 use if_chain::if_chain;
 use rustc_ast::{AttrKind, AttrStyle, Attribute, Lit, LitKind, MetaItemKind, NestedMetaItem};
 use rustc_errors::Applicability;
@@ -443,20 +444,15 @@ fn is_relevant_block(cx: &LateContext<'_>, typeck_results: &ty::TypeckResults<'_
 }
 
 fn is_relevant_expr(cx: &LateContext<'_>, typeck_results: &ty::TypeckResults<'_>, expr: &Expr<'_>) -> bool {
+    if macro_backtrace(expr.span).last().map_or(false, |macro_call| {
+        is_panic(cx, macro_call.def_id) || cx.tcx.item_name(macro_call.def_id) == sym::unreachable
+    }) {
+        return false;
+    }
     match &expr.kind {
         ExprKind::Block(block, _) => is_relevant_block(cx, typeck_results, block),
         ExprKind::Ret(Some(e)) => is_relevant_expr(cx, typeck_results, e),
         ExprKind::Ret(None) | ExprKind::Break(_, None) => false,
-        ExprKind::Call(path_expr, _) => {
-            if let ExprKind::Path(qpath) = &path_expr.kind {
-                typeck_results
-                    .qpath_res(qpath, path_expr.hir_id)
-                    .opt_def_id()
-                    .map_or(true, |fun_id| !match_panic_def_id(cx, fun_id))
-            } else {
-                true
-            }
-        },
         _ => true,
     }
 }

clippy_lints/src/bool_assert_comparison.rs

@@ -1,4 +1,5 @@
-use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, ty::implements_trait};
+use clippy_utils::macros::{find_assert_eq_args, root_macro_call_first_node};
+use clippy_utils::{diagnostics::span_lint_and_sugg, ty::implements_trait};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, Lit};
@@ -66,44 +67,40 @@ fn is_impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) ->
 
 impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        let macros = ["assert_eq", "debug_assert_eq"];
-        let inverted_macros = ["assert_ne", "debug_assert_ne"];
-
-        for mac in macros.iter().chain(inverted_macros.iter()) {
-            if let Some(span) = is_direct_expn_of(expr.span, mac) {
-                if let Some(args) = higher::extract_assert_macro_args(expr) {
-                    if let [a, b, ..] = args[..] {
-                        let nb_bool_args = usize::from(is_bool_lit(a)) + usize::from(is_bool_lit(b));
-
-                        if nb_bool_args != 1 {
-                            // If there are two boolean arguments, we definitely don't understand
-                            // what's going on, so better leave things as is...
-                            //
-                            // Or there is simply no boolean and then we can leave things as is!
-                            return;
-                        }
-
-                        if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) {
-                            // At this point the expression which is not a boolean
-                            // literal does not implement Not trait with a bool output,
-                            // so we cannot suggest to rewrite our code
-                            return;
-                        }
+        let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return };
+        let macro_name = cx.tcx.item_name(macro_call.def_id);
+        if !matches!(
+            macro_name.as_str(),
+            "assert_eq" | "debug_assert_eq" | "assert_ne" | "debug_assert_ne"
+        ) {
+            return;
+        }
+        let Some ((a, b, _)) = find_assert_eq_args(cx, expr, macro_call.expn) else { return };
+        if !(is_bool_lit(a) ^ is_bool_lit(b)) {
+            // If there are two boolean arguments, we definitely don't understand
+            // what's going on, so better leave things as is...
+            //
+            // Or there is simply no boolean and then we can leave things as is!
+            return;
+        }
 
-                        let non_eq_mac = &mac[..mac.len() - 3];
-                        span_lint_and_sugg(
-                            cx,
-                            BOOL_ASSERT_COMPARISON,
-                            span,
-                            &format!("used `{}!` with a literal bool", mac),
-                            "replace it with",
-                            format!("{}!(..)", non_eq_mac),
-                            Applicability::MaybeIncorrect,
-                        );
-                        return;
-                    }
-                }
-            }
+        if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) {
+            // At this point the expression which is not a boolean
+            // literal does not implement Not trait with a bool output,
+            // so we cannot suggest to rewrite our code
+            return;
         }
+
+        let macro_name = macro_name.as_str();
+        let non_eq_mac = &macro_name[..macro_name.len() - 3];
+        span_lint_and_sugg(
+            cx,
+            BOOL_ASSERT_COMPARISON,
+            macro_call.span,
+            &format!("used `{}!` with a literal bool", macro_name),
+            "replace it with",
+            format!("{}!(..)", non_eq_mac),
+            Applicability::MaybeIncorrect,
+        );
     }
 }

clippy_lints/src/doc.rs

@@ -1,8 +1,9 @@
 use clippy_utils::attrs::is_doc_hidden;
 use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_then};
+use clippy_utils::macros::{is_panic, root_macro_call_first_node};
 use clippy_utils::source::{first_line_of_span, snippet_with_applicability};
 use clippy_utils::ty::{implements_trait, is_type_diagnostic_item};
-use clippy_utils::{is_entrypoint_fn, is_expn_of, match_panic_def_id, method_chain_args, return_ty};
+use clippy_utils::{is_entrypoint_fn, method_chain_args, return_ty};
 use if_chain::if_chain;
 use itertools::Itertools;
 use rustc_ast::ast::{Async, AttrKind, Attribute, Fn, FnRetTy, ItemKind};
@@ -13,7 +14,7 @@ use rustc_errors::emitter::EmitterWriter;
 use rustc_errors::{Applicability, Handler, SuggestionStyle};
 use rustc_hir as hir;
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
-use rustc_hir::{AnonConst, Expr, ExprKind, QPath};
+use rustc_hir::{AnonConst, Expr};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
@@ -805,24 +806,17 @@ impl<'a, 'tcx> Visitor<'tcx> for FindPanicUnwrap<'a, 'tcx> {
             return;
         }
 
-        // check for `begin_panic`
-        if_chain! {
-            if let ExprKind::Call(func_expr, _) = expr.kind;
-            if let ExprKind::Path(QPath::Resolved(_, path)) = func_expr.kind;
-            if let Some(path_def_id) = path.res.opt_def_id();
-            if match_panic_def_id(self.cx, path_def_id);
-            if is_expn_of(expr.span, "unreachable").is_none();
-            if !is_expn_of_debug_assertions(expr.span);
-            then {
-                self.panic_span = Some(expr.span);
+        if let Some(macro_call) = root_macro_call_first_node(self.cx, expr) {
+            if is_panic(self.cx, macro_call.def_id)
+                || matches!(
+                    self.cx.tcx.item_name(macro_call.def_id).as_str(),
+                    "assert" | "assert_eq" | "assert_ne" | "todo"
+                )
+            {
+                self.panic_span = Some(macro_call.span);
             }
         }
 
-        // check for `assert_eq` or `assert_ne`
-        if is_expn_of(expr.span, "assert_eq").is_some() || is_expn_of(expr.span, "assert_ne").is_some() {
-            self.panic_span = Some(expr.span);
-        }
-
         // check for `unwrap`
         if let Some(arglists) = method_chain_args(expr, &["unwrap"]) {
             let receiver_ty = self.typeck_results.expr_ty(&arglists[0][0]).peel_refs();
@@ -844,8 +838,3 @@ impl<'a, 'tcx> Visitor<'tcx> for FindPanicUnwrap<'a, 'tcx> {
         NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
     }
 }
-
-fn is_expn_of_debug_assertions(span: Span) -> bool {
-    const MACRO_NAMES: &[&str] = &["debug_assert", "debug_assert_eq", "debug_assert_ne"];
-    MACRO_NAMES.iter().any(|name| is_expn_of(span, name).is_some())
-}

clippy_lints/src/eq_op.rs

@@ -1,10 +1,11 @@
 use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then};
+use clippy_utils::macros::{find_assert_eq_args, first_node_macro_backtrace};
 use clippy_utils::source::snippet;
 use clippy_utils::ty::{implements_trait, is_copy};
-use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher, is_expn_of, is_in_test_function};
+use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, is_in_test_function};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, StmtKind};
+use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
@@ -68,32 +69,26 @@ declare_clippy_lint! {
 
 declare_lint_pass!(EqOp => [EQ_OP, OP_REF]);
 
-const ASSERT_MACRO_NAMES: [&str; 4] = ["assert_eq", "assert_ne", "debug_assert_eq", "debug_assert_ne"];
-
 impl<'tcx> LateLintPass<'tcx> for EqOp {
     #[allow(clippy::similar_names, clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
-        if let ExprKind::Block(block, _) = e.kind {
-            for stmt in block.stmts {
-                for amn in &ASSERT_MACRO_NAMES {
-                    if_chain! {
-                        if is_expn_of(stmt.span, amn).is_some();
-                        if let StmtKind::Semi(matchexpr) = stmt.kind;
-                        if let Some(macro_args) = higher::extract_assert_macro_args(matchexpr);
-                        if macro_args.len() == 2;
-                        let (lhs, rhs) = (macro_args[0], macro_args[1]);
-                        if eq_expr_value(cx, lhs, rhs);
-                        if !is_in_test_function(cx.tcx, e.hir_id);
-                        then {
-                            span_lint(
-                                cx,
-                                EQ_OP,
-                                lhs.span.to(rhs.span),
-                                &format!("identical args used in this `{}!` macro call", amn),
-                            );
-                        }
-                    }
-                }
+        if_chain! {
+            if let Some((macro_call, macro_name)) = first_node_macro_backtrace(cx, e).find_map(|macro_call| {
+                let name = cx.tcx.item_name(macro_call.def_id);
+                matches!(name.as_str(), "assert_eq" | "assert_ne" | "debug_assert_eq" | "debug_assert_ne")
+                    .then(|| (macro_call, name))
+            });
+            if let Some((lhs, rhs, _)) = find_assert_eq_args(cx, e, macro_call.expn);
+            if eq_expr_value(cx, lhs, rhs);
+            if macro_call.is_local();
+            if !is_in_test_function(cx.tcx, e.hir_id);
+            then {
+                span_lint(
+                    cx,
+                    EQ_OP,
+                    lhs.span.to(rhs.span),
+                    &format!("identical args used in this `{}!` macro call", macro_name),
+                );
             }
         }
         if let ExprKind::Binary(op, left, right) = e.kind {