handle nested macros and add tests for them

Author: y21

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<Span> {
+    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, ..) => (

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 {

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