-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add lint for panic!(123)
which is not accepted in Rust 2021.
#81645
Conversation
The job Click to see the possible cause of the failure (guessed by this bot)
|
Ah, forgot to run the lint over rustc itself. Well, looks like it's working then ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code seems good, but I have one concern around backwards compat.
@@ -168,7 +168,7 @@ macro_rules! late_lint_passes { | |||
ClashingExternDeclarations: ClashingExternDeclarations::new(), | |||
DropTraitConstraints: DropTraitConstraints, | |||
TemporaryCStringAsPtr: TemporaryCStringAsPtr, | |||
PanicFmt: PanicFmt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to PanicFmt
?
|
||
cx.struct_span_lint(NON_FMT_PANIC, arg.span, |lint| { | ||
let mut l = lint.build("panic message is not a string literal"); | ||
l.note("this is no longer accepted in Rust 2021"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we always mention the "edition" when talking about them: "Rust 2021 edition"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There used to be a mix of "Rust 20xx" and "edition 20xx". As part of #79576 this was changed to consistently use "Rust 20xx" everywhere: c574ded
E.g.
rust/compiler/rustc_parse/src/parser/item.rs
Lines 1678 to 1679 in d4e3570
struct_span_err!(diag, span, E0670, "`async fn` is not permitted in Rust 2015") | |
.span_label(span, "to use `async fn`, switch to Rust 2018 or later") |
/// an `i32` as message. | ||
/// | ||
/// Rust 2021 always interprets the first argument as format string. | ||
NON_FMT_PANIC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly concerned: we are changing the name of an existing lint. What will happen to anyone that has already used #![deny(fmt_panic)]
or #![allow(fmt_panic)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will show warning: unknown lint: `fmt_panic`
. fmt_panic
has only been there since .1.50, which is still in beta. I suppose we could also change the name there before it gets to stable. But I don't know how important this is. The lint does new things, so using the same name might also not be a great idea, as deny
or allow
will then deny or allow more things than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather we updated beta as well. The fact that the lint does more now is... well, we would be creating a new lint because we'd be having the same trade-offs, right?
Another alternative is to leave a "dead" fmt_panic
lint that does nothing just as a placeholder...
We're close to a release. Would you mind taking over potentially changing the existing lint's name in beta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change of the lint name was motivated by: #78088 (comment) Not sure how to do this in a backwards-compatible way, or how much backwards compatibility matters for this lint. |
Why not suggesting panic!("123"); instead of panic!("{}",123); for some simple types? |
@klensy Because it's very uncommon for someone to literally have |
The job Click to see the possible cause of the failure (guessed by this bot)
|
aab9a10
to
5f66fba
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors r+ |
📌 Commit 717355d has been approved by |
Add lint for `panic!(123)` which is not accepted in Rust 2021. This extends the `panic_fmt` lint to warn for all cases where the first argument cannot be interpreted as a format string, as will happen in Rust 2021. It suggests to add `"{}",` to format the message as a string. In the case of `std::panic!()`, it also suggests the recently stabilized `std::panic::panic_any()` function as an alternative. It renames the lint to `non_fmt_panic` to match the lint naming guidelines. ![image](https://user-images.githubusercontent.com/783247/106520928-675ea680-64d5-11eb-81f7-d8fa48b93a0b.png) This is part of rust-lang#80162. r? `@estebank`
Changed an Waiting for the checks to complete, as (unlike before) the
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
Rollup of 9 pull requests Successful merges: - rust-lang#74304 (Stabilize the Wake trait) - rust-lang#79805 (Rename Iterator::fold_first to reduce and stabilize it) - rust-lang#81556 (introduce future-compatibility warning for forbidden lint groups) - rust-lang#81645 (Add lint for `panic!(123)` which is not accepted in Rust 2021.) - rust-lang#81710 (OsStr eq_ignore_ascii_case takes arg by value) - rust-lang#81711 (add #[inline] to all the public IpAddr functions) - rust-lang#81725 (Move test to be with the others) - rust-lang#81727 (Revert stabilizing integer::BITS.) - rust-lang#81745 (Stabilize poison API of Once, rename poisoned()) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This seems like it is going to cause a lot of churn (for example, it broke Cargo's CI). It's not clear why this needs to be a warning. Was it considered to make this allowed on 2018 and make it part of the rust-2021-compatibility group? |
@ehuss See rfc 3007. The idea was that But if this too often causes warnings for cases where it was intentional, we should probably only warn in a subset of the cases by default, and make the rest a allow-by-default migration lint. Note that as a lint, it doesn't show up for dependencies at all. So it shouldn't break any builds unless you're bulding with |
Another problem is the suggested alternative ( EDIT: The way mentioned in this comment works well on stable rust. |
Hm, ok after reading the justifications I think it makes more sense. It just seemed a little surprising to me because the following: assert!(stuff.contains("thing"), stuff); doesn't seem unreasonable (display the string if it doesn't contain the text). It is not immediately obvious to me from the warning why that wouldn't be allowed, since panics have always supported arbitrary payloads. Cargo denies warnings on CI, just like rust-lang/rust does. I've been contemplating how to provide better explanations for lints, and I think this is a good example. I'd like to make the descriptions in the lint listing more accessible (maybe via something like the |
Fix warnings of the new non_fmt_panic lint This fixes the CI failure since the latest nightly. See rust-lang/rust#81645
Use format string in bootstrap panic instead of a string directly This fixes the following warning when compiling with nightly: ``` warning: panic message is not a string literal --> src/bootstrap/builder.rs:1515:24 | 1515 | panic!(out); | ^^^ | = note: `#[warn(non_fmt_panic)]` on by default = note: this is no longer accepted in Rust 2021 help: add a "{}" format string to Display the message | 1515 | panic!("{}", out); | ^^^^^ help: or use std::panic::panic_any instead | 1515 | std::panic::panic_any(out); | ^^^^^^^^^^^^^^^^^^^^^^ ``` Found while working on rust-lang#79540. cc rust-lang#81645, which landed in 1.51.
Fix warnings of the new non_fmt_panic lint This fixes the CI failure since the latest nightly. See rust-lang/rust#81645
See <rust-lang/rust#81645> for example
Rust 2021 makes std::panic and core::panic identical. This caused a bunch of warnings to pop up. see: rust-lang/rust#81645 and: rust-lang/rust#80162 This updates the use of panic! and debug_assert to the new format.
This extends the
panic_fmt
lint to warn for all cases where the first argument cannot be interpreted as a format string, as will happen in Rust 2021.It suggests to add
"{}",
to format the message as a string. In the case ofstd::panic!()
, it also suggests the recently stabilizedstd::panic::panic_any()
function as an alternative.It renames the lint to
non_fmt_panic
to match the lint naming guidelines.This is part of #80162.
r? @estebank