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

don't remove dbg! in arbitrary expressions #10725

Merged
merged 2 commits into from
May 18, 2023
Merged

Conversation

y21
Copy link
Member

@y21 y21 commented Apr 29, 2023

Fixes #9914

The dbg_macro lint replaces empty dbg! invocations with the empty string in its suggestion, which is not always valid code in certain contexts (e.g. let _ = dbg!(); becomes let _ = ;). This PR changes it to (), which should always be valid where dbg!() is valid (dbg!() with no arguments evaluates to ()).

It also special-cases "standalone" dbg!(); expression statements, where it will suggest removing the whole statement entirely like it did before.

changelog: [dbg_macro]: don't remove dbg!() in arbitrary expressions as it sometimes results in syntax errors

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2023

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 29, 2023
@Jarcho
Copy link
Contributor

Jarcho commented May 2, 2023

Does this work properly when dbg!() is expanded from another macro? e.g. foo!(); where foo!() expands to dbg!().

@y21
Copy link
Member Author

y21 commented May 2, 2023

Does this work properly when dbg!() is expanded from another macro? e.g. foo!(); where foo!() expands to dbg!().

Hmm it seems like it breaks if the dbg!() is passed as an expr metavariable

macro_rules! foo {
    ($x:expr) => { $x; };
}

fn main() {
    foo!(dbg!());
}
warning: the `dbg!` macro is intended as a debugging tool
  --> test.rs:16:10
   |
16 |     foo!(dbg!());
   |          ^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro
note: the lint level is defined here
  --> test.rs:1:9
   |
1  | #![warn(clippy::dbg_macro)]
   |         ^^^^^^^^^^^^^^^^^
help: remove the invocation before committing it to a version control system
   |
16 -     foo!(dbg!());
16 +     foo!(
   |

so perhaps my way of checking if we're allowed to remove the dbg!() by checking if the parent node is a StmtKind::Semi is not that reliable.

@Jarcho
Copy link
Contributor

Jarcho commented May 14, 2023

I think the only thing you can do here is to start tokenizing from the end of the call site and see if the first non-whitespace token is a semicolon.

@y21
Copy link
Member Author

y21 commented May 15, 2023

Alright, it's not exactly pretty but it works.

@Jarcho
Copy link
Contributor

Jarcho commented May 18, 2023

It works, so...

Thank you. @bors r+

@bors
Copy link
Collaborator

bors commented May 18, 2023

📌 Commit f0be0ee has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 18, 2023

⌛ Testing commit f0be0ee with merge 815a07e...

@bors
Copy link
Collaborator

bors commented May 18, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 815a07e to master...

@bors bors merged commit 815a07e into rust-lang:master May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dbg_macro removes empty dbg! macro
4 participants