From 1fe884401aa12b08408b101cf931cb396d0ccf3f Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 15 Jan 2024 17:51:29 +0800 Subject: [PATCH 1/3] 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 ec66556cebf..a517d7bd661 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 3f4770c63d0..bdb6c379f88 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 5ad0bbfed94..f632497bd73 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 From 0535f558317ca934bccbf5cda3e0f9779dbfd972 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 21 Feb 2024 02:11:19 +0800 Subject: [PATCH 2/3] lint nested `dbg!` macros, split tests --- clippy_lints/src/dbg_macro.rs | 16 +-- tests/ui-toml/dbg_macro/dbg_macro.fixed | 38 ++++++ tests/ui-toml/dbg_macro/dbg_macro.rs | 5 +- tests/ui-toml/dbg_macro/dbg_macro.stderr | 34 +----- tests/ui/dbg_macro/dbg_macro.fixed | 110 ++++++++++++++++++ tests/ui/dbg_macro/dbg_macro.rs | 10 +- tests/ui/dbg_macro/dbg_macro.stderr | 76 ++++-------- tests/ui/dbg_macro/dbg_macro_unfixable.rs | 12 ++ tests/ui/dbg_macro/dbg_macro_unfixable.stderr | 71 +++++++++++ 9 files changed, 269 insertions(+), 103 deletions(-) create mode 100644 tests/ui-toml/dbg_macro/dbg_macro.fixed create mode 100644 tests/ui/dbg_macro/dbg_macro.fixed create mode 100644 tests/ui/dbg_macro/dbg_macro_unfixable.rs create mode 100644 tests/ui/dbg_macro/dbg_macro_unfixable.stderr diff --git a/clippy_lints/src/dbg_macro.rs b/clippy_lints/src/dbg_macro.rs index a517d7bd661..bd61260d94e 100644 --- a/clippy_lints/src/dbg_macro.rs +++ b/clippy_lints/src/dbg_macro.rs @@ -2,6 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; 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_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, Node}; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -31,12 +32,12 @@ declare_clippy_lint! { "`dbg!` macro is intended as a debugging tool" } -#[derive(Copy, Clone)] +#[derive(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, + /// Keep tracks the `dbg!` macro callsites that are already checked, + /// so that we can save some performance by skipping any expressions from the same expansion. + checked_dbg_call_site: FxHashSet, } impl_lint_pass!(DbgMacro => [DBG_MACRO]); @@ -45,7 +46,7 @@ impl DbgMacro { pub fn new(allow_dbg_in_tests: bool) -> Self { DbgMacro { allow_dbg_in_tests, - prev_dbg_call_site: Span::default(), + checked_dbg_call_site: FxHashSet::default(), } } } @@ -57,11 +58,10 @@ impl LateLintPass<'_> for DbgMacro { else { return; }; - // skip previous checked exprs - if self.prev_dbg_call_site.contains(macro_call.span) { + if self.checked_dbg_call_site.contains(¯o_call.span) { return; } - self.prev_dbg_call_site = macro_call.span; + self.checked_dbg_call_site.insert(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)) diff --git a/tests/ui-toml/dbg_macro/dbg_macro.fixed b/tests/ui-toml/dbg_macro/dbg_macro.fixed new file mode 100644 index 00000000000..d42b29ba21a --- /dev/null +++ b/tests/ui-toml/dbg_macro/dbg_macro.fixed @@ -0,0 +1,38 @@ +//@compile-flags: --test +#![warn(clippy::dbg_macro)] +#![allow(clippy::unnecessary_operation, clippy::no_effect)] + +fn foo(n: u32) -> u32 { + if let Some(n) = n.checked_sub(4) { n } else { n } +} + +fn factorial(n: u32) -> u32 { + if n <= 1 { + 1 + } else { + n * factorial(n - 1) + } +} + +fn main() { + 42; + foo(3) + factorial(4); + (1, 2, 3, 4, 5); +} + +#[test] +pub fn issue8481() { + dbg!(1); +} + +#[cfg(test)] +fn foo2() { + dbg!(1); +} + +#[cfg(test)] +mod mod1 { + fn func() { + dbg!(1); + } +} diff --git a/tests/ui-toml/dbg_macro/dbg_macro.rs b/tests/ui-toml/dbg_macro/dbg_macro.rs index 67129e62477..bd189b1576f 100644 --- a/tests/ui-toml/dbg_macro/dbg_macro.rs +++ b/tests/ui-toml/dbg_macro/dbg_macro.rs @@ -1,6 +1,7 @@ //@compile-flags: --test #![warn(clippy::dbg_macro)] -//@no-rustfix +#![allow(clippy::unnecessary_operation, clippy::no_effect)] + fn foo(n: u32) -> u32 { if let Some(n) = dbg!(n.checked_sub(4)) { n } else { n } } @@ -15,9 +16,7 @@ fn factorial(n: u32) -> u32 { fn main() { dbg!(42); - dbg!(dbg!(dbg!(42))); foo(3) + dbg!(factorial(4)); - dbg!(1, 2, dbg!(3, 4)); dbg!(1, 2, 3, 4, 5); } diff --git a/tests/ui-toml/dbg_macro/dbg_macro.stderr b/tests/ui-toml/dbg_macro/dbg_macro.stderr index 8ffc426be2d..129fab5ff97 100644 --- a/tests/ui-toml/dbg_macro/dbg_macro.stderr +++ b/tests/ui-toml/dbg_macro/dbg_macro.stderr @@ -1,5 +1,5 @@ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:5:22 + --> tests/ui-toml/dbg_macro/dbg_macro.rs:6:22 | LL | if let Some(n) = dbg!(n.checked_sub(4)) { n } else { n } | ^^^^^^^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL | if let Some(n) = n.checked_sub(4) { n } else { n } | ~~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:9:8 + --> tests/ui-toml/dbg_macro/dbg_macro.rs:10:8 | LL | if dbg!(n <= 1) { | ^^^^^^^^^^^^ @@ -23,7 +23,7 @@ LL | if n <= 1 { | ~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:10:9 + --> tests/ui-toml/dbg_macro/dbg_macro.rs:11:9 | LL | dbg!(1) | ^^^^^^^ @@ -34,7 +34,7 @@ LL | 1 | error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:12:9 + --> tests/ui-toml/dbg_macro/dbg_macro.rs:13:9 | LL | dbg!(n * factorial(n - 1)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -45,7 +45,7 @@ LL | n * factorial(n - 1) | error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:17:5 + --> tests/ui-toml/dbg_macro/dbg_macro.rs:18:5 | LL | dbg!(42); | ^^^^^^^^ @@ -55,17 +55,6 @@ help: remove the invocation before committing it to a version control system LL | 42; | ~~ -error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:18:5 - | -LL | dbg!(dbg!(dbg!(42))); - | ^^^^^^^^^^^^^^^^^^^^ - | -help: remove the invocation before committing it to a version control system - | -LL | dbg!(dbg!(42)); - | ~~~~~~~~~~~~~~ - error: the `dbg!` macro is intended as a debugging tool --> tests/ui-toml/dbg_macro/dbg_macro.rs:19:14 | @@ -80,17 +69,6 @@ LL | foo(3) + factorial(4); error: the `dbg!` macro is intended as a debugging tool --> tests/ui-toml/dbg_macro/dbg_macro.rs:20:5 | -LL | dbg!(1, 2, dbg!(3, 4)); - | ^^^^^^^^^^^^^^^^^^^^^^ - | -help: remove the invocation before committing it to a version control system - | -LL | (1, 2, dbg!(3, 4)); - | ~~~~~~~~~~~~~~~~~~ - -error: the `dbg!` macro is intended as a debugging tool - --> tests/ui-toml/dbg_macro/dbg_macro.rs:21:5 - | LL | dbg!(1, 2, 3, 4, 5); | ^^^^^^^^^^^^^^^^^^^ | @@ -99,5 +77,5 @@ help: remove the invocation before committing it to a version control system LL | (1, 2, 3, 4, 5); | ~~~~~~~~~~~~~~~ -error: aborting due to 9 previous errors +error: aborting due to 7 previous errors diff --git a/tests/ui/dbg_macro/dbg_macro.fixed b/tests/ui/dbg_macro/dbg_macro.fixed new file mode 100644 index 00000000000..7197c3c8f47 --- /dev/null +++ b/tests/ui/dbg_macro/dbg_macro.fixed @@ -0,0 +1,110 @@ +#![warn(clippy::dbg_macro)] +#![allow(clippy::unnecessary_operation, clippy::no_effect)] + +fn foo(n: u32) -> u32 { + if let Some(n) = n.checked_sub(4) { n } else { n } + //~^ ERROR: the `dbg!` macro is intended as a debugging tool +} +fn bar(_: ()) {} + +fn factorial(n: u32) -> u32 { + if n <= 1 { + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + 1 + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + } else { + n * factorial(n - 1) + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + } +} + +fn main() { + 42; + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + foo(3) + factorial(4); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + (1, 2, 3, 4, 5); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool +} + +fn issue9914() { + macro_rules! foo { + ($x:expr) => { + $x; + }; + } + macro_rules! foo2 { + ($x:expr) => { + $x; + }; + } + macro_rules! expand_to_dbg { + () => { + dbg!(); + }; + } + + + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + #[allow(clippy::let_unit_value)] + let _ = (); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + bar(()); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + foo!(()); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + foo2!(foo!(())); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + expand_to_dbg!(); +} + +mod issue7274 { + trait Thing<'b> { + fn foo(&self); + } + + macro_rules! define_thing { + ($thing:ident, $body:expr) => { + impl<'a> Thing<'a> for $thing { + fn foo<'b>(&self) { + $body + } + } + }; + } + + struct MyThing; + define_thing!(MyThing, { + 2; + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + }); +} + +#[test] +pub fn issue8481() { + 1; + //~^ ERROR: the `dbg!` macro is intended as a debugging tool +} + +#[cfg(test)] +fn foo2() { + 1; + //~^ ERROR: the `dbg!` macro is intended as a debugging tool +} + +#[cfg(test)] +mod mod1 { + fn func() { + 1; + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + } +} + +mod issue12131 { + fn dbg_in_print(s: &str) { + println!("dbg: {:?}", s); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + print!("{}", s); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + } +} diff --git a/tests/ui/dbg_macro/dbg_macro.rs b/tests/ui/dbg_macro/dbg_macro.rs index bdb6c379f88..0815d9f38dc 100644 --- a/tests/ui/dbg_macro/dbg_macro.rs +++ b/tests/ui/dbg_macro/dbg_macro.rs @@ -1,9 +1,5 @@ -//@no-rustfix - #![warn(clippy::dbg_macro)] - -#[path = "auxiliary/submodule.rs"] -mod submodule; +#![allow(clippy::unnecessary_operation, clippy::no_effect)] fn foo(n: u32) -> u32 { if let Some(n) = dbg!(n.checked_sub(4)) { n } else { n } @@ -25,12 +21,8 @@ fn factorial(n: u32) -> u32 { fn main() { dbg!(42); //~^ ERROR: the `dbg!` macro is intended as a debugging tool - dbg!(dbg!(dbg!(42))); - //~^ ERROR: the `dbg!` macro is intended as a debugging tool foo(3) + dbg!(factorial(4)); //~^ ERROR: the `dbg!` macro is intended as a debugging tool - dbg!(1, 2, dbg!(3, 4)); - //~^ ERROR: the `dbg!` macro is intended as a debugging tool dbg!(1, 2, 3, 4, 5); //~^ 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 f632497bd73..5ff49816d00 100644 --- a/tests/ui/dbg_macro/dbg_macro.stderr +++ b/tests/ui/dbg_macro/dbg_macro.stderr @@ -1,30 +1,18 @@ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/auxiliary/submodule.rs:2:5 - | -LL | dbg!(); - | ^^^^^^^ - | - = note: `-D clippy::dbg-macro` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::dbg_macro)]` -help: remove the invocation before committing it to a version control system - | -LL - dbg!(); -LL + - | - -error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:9:22 + --> tests/ui/dbg_macro/dbg_macro.rs:5:22 | LL | if let Some(n) = dbg!(n.checked_sub(4)) { n } else { n } | ^^^^^^^^^^^^^^^^^^^^^^ | + = note: `-D clippy::dbg-macro` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::dbg_macro)]` help: remove the invocation before committing it to a version control system | LL | if let Some(n) = n.checked_sub(4) { n } else { n } | ~~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:15:8 + --> tests/ui/dbg_macro/dbg_macro.rs:11:8 | LL | if dbg!(n <= 1) { | ^^^^^^^^^^^^ @@ -35,7 +23,7 @@ LL | if n <= 1 { | ~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:17:9 + --> tests/ui/dbg_macro/dbg_macro.rs:13:9 | LL | dbg!(1) | ^^^^^^^ @@ -46,7 +34,7 @@ LL | 1 | error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:20:9 + --> tests/ui/dbg_macro/dbg_macro.rs:16:9 | LL | dbg!(n * factorial(n - 1)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -57,7 +45,7 @@ LL | n * factorial(n - 1) | error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:26:5 + --> tests/ui/dbg_macro/dbg_macro.rs:22:5 | LL | dbg!(42); | ^^^^^^^^ @@ -68,18 +56,7 @@ LL | 42; | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:28:5 - | -LL | dbg!(dbg!(dbg!(42))); - | ^^^^^^^^^^^^^^^^^^^^ - | -help: remove the invocation before committing it to a version control system - | -LL | dbg!(dbg!(42)); - | ~~~~~~~~~~~~~~ - -error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:30:14 + --> tests/ui/dbg_macro/dbg_macro.rs:24:14 | LL | foo(3) + dbg!(factorial(4)); | ^^^^^^^^^^^^^^^^^^ @@ -90,18 +67,7 @@ LL | foo(3) + factorial(4); | ~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:32:5 - | -LL | dbg!(1, 2, dbg!(3, 4)); - | ^^^^^^^^^^^^^^^^^^^^^^ - | -help: remove the invocation before committing it to a version control system - | -LL | (1, 2, dbg!(3, 4)); - | ~~~~~~~~~~~~~~~~~~ - -error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:34:5 + --> tests/ui/dbg_macro/dbg_macro.rs:26:5 | LL | dbg!(1, 2, 3, 4, 5); | ^^^^^^^^^^^^^^^^^^^ @@ -112,7 +78,7 @@ LL | (1, 2, 3, 4, 5); | ~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:55:5 + --> tests/ui/dbg_macro/dbg_macro.rs:47:5 | LL | dbg!(); | ^^^^^^^ @@ -124,7 +90,7 @@ LL + | error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:58:13 + --> tests/ui/dbg_macro/dbg_macro.rs:50:13 | LL | let _ = dbg!(); | ^^^^^^ @@ -135,7 +101,7 @@ LL | let _ = (); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:60:9 + --> tests/ui/dbg_macro/dbg_macro.rs:52:9 | LL | bar(dbg!()); | ^^^^^^ @@ -146,7 +112,7 @@ LL | bar(()); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:62:10 + --> tests/ui/dbg_macro/dbg_macro.rs:54:10 | LL | foo!(dbg!()); | ^^^^^^ @@ -157,7 +123,7 @@ LL | foo!(()); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:64:16 + --> tests/ui/dbg_macro/dbg_macro.rs:56:16 | LL | foo2!(foo!(dbg!())); | ^^^^^^ @@ -168,7 +134,7 @@ LL | foo2!(foo!(())); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:86:9 + --> tests/ui/dbg_macro/dbg_macro.rs:78:9 | LL | dbg!(2); | ^^^^^^^ @@ -179,7 +145,7 @@ LL | 2; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:93:5 + --> tests/ui/dbg_macro/dbg_macro.rs:85:5 | LL | dbg!(1); | ^^^^^^^ @@ -190,7 +156,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:99:5 + --> tests/ui/dbg_macro/dbg_macro.rs:91:5 | LL | dbg!(1); | ^^^^^^^ @@ -201,7 +167,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:106:9 + --> tests/ui/dbg_macro/dbg_macro.rs:98:9 | LL | dbg!(1); | ^^^^^^^ @@ -212,7 +178,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:113:31 + --> tests/ui/dbg_macro/dbg_macro.rs:105:31 | LL | println!("dbg: {:?}", dbg!(s)); | ^^^^^^^ @@ -223,7 +189,7 @@ LL | println!("dbg: {:?}", s); | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:115:22 + --> tests/ui/dbg_macro/dbg_macro.rs:107:22 | LL | print!("{}", dbg!(s)); | ^^^^^^^ @@ -233,5 +199,5 @@ help: remove the invocation before committing it to a version control system LL | print!("{}", s); | ~ -error: aborting due to 21 previous errors +error: aborting due to 18 previous errors diff --git a/tests/ui/dbg_macro/dbg_macro_unfixable.rs b/tests/ui/dbg_macro/dbg_macro_unfixable.rs new file mode 100644 index 00000000000..0e83766ccae --- /dev/null +++ b/tests/ui/dbg_macro/dbg_macro_unfixable.rs @@ -0,0 +1,12 @@ +//@no-rustfix +#![warn(clippy::dbg_macro)] + +#[path = "auxiliary/submodule.rs"] +mod submodule; + +fn main() { + dbg!(dbg!(dbg!(42))); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool + dbg!(1, 2, dbg!(3, 4)); + //~^ ERROR: the `dbg!` macro is intended as a debugging tool +} diff --git a/tests/ui/dbg_macro/dbg_macro_unfixable.stderr b/tests/ui/dbg_macro/dbg_macro_unfixable.stderr new file mode 100644 index 00000000000..d21595c2fcd --- /dev/null +++ b/tests/ui/dbg_macro/dbg_macro_unfixable.stderr @@ -0,0 +1,71 @@ +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/auxiliary/submodule.rs:2:5 + | +LL | dbg!(); + | ^^^^^^^ + | + = note: `-D clippy::dbg-macro` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::dbg_macro)]` +help: remove the invocation before committing it to a version control system + | +LL - dbg!(); +LL + + | + +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/dbg_macro_unfixable.rs:8:5 + | +LL | dbg!(dbg!(dbg!(42))); + | ^^^^^^^^^^^^^^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | dbg!(dbg!(42)); + | ~~~~~~~~~~~~~~ + +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/dbg_macro_unfixable.rs:8:10 + | +LL | dbg!(dbg!(dbg!(42))); + | ^^^^^^^^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | dbg!(dbg!(42)); + | ~~~~~~~~ + +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/dbg_macro_unfixable.rs:8:15 + | +LL | dbg!(dbg!(dbg!(42))); + | ^^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | dbg!(dbg!(42)); + | ~~ + +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/dbg_macro_unfixable.rs:10:5 + | +LL | dbg!(1, 2, dbg!(3, 4)); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | (1, 2, dbg!(3, 4)); + | ~~~~~~~~~~~~~~~~~~ + +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/dbg_macro_unfixable.rs:10:16 + | +LL | dbg!(1, 2, dbg!(3, 4)); + | ^^^^^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | dbg!(1, 2, (3, 4)); + | ~~~~~~ + +error: aborting due to 6 previous errors + From fe4e0aca73666b1be9a7f285fb9ae6e03d58a54c Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 6 Mar 2024 23:08:59 +0800 Subject: [PATCH 3/3] checks `dbg` inside other macros as well (but no ext macro); some refractoring; --- clippy_lints/src/dbg_macro.rs | 152 +++++++++++++++------------- tests/ui/dbg_macro/dbg_macro.fixed | 3 +- tests/ui/dbg_macro/dbg_macro.rs | 1 + tests/ui/dbg_macro/dbg_macro.stderr | 40 +++++--- 4 files changed, 113 insertions(+), 83 deletions(-) diff --git a/clippy_lints/src/dbg_macro.rs b/clippy_lints/src/dbg_macro.rs index bd61260d94e..e2296767431 100644 --- a/clippy_lints/src/dbg_macro.rs +++ b/clippy_lints/src/dbg_macro.rs @@ -1,13 +1,14 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::macros::root_macro_call; +use clippy_utils::macros::{macro_backtrace, MacroCall}; use clippy_utils::source::snippet_with_applicability; use clippy_utils::{is_in_cfg_test, is_in_test_function}; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, Node}; +use rustc_hir::{Expr, ExprKind, HirId, Node}; use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; use rustc_session::impl_lint_pass; -use rustc_span::{sym, Span}; +use rustc_span::{sym, Span, SyntaxContext}; declare_clippy_lint! { /// ### What it does @@ -35,9 +36,10 @@ declare_clippy_lint! { #[derive(Clone)] pub struct DbgMacro { allow_dbg_in_tests: bool, - /// Keep tracks the `dbg!` macro callsites that are already checked, - /// so that we can save some performance by skipping any expressions from the same expansion. + /// Tracks the `dbg!` macro callsites that are already checked. checked_dbg_call_site: FxHashSet, + /// Tracks the previous `SyntaxContext`, to avoid walking the same context chain. + prev_ctxt: SyntaxContext, } impl_lint_pass!(DbgMacro => [DBG_MACRO]); @@ -47,80 +49,90 @@ impl DbgMacro { DbgMacro { allow_dbg_in_tests, checked_dbg_call_site: FxHashSet::default(), + prev_ctxt: SyntaxContext::root(), } } } impl LateLintPass<'_> for DbgMacro { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { - 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 self.checked_dbg_call_site.contains(¯o_call.span) { - return; - } - self.checked_dbg_call_site.insert(macro_call.span); + let cur_syntax_ctxt = expr.span.ctxt(); - // 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)) + if cur_syntax_ctxt != self.prev_ctxt && + let Some(macro_call) = first_dbg_macro_in_expansion(cx, expr.span) && + !in_external_macro(cx.sess(), macro_call.span) && + self.checked_dbg_call_site.insert(macro_call.span) && + // allows `dbg!` in test code if allow-dbg-in-test is set to true in clippy.toml + !(self.allow_dbg_in_tests && is_in_test(cx, 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, + }; + + self.prev_ctxt = cur_syntax_ctxt; + + 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, + ); } - 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, - ); + fn check_crate_post(&mut self, _: &LateContext<'_>) { + self.checked_dbg_call_site = FxHashSet::default(); } } + +fn is_in_test(cx: &LateContext<'_>, hir_id: HirId) -> bool { + is_in_test_function(cx.tcx, hir_id) || is_in_cfg_test(cx.tcx, hir_id) +} + +fn first_dbg_macro_in_expansion(cx: &LateContext<'_>, span: Span) -> Option { + macro_backtrace(span).find(|mc| cx.tcx.is_diagnostic_item(sym::dbg_macro, mc.def_id)) +} diff --git a/tests/ui/dbg_macro/dbg_macro.fixed b/tests/ui/dbg_macro/dbg_macro.fixed index 7197c3c8f47..e3525191423 100644 --- a/tests/ui/dbg_macro/dbg_macro.fixed +++ b/tests/ui/dbg_macro/dbg_macro.fixed @@ -40,7 +40,8 @@ fn issue9914() { } macro_rules! expand_to_dbg { () => { - dbg!(); + + //~^ ERROR: the `dbg!` macro is intended as a debugging tool }; } diff --git a/tests/ui/dbg_macro/dbg_macro.rs b/tests/ui/dbg_macro/dbg_macro.rs index 0815d9f38dc..80606c2db05 100644 --- a/tests/ui/dbg_macro/dbg_macro.rs +++ b/tests/ui/dbg_macro/dbg_macro.rs @@ -41,6 +41,7 @@ fn issue9914() { macro_rules! expand_to_dbg { () => { dbg!(); + //~^ 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 5ff49816d00..86667701da0 100644 --- a/tests/ui/dbg_macro/dbg_macro.stderr +++ b/tests/ui/dbg_macro/dbg_macro.stderr @@ -78,7 +78,7 @@ LL | (1, 2, 3, 4, 5); | ~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:47:5 + --> tests/ui/dbg_macro/dbg_macro.rs:48:5 | LL | dbg!(); | ^^^^^^^ @@ -90,7 +90,7 @@ LL + | error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:50:13 + --> tests/ui/dbg_macro/dbg_macro.rs:51:13 | LL | let _ = dbg!(); | ^^^^^^ @@ -101,7 +101,7 @@ LL | let _ = (); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:52:9 + --> tests/ui/dbg_macro/dbg_macro.rs:53:9 | LL | bar(dbg!()); | ^^^^^^ @@ -112,7 +112,7 @@ LL | bar(()); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:54:10 + --> tests/ui/dbg_macro/dbg_macro.rs:55:10 | LL | foo!(dbg!()); | ^^^^^^ @@ -123,7 +123,7 @@ LL | foo!(()); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:56:16 + --> tests/ui/dbg_macro/dbg_macro.rs:57:16 | LL | foo2!(foo!(dbg!())); | ^^^^^^ @@ -134,7 +134,23 @@ LL | foo2!(foo!(())); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:78:9 + --> tests/ui/dbg_macro/dbg_macro.rs:43:13 + | +LL | dbg!(); + | ^^^^^^^ +... +LL | expand_to_dbg!(); + | ---------------- in this macro invocation + | + = note: this error originates in the macro `expand_to_dbg` (in Nightly builds, run with -Z macro-backtrace for more info) +help: remove the invocation before committing it to a version control system + | +LL - dbg!(); +LL + + | + +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui/dbg_macro/dbg_macro.rs:79:9 | LL | dbg!(2); | ^^^^^^^ @@ -145,7 +161,7 @@ LL | 2; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:85:5 + --> tests/ui/dbg_macro/dbg_macro.rs:86:5 | LL | dbg!(1); | ^^^^^^^ @@ -156,7 +172,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:91:5 + --> tests/ui/dbg_macro/dbg_macro.rs:92:5 | LL | dbg!(1); | ^^^^^^^ @@ -167,7 +183,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:98:9 + --> tests/ui/dbg_macro/dbg_macro.rs:99:9 | LL | dbg!(1); | ^^^^^^^ @@ -178,7 +194,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:105:31 + --> tests/ui/dbg_macro/dbg_macro.rs:106:31 | LL | println!("dbg: {:?}", dbg!(s)); | ^^^^^^^ @@ -189,7 +205,7 @@ LL | println!("dbg: {:?}", s); | ~ error: the `dbg!` macro is intended as a debugging tool - --> tests/ui/dbg_macro/dbg_macro.rs:107:22 + --> tests/ui/dbg_macro/dbg_macro.rs:108:22 | LL | print!("{}", dbg!(s)); | ^^^^^^^ @@ -199,5 +215,5 @@ help: remove the invocation before committing it to a version control system LL | print!("{}", s); | ~ -error: aborting due to 18 previous errors +error: aborting due to 19 previous errors