Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix [dbg_macro] FN when dbg is inside some complex macros #12151

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions clippy_lints/src/dbg_macro.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::macros::root_macro_call_first_node;
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;
use rustc_span::{sym, Span, SyntaxContext};

declare_clippy_lint! {
/// ### What it does
Expand All @@ -31,31 +33,38 @@ declare_clippy_lint! {
"`dbg!` macro is intended as a debugging tool"
}

#[derive(Copy, Clone)]
#[derive(Clone)]
pub struct DbgMacro {
allow_dbg_in_tests: bool,
/// Tracks the `dbg!` macro callsites that are already checked.
checked_dbg_call_site: FxHashSet<Span>,
/// Tracks the previous `SyntaxContext`, to avoid walking the same context chain.
prev_ctxt: SyntaxContext,
}

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,
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_first_node(cx, expr) else {
return;
};
if cx.tcx.is_diagnostic_item(sym::dbg_macro, macro_call.def_id) {
let cur_syntax_ctxt = expr.span.ctxt();

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
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;
}
!(self.allow_dbg_in_tests && is_in_test(cx, expr.hir_id))
{
let mut applicability = Applicability::MachineApplicable;

let (sugg_span, suggestion) = match expr.peel_drop_temps().kind {
Expand Down Expand Up @@ -101,6 +110,8 @@ impl LateLintPass<'_> for DbgMacro {
_ => return,
};

self.prev_ctxt = cur_syntax_ctxt;

span_lint_and_sugg(
cx,
DBG_MACRO,
Expand All @@ -112,4 +123,16 @@ impl LateLintPass<'_> for DbgMacro {
);
}
}

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<MacroCall> {
macro_backtrace(span).find(|mc| cx.tcx.is_diagnostic_item(sym::dbg_macro, mc.def_id))
}
38 changes: 38 additions & 0 deletions tests/ui-toml/dbg_macro/dbg_macro.fixed
Original file line number Diff line number Diff line change
@@ -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);
}
}
5 changes: 2 additions & 3 deletions tests/ui-toml/dbg_macro/dbg_macro.rs
Original file line number Diff line number Diff line change
@@ -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 }
}
Expand All @@ -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);
}

Expand Down
34 changes: 6 additions & 28 deletions tests/ui-toml/dbg_macro/dbg_macro.stderr
Original file line number Diff line number Diff line change
@@ -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 }
| ^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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) {
| ^^^^^^^^^^^^
Expand All @@ -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)
| ^^^^^^^
Expand All @@ -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))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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);
| ^^^^^^^^
Expand All @@ -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
|
Expand All @@ -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);
| ^^^^^^^^^^^^^^^^^^^
|
Expand All @@ -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

111 changes: 111 additions & 0 deletions tests/ui/dbg_macro/dbg_macro.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
#![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 {
() => {

//~^ ERROR: the `dbg!` macro is intended as a debugging tool
};
}


//~^ 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
}
}
20 changes: 11 additions & 9 deletions tests/ui/dbg_macro/dbg_macro.rs
Original file line number Diff line number Diff line change
@@ -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 }
Expand All @@ -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
}
Expand All @@ -49,6 +41,7 @@ fn issue9914() {
macro_rules! expand_to_dbg {
() => {
dbg!();
//~^ ERROR: the `dbg!` macro is intended as a debugging tool
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to put that ERROR comment inside this macro because I don't know where else to put it 😆

}

Expand Down Expand Up @@ -107,3 +100,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
}
}
Loading
Loading