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

Implement dbg_macro rule #3723

Merged
merged 11 commits into from Feb 3, 2019

Conversation

Projects
None yet
6 participants
@rhysd
Copy link
Contributor

commented Jan 30, 2019

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 #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

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

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
Copy link
Collaborator

left a comment

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?

/// ```
declare_clippy_lint! {
pub DBG_MACRO,
style,

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jan 30, 2019

Collaborator

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

Show resolved Hide resolved clippy_lints/src/dbg_macro.rs Outdated
Show resolved Hide resolved clippy_lints/src/dbg_macro.rs
Show resolved Hide resolved clippy_lints/src/dbg_macro.rs Outdated
Show resolved Hide resolved clippy_lints/src/dbg_macro.rs Outdated
@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@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.

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

@mikerite

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

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.

@mikerite

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

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

@rhysd rhysd force-pushed the rhysd:issue3721 branch from 5d9aec4 to f4500bb Feb 1, 2019

@rhysd rhysd force-pushed the rhysd:issue3721 branch from f4500bb to 7ec5528 Feb 1, 2019

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

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.

@rhysd rhysd force-pushed the rhysd:issue3721 branch from e86968f to 9d130a5 Feb 1, 2019

@oli-obk
Copy link
Collaborator

left a comment

I agree, the restriction category fits best for now

Show resolved Hide resolved clippy_lints/src/dbg_macro.rs Outdated
Show resolved Hide resolved tests/ui/dbg_macro.rs
@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Feb 1, 2019

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

mac.span,
"`dbg!` macro is intended as a debugging tool",
"ensure to avoid having uses of it in version control",
);

This comment has been minimized.

Copy link
@rhysd

rhysd Feb 3, 2019

Author Contributor

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

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

@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?

@rhysd rhysd force-pushed the rhysd:issue3721 branch from 66064b3 to 83d620b Feb 3, 2019

@oli-obk

This comment has been minimized.

Copy link
Collaborator

commented Feb 3, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

📌 Commit 83d620b has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

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

bors added a commit that referenced this pull request Feb 3, 2019

Auto merge of #3723 - rhysd:issue3721, r=oli-obk
Implement dbg_macro rule

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 #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.
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

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

@bors bors merged commit 83d620b into rust-lang:master Feb 3, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.