Skip to content

Commit

Permalink
Rollup merge of #82113 - m-ou-se:panic-format-lint, r=estebank
Browse files Browse the repository at this point in the history
Improve non_fmt_panic lint.

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`
  • Loading branch information
Dylan-DPC committed Feb 23, 2021
2 parents 18d1284 + ad93f48 commit 547b3ad
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 13 deletions.
73 changes: 64 additions & 9 deletions compiler/rustc_lint/src/non_fmt_panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,65 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc

let (span, panic) = panic_call(cx, f);

cx.struct_span_lint(NON_FMT_PANIC, arg.span, |lint| {
// Find the span of the argument to `panic!()`, before expansion in the
// case of `panic!(some_macro!())`.
// We don't use source_callsite(), because this `panic!(..)` might itself
// be expanded from another macro, in which case we want to stop at that
// expansion.
let mut arg_span = arg.span;
let mut arg_macro = None;
while !span.contains(arg_span) {
let expn = arg_span.ctxt().outer_expn_data();
if expn.is_root() {
break;
}
arg_macro = expn.macro_def_id;
arg_span = expn.call_site;
}

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");
if span.contains(arg.span) {
if !span.contains(arg_span) {
// No clue where this argument is coming from.
l.emit();
return;
}
if arg_macro.map_or(false, |id| cx.tcx.is_diagnostic_item(sym::format_macro, id)) {
// A case of `panic!(format!(..))`.
l.note("the panic!() macro supports formatting, so there's no need for the format!() macro here");
if let Some((open, close, _)) = find_delimiters(cx, arg_span) {
l.multipart_suggestion(
"remove the `format!(..)` macro call",
vec![
(arg_span.until(open.shrink_to_hi()), "".into()),
(close.until(arg_span.shrink_to_hi()), "".into()),
],
Applicability::MachineApplicable,
);
}
} else {
l.span_suggestion_verbose(
arg.span.shrink_to_lo(),
arg_span.shrink_to_lo(),
"add a \"{}\" format string to Display the message",
"\"{}\", ".into(),
Applicability::MaybeIncorrect,
);
if panic == sym::std_panic_macro {
l.span_suggestion_verbose(
span.until(arg.span),
"or use std::panic::panic_any instead",
"std::panic::panic_any(".into(),
Applicability::MachineApplicable,
);
if let Some((open, close, del)) = find_delimiters(cx, span) {
l.multipart_suggestion(
"or use std::panic::panic_any instead",
if del == '(' {
vec![(span.until(open), "std::panic::panic_any".into())]
} else {
vec![
(span.until(open.shrink_to_hi()), "std::panic::panic_any(".into()),
(close, ")".into()),
]
},
Applicability::MachineApplicable,
);
}
}
}
l.emit();
Expand Down Expand Up @@ -175,6 +217,19 @@ fn check_panic_str<'tcx>(
}
}

/// Given the span of `some_macro!(args);`, gives the span of `(` and `)`,
/// and the type of (opening) delimiter used.
fn find_delimiters<'tcx>(cx: &LateContext<'tcx>, span: Span) -> Option<(Span, Span, char)> {
let snippet = cx.sess().parse_sess.source_map().span_to_snippet(span).ok()?;
let (open, open_ch) = snippet.char_indices().find(|&(_, c)| "([{".contains(c))?;
let close = snippet.rfind(|c| ")]}".contains(c))?;
Some((
span.from_inner(InnerSpan { start: open, end: open + 1 }),
span.from_inner(InnerSpan { start: close, end: close + 1 }),
open_ch,
))
}

fn panic_call<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>) -> (Span, Symbol) {
let mut expn = f.span.ctxt().outer_expn_data();

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ symbols! {
format_args,
format_args_capture,
format_args_nl,
format_macro,
freeze,
freg,
frem_fast,
Expand Down
1 change: 1 addition & 0 deletions library/alloc/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ macro_rules! vec {
/// ```
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(test), rustc_diagnostic_item = "format_macro")]
macro_rules! format {
($($arg:tt)*) => {{
let res = $crate::fmt::format($crate::__export::format_args!($($arg)*));
Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/non-fmt-panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ fn main() {
fancy_panic::fancy_panic!(S);
//~^ WARN panic message is not a string literal

macro_rules! a {
() => { 123 };
}

panic!(a!()); //~ WARN panic message is not a string literal

panic!(format!("{}", 1)); //~ WARN panic message is not a string literal

panic![123]; //~ WARN panic message is not a string literal
panic!{123}; //~ WARN panic message is not a string literal

// Check that the lint only triggers for std::panic and core::panic,
// not any panic macro:
macro_rules! panic {
Expand Down
69 changes: 65 additions & 4 deletions src/test/ui/non-fmt-panic.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ LL | panic!("{}", C);
help: or use std::panic::panic_any instead
|
LL | std::panic::panic_any(C);
| ^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^

warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:20:12
Expand All @@ -109,7 +109,7 @@ LL | panic!("{}", S);
help: or use std::panic::panic_any instead
|
LL | std::panic::panic_any(S);
| ^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^

warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:21:17
Expand All @@ -125,7 +125,7 @@ LL | std::panic!("{}", 123);
help: or use std::panic::panic_any instead
|
LL | std::panic::panic_any(123);
| ^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^

warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:22:18
Expand Down Expand Up @@ -183,5 +183,66 @@ LL | fancy_panic::fancy_panic!(S);
|
= note: this is no longer accepted in Rust 2021

warning: 14 warnings emitted
warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:36:12
|
LL | panic!(a!());
| ^^^^
|
= note: this is no longer accepted in Rust 2021
help: add a "{}" format string to Display the message
|
LL | panic!("{}", a!());
| ^^^^^
help: or use std::panic::panic_any instead
|
LL | std::panic::panic_any(a!());
| ^^^^^^^^^^^^^^^^^^^^^

warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:38:12
|
LL | panic!(format!("{}", 1));
| ^^^^^^^^^^^^^^^^
|
= 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
|
LL | panic!("{}", 1);
| -- --

warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:40:12
|
LL | panic![123];
| ^^^
|
= note: this is no longer accepted in Rust 2021
help: add a "{}" format string to Display the message
|
LL | panic!["{}", 123];
| ^^^^^
help: or use std::panic::panic_any instead
|
LL | std::panic::panic_any(123);
| ^^^^^^^^^^^^^^^^^^^^^^ ^

warning: panic message is not a string literal
--> $DIR/non-fmt-panic.rs:41:12
|
LL | panic!{123};
| ^^^
|
= note: this is no longer accepted in Rust 2021
help: add a "{}" format string to Display the message
|
LL | panic!{"{}", 123};
| ^^^^^
help: or use std::panic::panic_any instead
|
LL | std::panic::panic_any(123);
| ^^^^^^^^^^^^^^^^^^^^^^ ^

warning: 18 warnings emitted

0 comments on commit 547b3ad

Please sign in to comment.