[rhysd]

Fixes #3721

This patch adds new dbg_macro rule to check dbg! macro use.

Since this is my first patch to clippy, I'm not confident about following points:

• Currently only checks dbg! span. Is it possible to check if the dbg! macro is provided by standard library or user-defined? If it's possible, I can make the check more strict. Resolved as Implement dbg_macro rule #3723 (comment)
• Is category style correct for this rule?'restriction' is used instead
• Should I use span_lint_and_sugg instead of span_lint? Currently entire message is put as msg. But later part ensure to avoid having uses of it in version control may be put as suggestion. Done
• I'm not native English speaker. The message and doc may not be natural as English.

[mati865]

If somebody is using Clippy during development (through RLS for an example) this warning will appear all the time.
@oli-obk is there any way to detect debug or release build from within Clippy?

[oli-obk]

is there any way to detect debug or release build from within Clippy?

That's a little problematic, the compiler itself only has a notion of optimization levels and debug info, not release/debug. The latter is solely a cargo concept. What we can do is ask which cfgs have been set by looking at cx.tcx.sess.parse_sess.config. Maybe there are cfgs for release/debug?

[oli-obk]

I wonder if we need a whole new category for such lints at some point. We already have things like linting println! calls.

[rhysd]

@mati865 @oli-obk

Thank you for the nice point. I found following SO question/answer and debug_assertions might be available to check it's run in release mode.

• https://stackoverflow.com/questions/39204908/how-to-check-release-debug-builds-using-cfg-in-rust
• https://doc.rust-lang.org/std/macro.debug_assert.html

But I'm not sure actually. What do you think?

[ghost]

This lint is similar to use_debug and print_stdout whose purpose is "to catch debugging remnants". Based on that, I'd expect it to be a restriction lint that you enable on CI.

Also, maybe all three lints could be combined into a debugging_remnants lint.

[ghost]

I change my mind about my second point. You definitely wouldn't want to combine them.

[rhysd]

@oli-obk @mikerite @mati865

I changed category to 'restriction' and used suggestion. Though applicability is not machine-applicable. Could you review 7ec5528?

warning: `dbg!` macro is intended as a debugging tool
 --> foo.rs:2:5
  |
2 |     dbg!(42);
  |     ^^^^^^^^
  |
  = note: requested on the command line with `-W clippy::dbg-macro`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro
help: ensure to avoid having uses of it in version control
  |
2 |     42;
  |     ^^

[rhysd]

Regarding to category, I'm having two options

1. 'restriction' category which is disabled by default. Users are able to enable manually it by -W clippy::dbg_macro
2. 'style' category which is enabled by default. And check it only when debug_assertion config is enabled in later pass. It means check is performed on only --release

I think 2. is potentially confusing since users would not expect that clippy changes its behavior looking release/debug mode.

[oli-obk]

I agree, the restriction category fits best for now

[rhysd]

By adding new tests, I found two problems in current implementation.

At first, the token stream inserts a space between token and token:

warning: `dbg!` macro is intended as a debugging tool
  --> foo.rs:22:14
   |
22 |     foo(3) + dbg!(factorial(4));
   |              ^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro
help: ensure to avoid having uses of it in version control
   |
22 |     foo(3) + factorial ( 4 );
   |              ^^^^^^^^^^^^^^^

At second, if dbg!() is nested like dbg!(foo(dbg!(bar()))), dbg! is remaining in suggestion:

warning: `dbg!` macro is intended as a debugging tool
  --> foo.rs:21:5
   |
21 |     dbg!(dbg!(dbg!(42)));
   |     ^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro
help: ensure to avoid having uses of it in version control
   |
21 |     dbg ! ( dbg ! ( 42 ) );
   |     ^^^^^^^^^^^^^^^^^^^^^^

I think I should set "" to suggestion as @oli-obk suggested in review comment. If someone have any idea for workaround of above problems, please let me know..

[rhysd]

I also have found empty suggestion message does not work. When I tried to set String::new() at 6th argument of span_lint_and_sugg(), I got warning message as follows:

warning: `dbg!` macro is intended as a debugging tool
  --> foo.rs:23:14
   |
23 |     foo(3) + dbg!(factorial(4));
   |              ^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro
help: ensure to avoid having uses of it in version control
   |
23 |     foo(3) + ;
   |             --

[rhysd]

@oli-obk I used span_help_and_lint() to show both message and help, but not suggestion at 268ff85. And I added more tests at 54d49af. Could you review them again?

[oli-obk]

Instead of serializing the tokens you can use the snippet function on the span of the tokenstream. Then you get the actual text without the spaces

[rhysd]

I still used span_help_and_lint when snippet cannot be made from the token stream.

[rhysd]

@oli-obk Thank you for your suggestion. I read libsyntax implementation and found that retrieving first and last TokenTrees from TokenStream can make a span object enclosing entire the stream. At 3100fec I revived span_lint_and_sugg with snippet though still using span_help_and_lint when snippet is not available. Could you review the commit?

[oli-obk]

@bors r+

Thanks! this looks great to me now. Sorry about the tokentree thing, I wrongly thought there was a method to give you the span.

[bors]

📌 Commit 83d620b has been approved by oli-obk

[bors]

⌛ Testing commit 83d620b with merge 273b7f7...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 273b7f7 to master...

[rhysd]

@oli-obk No problem. It was a good chance to read a source of rustc compiler for me. Thank you for many code reviews!