From cc607fe32ed69ac35b60e962803a2fb2133471cd Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 30 Apr 2023 01:30:15 +0200 Subject: [PATCH 1/2] don't remove `dbg!` in arbitrary expressions --- clippy_lints/src/dbg_macro.rs | 38 +++++++++++++++------- tests/ui/dbg_macro.rs | 8 +++++ tests/ui/dbg_macro.stderr | 60 +++++++++++++++++++++++++++-------- 3 files changed, 82 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/dbg_macro.rs b/clippy_lints/src/dbg_macro.rs index 799e71e847a..cea712f2fee 100644 --- a/clippy_lints/src/dbg_macro.rs +++ b/clippy_lints/src/dbg_macro.rs @@ -3,10 +3,10 @@ use clippy_utils::macros::root_macro_call_first_node; 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}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_hir::{Expr, ExprKind, Node, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::sym; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does @@ -31,6 +31,11 @@ declare_clippy_lint! { "`dbg!` macro is intended as a debugging tool" } +fn span_including_semi(cx: &LateContext<'_>, span: Span) -> Span { + let span = cx.sess().source_map().span_extend_to_next_char(span, ';', true); + span.with_hi(span.hi() + rustc_span::BytePos(1)) +} + #[derive(Copy, Clone)] pub struct DbgMacro { allow_dbg_in_tests: bool, @@ -55,13 +60,24 @@ impl LateLintPass<'_> for DbgMacro { return; } let mut applicability = Applicability::MachineApplicable; - let suggestion = match expr.peel_drop_temps().kind { - // dbg!() - ExprKind::Block(_, _) => String::new(), - // dbg!(1) - ExprKind::Match(val, ..) => { - snippet_with_applicability(cx, val.span.source_callsite(), "..", &mut applicability).to_string() + + let (sugg_span, suggestion) = match expr.peel_drop_temps().kind { + ExprKind::Block(..) => match cx.tcx.hir().find_parent(expr.hir_id) { + // dbg!() as a standalone statement, suggest removing the whole statement entirely + Some(Node::Stmt( + stmt @ Stmt { + kind: StmtKind::Semi(_), + .. + }, + )) => (span_including_semi(cx, stmt.span.source_callsite()), String::new()), + // empty dbg!() in arbitrary position (e.g. `foo(dbg!())`), suggest replacing with `foo(())` + _ => (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( [ @@ -82,7 +98,7 @@ impl LateLintPass<'_> for DbgMacro { "..", &mut applicability, ); - format!("({snippet})") + (macro_call.span, format!("({snippet})")) }, _ => return, }; @@ -90,7 +106,7 @@ impl LateLintPass<'_> for DbgMacro { span_lint_and_sugg( cx, DBG_MACRO, - macro_call.span, + sugg_span, "the `dbg!` macro is intended as a debugging tool", "remove the invocation before committing it to a version control system", suggestion, diff --git a/tests/ui/dbg_macro.rs b/tests/ui/dbg_macro.rs index 8701e3cd29f..ae2075e7dad 100644 --- a/tests/ui/dbg_macro.rs +++ b/tests/ui/dbg_macro.rs @@ -4,6 +4,7 @@ fn foo(n: u32) -> u32 { if let Some(n) = dbg!(n.checked_sub(4)) { n } else { n } } +fn bar(_: ()) {} fn factorial(n: u32) -> u32 { if dbg!(n <= 1) { @@ -21,6 +22,13 @@ fn main() { dbg!(1, 2, 3, 4, 5); } +fn issue9914() { + dbg!(); + #[allow(clippy::let_unit_value)] + let _ = dbg!(); + bar(dbg!()); +} + mod issue7274 { trait Thing<'b> { fn foo(&self); diff --git a/tests/ui/dbg_macro.stderr b/tests/ui/dbg_macro.stderr index ddb5f1342e9..34329a06b97 100644 --- a/tests/ui/dbg_macro.stderr +++ b/tests/ui/dbg_macro.stderr @@ -11,7 +11,7 @@ LL | if let Some(n) = n.checked_sub(4) { n } else { n } | ~~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:9:8 + --> $DIR/dbg_macro.rs:10:8 | LL | if dbg!(n <= 1) { | ^^^^^^^^^^^^ @@ -22,7 +22,7 @@ LL | if n <= 1 { | ~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:10:9 + --> $DIR/dbg_macro.rs:11:9 | LL | dbg!(1) | ^^^^^^^ @@ -33,7 +33,7 @@ LL | 1 | error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:12:9 + --> $DIR/dbg_macro.rs:13:9 | LL | dbg!(n * factorial(n - 1)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -44,7 +44,7 @@ LL | n * factorial(n - 1) | error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:17:5 + --> $DIR/dbg_macro.rs:18:5 | LL | dbg!(42); | ^^^^^^^^ @@ -55,7 +55,7 @@ LL | 42; | ~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:18:5 + --> $DIR/dbg_macro.rs:19:5 | LL | dbg!(dbg!(dbg!(42))); | ^^^^^^^^^^^^^^^^^^^^ @@ -66,7 +66,7 @@ LL | dbg!(dbg!(42)); | ~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:19:14 + --> $DIR/dbg_macro.rs:20:14 | LL | foo(3) + dbg!(factorial(4)); | ^^^^^^^^^^^^^^^^^^ @@ -77,7 +77,7 @@ LL | foo(3) + factorial(4); | ~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:20:5 + --> $DIR/dbg_macro.rs:21:5 | LL | dbg!(1, 2, dbg!(3, 4)); | ^^^^^^^^^^^^^^^^^^^^^^ @@ -88,7 +88,7 @@ LL | (1, 2, dbg!(3, 4)); | ~~~~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:21:5 + --> $DIR/dbg_macro.rs:22:5 | LL | dbg!(1, 2, 3, 4, 5); | ^^^^^^^^^^^^^^^^^^^ @@ -99,7 +99,41 @@ LL | (1, 2, 3, 4, 5); | ~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:41:9 + --> $DIR/dbg_macro.rs:26:5 + | +LL | dbg!(); + | ^^^^^^^ + | +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 + --> $DIR/dbg_macro.rs:28:13 + | +LL | let _ = dbg!(); + | ^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | let _ = (); + | ~~ + +error: the `dbg!` macro is intended as a debugging tool + --> $DIR/dbg_macro.rs:29:9 + | +LL | bar(dbg!()); + | ^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | bar(()); + | ~~ + +error: the `dbg!` macro is intended as a debugging tool + --> $DIR/dbg_macro.rs:49:9 | LL | dbg!(2); | ^^^^^^^ @@ -110,7 +144,7 @@ LL | 2; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:47:5 + --> $DIR/dbg_macro.rs:55:5 | LL | dbg!(1); | ^^^^^^^ @@ -121,7 +155,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:52:5 + --> $DIR/dbg_macro.rs:60:5 | LL | dbg!(1); | ^^^^^^^ @@ -132,7 +166,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:58:9 + --> $DIR/dbg_macro.rs:66:9 | LL | dbg!(1); | ^^^^^^^ @@ -142,5 +176,5 @@ help: remove the invocation before committing it to a version control system LL | 1; | ~ -error: aborting due to 13 previous errors +error: aborting due to 16 previous errors From f0be0ee1aa5314f6c7bb6949a8afe19e84ead35a Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 15 May 2023 21:33:56 +0200 Subject: [PATCH 2/2] handle nested macros and add tests for them --- clippy_lints/src/dbg_macro.rs | 51 ++++++++++++++++++++++++----------- tests/ui/dbg_macro.rs | 19 +++++++++++++ tests/ui/dbg_macro.stderr | 38 ++++++++++++++++++++------ 3 files changed, 85 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/dbg_macro.rs b/clippy_lints/src/dbg_macro.rs index cea712f2fee..ea17e7a6071 100644 --- a/clippy_lints/src/dbg_macro.rs +++ b/clippy_lints/src/dbg_macro.rs @@ -3,10 +3,10 @@ use clippy_utils::macros::root_macro_call_first_node; 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, Stmt, StmtKind}; +use rustc_hir::{Expr, ExprKind, Node}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{sym, Span}; +use rustc_span::{sym, BytePos, Pos, Span}; declare_clippy_lint! { /// ### What it does @@ -31,9 +31,29 @@ declare_clippy_lint! { "`dbg!` macro is intended as a debugging tool" } -fn span_including_semi(cx: &LateContext<'_>, span: Span) -> Span { - let span = cx.sess().source_map().span_extend_to_next_char(span, ';', true); - span.with_hi(span.hi() + rustc_span::BytePos(1)) +/// Gets the span of the statement up to the next semicolon, if and only if the next +/// non-whitespace character actually is a semicolon. +/// E.g. +/// ```rust,ignore +/// +/// dbg!(); +/// ^^^^^^^ this span is returned +/// +/// foo!(dbg!()); +/// no span is returned +/// ``` +fn span_including_semi(cx: &LateContext<'_>, span: Span) -> Option { + let sm = cx.sess().source_map(); + let sf = sm.lookup_source_file(span.hi()); + let src = sf.src.as_ref()?.get(span.hi().to_usize()..)?; + let first_non_whitespace = src.find(|c: char| !c.is_whitespace())?; + + if src.as_bytes()[first_non_whitespace] == b';' { + let hi = span.hi() + BytePos::from_usize(first_non_whitespace + 1); + Some(span.with_hi(hi)) + } else { + None + } } #[derive(Copy, Clone)] @@ -62,16 +82,17 @@ impl LateLintPass<'_> for DbgMacro { let mut applicability = Applicability::MachineApplicable; let (sugg_span, suggestion) = match expr.peel_drop_temps().kind { - ExprKind::Block(..) => match cx.tcx.hir().find_parent(expr.hir_id) { - // dbg!() as a standalone statement, suggest removing the whole statement entirely - Some(Node::Stmt( - stmt @ Stmt { - kind: StmtKind::Semi(_), - .. - }, - )) => (span_including_semi(cx, stmt.span.source_callsite()), String::new()), - // empty dbg!() in arbitrary position (e.g. `foo(dbg!())`), suggest replacing with `foo(())` - _ => (macro_call.span, String::from("()")), + // 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(stmt)) = cx.tcx.hir().find_parent(expr.hir_id) + && let Some(span) = span_including_semi(cx, stmt.span.source_callsite()) + { + (span, String::new()) + } else { + (macro_call.span, String::from("()")) + } }, // dbg!(1) ExprKind::Match(val, ..) => ( diff --git a/tests/ui/dbg_macro.rs b/tests/ui/dbg_macro.rs index ae2075e7dad..10788d40481 100644 --- a/tests/ui/dbg_macro.rs +++ b/tests/ui/dbg_macro.rs @@ -23,10 +23,29 @@ fn main() { } fn issue9914() { + macro_rules! foo { + ($x:expr) => { + $x; + }; + } + macro_rules! foo2 { + ($x:expr) => { + $x; + }; + } + macro_rules! expand_to_dbg { + () => { + dbg!(); + }; + } + dbg!(); #[allow(clippy::let_unit_value)] let _ = dbg!(); bar(dbg!()); + foo!(dbg!()); + foo2!(foo!(dbg!())); + expand_to_dbg!(); } mod issue7274 { diff --git a/tests/ui/dbg_macro.stderr b/tests/ui/dbg_macro.stderr index 34329a06b97..530e7663317 100644 --- a/tests/ui/dbg_macro.stderr +++ b/tests/ui/dbg_macro.stderr @@ -99,7 +99,7 @@ LL | (1, 2, 3, 4, 5); | ~~~~~~~~~~~~~~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:26:5 + --> $DIR/dbg_macro.rs:42:5 | LL | dbg!(); | ^^^^^^^ @@ -111,7 +111,7 @@ LL + | error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:28:13 + --> $DIR/dbg_macro.rs:44:13 | LL | let _ = dbg!(); | ^^^^^^ @@ -122,7 +122,7 @@ LL | let _ = (); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:29:9 + --> $DIR/dbg_macro.rs:45:9 | LL | bar(dbg!()); | ^^^^^^ @@ -133,7 +133,29 @@ LL | bar(()); | ~~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:49:9 + --> $DIR/dbg_macro.rs:46:10 + | +LL | foo!(dbg!()); + | ^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | foo!(()); + | ~~ + +error: the `dbg!` macro is intended as a debugging tool + --> $DIR/dbg_macro.rs:47:16 + | +LL | foo2!(foo!(dbg!())); + | ^^^^^^ + | +help: remove the invocation before committing it to a version control system + | +LL | foo2!(foo!(())); + | ~~ + +error: the `dbg!` macro is intended as a debugging tool + --> $DIR/dbg_macro.rs:68:9 | LL | dbg!(2); | ^^^^^^^ @@ -144,7 +166,7 @@ LL | 2; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:55:5 + --> $DIR/dbg_macro.rs:74:5 | LL | dbg!(1); | ^^^^^^^ @@ -155,7 +177,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:60:5 + --> $DIR/dbg_macro.rs:79:5 | LL | dbg!(1); | ^^^^^^^ @@ -166,7 +188,7 @@ LL | 1; | ~ error: the `dbg!` macro is intended as a debugging tool - --> $DIR/dbg_macro.rs:66:9 + --> $DIR/dbg_macro.rs:85:9 | LL | dbg!(1); | ^^^^^^^ @@ -176,5 +198,5 @@ help: remove the invocation before committing it to a version control system LL | 1; | ~ -error: aborting due to 16 previous errors +error: aborting due to 18 previous errors