From 1fe884401aa12b08408b101cf931cb396d0ccf3f Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 15 Jan 2024 17:51:29 +0800 Subject: [PATCH] fix [`dbg_macro`] FN when `dbg` is inside some complex macros --- clippy_lints/src/dbg_macro.rs | 141 +++++++++++++++------------- tests/ui/dbg_macro/dbg_macro.rs | 9 ++ tests/ui/dbg_macro/dbg_macro.stderr | 24 ++++- 3 files changed, 108 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/dbg_macro.rs b/clippy_lints/src/dbg_macro.rs index ec66556ce..a517d7bd6 100644 --- a/clippy_lints/src/dbg_macro.rs +++ b/clippy_lints/src/dbg_macro.rs @@ -1,12 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::macros::root_macro_call_first_node; +use clippy_utils::macros::root_macro_call; use clippy_utils::source::snippet_with_applicability; use clippy_utils::{is_in_cfg_test, is_in_test_function}; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, Node}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::impl_lint_pass; -use rustc_span::sym; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does @@ -34,82 +34,93 @@ declare_clippy_lint! { #[derive(Copy, Clone)] pub struct DbgMacro { allow_dbg_in_tests: bool, + /// Keep tracking of the previous `dbg!` macro call site in order to + /// skip any other expressions from the same expansion, including nested macro calls. + prev_dbg_call_site: Span, } impl_lint_pass!(DbgMacro => [DBG_MACRO]); impl DbgMacro { pub fn new(allow_dbg_in_tests: bool) -> Self { - DbgMacro { allow_dbg_in_tests } + DbgMacro { + allow_dbg_in_tests, + prev_dbg_call_site: Span::default(), + } } } impl LateLintPass<'_> for DbgMacro { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { - let Some(macro_call) = root_macro_call_first_node(cx, expr) else { + let Some(macro_call) = + root_macro_call(expr.span).filter(|mc| cx.tcx.is_diagnostic_item(sym::dbg_macro, mc.def_id)) + else { return; }; - if cx.tcx.is_diagnostic_item(sym::dbg_macro, macro_call.def_id) { - // allows `dbg!` in test code if allow-dbg-in-test is set to true in clippy.toml - if self.allow_dbg_in_tests - && (is_in_test_function(cx.tcx, expr.hir_id) || is_in_cfg_test(cx.tcx, expr.hir_id)) - { - return; - } - let mut applicability = Applicability::MachineApplicable; - - let (sugg_span, suggestion) = match expr.peel_drop_temps().kind { - // dbg!() - ExprKind::Block(..) => { - // If the `dbg!` macro is a "free" statement and not contained within other expressions, - // remove the whole statement. - if let Node::Stmt(_) = cx.tcx.parent_hir_node(expr.hir_id) - && let Some(semi_span) = cx.sess().source_map().mac_call_stmt_semi_span(macro_call.span) - { - (macro_call.span.to(semi_span), String::new()) - } else { - (macro_call.span, String::from("()")) - } - }, - // dbg!(1) - ExprKind::Match(val, ..) => ( - macro_call.span, - snippet_with_applicability(cx, val.span.source_callsite(), "..", &mut applicability).to_string(), - ), - // dbg!(2, 3) - ExprKind::Tup( - [ - Expr { - kind: ExprKind::Match(first, ..), - .. - }, - .., - Expr { - kind: ExprKind::Match(last, ..), - .. - }, - ], - ) => { - let snippet = snippet_with_applicability( - cx, - first.span.source_callsite().to(last.span.source_callsite()), - "..", - &mut applicability, - ); - (macro_call.span, format!("({snippet})")) - }, - _ => return, - }; - - span_lint_and_sugg( - cx, - DBG_MACRO, - sugg_span, - "the `dbg!` macro is intended as a debugging tool", - "remove the invocation before committing it to a version control system", - suggestion, - applicability, - ); + // skip previous checked exprs + if self.prev_dbg_call_site.contains(macro_call.span) { + return; } + self.prev_dbg_call_site = macro_call.span; + + // allows `dbg!` in test code if allow-dbg-in-test is set to true in clippy.toml + if self.allow_dbg_in_tests && (is_in_test_function(cx.tcx, expr.hir_id) || is_in_cfg_test(cx.tcx, expr.hir_id)) + { + return; + } + let mut applicability = Applicability::MachineApplicable; + + let (sugg_span, suggestion) = match expr.peel_drop_temps().kind { + // dbg!() + ExprKind::Block(..) => { + // If the `dbg!` macro is a "free" statement and not contained within other expressions, + // remove the whole statement. + if let Some(Node::Stmt(_)) = cx.tcx.hir().find_parent(expr.hir_id) + && let Some(semi_span) = cx.sess().source_map().mac_call_stmt_semi_span(macro_call.span) + { + (macro_call.span.to(semi_span), String::new()) + } else { + (macro_call.span, String::from("()")) + } + }, + // dbg!(1) + ExprKind::Match(val, ..) => ( + macro_call.span, + snippet_with_applicability(cx, val.span.source_callsite(), "..", &mut applicability).to_string(), + ), + // dbg!(2, 3) + ExprKind::Tup( + [ + Expr { + kind: ExprKind::Match(first, ..), + .. + }, + .., + Expr { + kind: ExprKind::Match(last, ..), + .. + }, + ], + ) => { + let snippet = snippet_with_applicability( + cx, + first.span.source_callsite().to(last.span.source_callsite()), + "..", + &mut applicability, + ); + (macro_call.span, format!("({snippet})")) + }, + _ => return, + }; + + span_lint_and_sugg( + cx, + DBG_MACRO, + sugg_span, + "the `dbg!` macro is intended as a debugging tool", + "remove the invocation before committing it to a version control system", + suggestion, + applicability, + ); } } diff --git a/tests/ui/dbg_macro/dbg_macro.rs b/tests/ui/dbg_macro/dbg_macro.rs index 3f4770c63..bdb6c379f 100644 --- a/tests/ui/dbg_macro/dbg_macro.rs +++ b/tests/ui/dbg_macro/dbg_macro.rs @@ -107,3 +107,12 @@ mod mod1 { //~^ ERROR: the `dbg!` macro is intended as a debugging tool } } + +mod issue12131 { + fn dbg_in_print(s: &str) { + println!("dbg: {:?}", dbg!(s)); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + print!("{}", dbg!(s)); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + } +} diff --git a/tests/ui/dbg_macro/dbg_macro.stderr b/tests/ui/dbg_macro/dbg_macro.stderr index 5ad0bbfed..f632497bd 100644 --- a/tests/ui/dbg_macro/dbg_macro.stderr +++ b/tests/ui/dbg_macro/dbg_macro.stderr @@ -211,5 +211,27 @@ help: remove the invocation before committing it to a version control system LL | 1; | ~ -error: aborting due to 19 previous errors +error: the `dbg!` macro is intended as a debugging tool + --> $DIR/dbg_macro.rs:113:31 + | +LL | println!("dbg: {:?}", dbg!(s)); + | ^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | println!("dbg: {:?}", s); + | ~ + +error: the `dbg!` macro is intended as a debugging tool + --> $DIR/dbg_macro.rs:115:22 + | +LL | print!("{}", dbg!(s)); + | ^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | print!("{}", s); + | ~ + +error: aborting due to 21 previous errors