Skip to content

Improve non_fmt_panic lint. #82113

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 9 commits into from
Feb 23, 2021
Merged

Improve non_fmt_panic lint. #82113

merged 9 commits into from
Feb 23, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Feb 14, 2021

This change:

  • fixes the span used by this lint in the case the panic argument is a single macro expansion (e.g. panic!(a!()));
  • adds a suggestion for panic!(format!(..)) to remove format!() instead of adding "{}", or using panic_any like it does now; and
  • fixes the incorrect suggestion to replace panic![123] by panic_any(123].

Fixes #82109.
Fixes #82110.
Fixes #82111.

Example output:

warning: panic message is not a string literal
 --> src/main.rs:8:12
  |
8 |     panic!(format!("error: {}", "oh no"));
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(non_fmt_panic)]` on by default
  = note: this is no longer accepted in Rust 2021
  = note: the panic!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
  |
8 |     panic!("error: {}", "oh no");
  |           --                  --

r? @estebank

@m-ou-se m-ou-se added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-confusing Diagnostics: Confusing error or lint that should be reworked. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Feb 14, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2021
@rust-log-analyzer

This comment has been minimized.

# to view
@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 14, 2021

Note: Applying the multipart suggestions automatically with cargo fix is blocked on #53934.

jyn514
jyn514 reviewed Feb 14, 2021
View reviewed changes
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 23, 2021

📌 Commit ad93f48 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 23, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
@bors
Auto merge of rust-lang#82443 - Dylan-DPC:rollup-yni7uio, r=Dylan-DPC
fe1bf8e 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81629 (Point out implicit deref coercions in borrow)
 - rust-lang#82113 (Improve non_fmt_panic lint.)
 - rust-lang#82258 (Implement -Z hir-stats for nested foreign items)
 - rust-lang#82296 (Support `pub` on `macro_rules`)
 - rust-lang#82297 (Consider auto derefs before warning about write only fields)
 - rust-lang#82305 (Remove many RefCells from DocContext)
 - rust-lang#82308 (Lower condition of `if` expression before it's "then" block)
 - rust-lang#82311 (Jsondocck improvements)
 - rust-lang#82362 (Fix mir-cfg dumps)
 - rust-lang#82391 (disable atomic_max/min tests in Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 547b3ad into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
jyn514
jyn514 reviewed Mar 5, 2021
View reviewed changes
compiler/rustc_lint/src/non_fmt_panic.rs
(arg_span.until(open.shrink_to_hi()), "".into()),
(close.until(arg_span.shrink_to_hi()), "".into()),
],
Applicability::MachineApplicable,
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this doesn't actually work due to a bug in cargo: rust-lang/rustfix#141

@m-ou-se m-ou-se deleted the panic-format-lint branch March 15, 2021 14:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@estebank estebank

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-confusing Diagnostics: Confusing error or lint that should be reworked. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

Invalid suggestion for panic![123] Better suggestion for panic!(format!(..)) Wrong span used in non_fmt_panic lint for panic!(a!())
7 participants
@m-ou-se @rust-log-analyzer @estebank @bors @jyn514 @rust-highfive @rustbot