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.

Sign up for GitHub

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

Jump to bottom

Improve non_fmt_panic lint. #82113

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.

Sign in 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.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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
7 participants
@m-ou-se @rust-log-analyzer @estebank @bors @jyn514 @rust-highfive @rustbot