From ef778e796502691ebfb1f3dd5c03248aea6d2ac8 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 18:14:23 +0100 Subject: [PATCH 1/9] Fix span in non_fmt_panic for panic!(some_macro!()). --- compiler/rustc_lint/src/non_fmt_panic.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/non_fmt_panic.rs b/compiler/rustc_lint/src/non_fmt_panic.rs index e98297b692c92..0c5456cf619d1 100644 --- a/compiler/rustc_lint/src/non_fmt_panic.rs +++ b/compiler/rustc_lint/src/non_fmt_panic.rs @@ -69,19 +69,30 @@ 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!())`. + let mut arg_span = arg.span; + while !span.contains(arg_span) { + let expn = arg_span.ctxt().outer_expn_data(); + if expn.is_root() { + break; + } + 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) { 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), + span.until(arg_span), "or use std::panic::panic_any instead", "std::panic::panic_any(".into(), Applicability::MachineApplicable, From 1abc1e2a43c116a87bd430fa14a86e67617b861a Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 18:17:34 +0100 Subject: [PATCH 2/9] Add test for non_fmt_panic lint for panic!(some_macro!()). --- src/test/ui/non-fmt-panic.rs | 6 ++++++ src/test/ui/non-fmt-panic.stderr | 18 +++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/test/ui/non-fmt-panic.rs b/src/test/ui/non-fmt-panic.rs index 25c53316e1290..ff9e3b497f23d 100644 --- a/src/test/ui/non-fmt-panic.rs +++ b/src/test/ui/non-fmt-panic.rs @@ -29,6 +29,12 @@ 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 + // Check that the lint only triggers for std::panic and core::panic, // not any panic macro: macro_rules! panic { diff --git a/src/test/ui/non-fmt-panic.stderr b/src/test/ui/non-fmt-panic.stderr index 45187c518c423..2f33f04cb19f6 100644 --- a/src/test/ui/non-fmt-panic.stderr +++ b/src/test/ui/non-fmt-panic.stderr @@ -183,5 +183,21 @@ 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: 15 warnings emitted From a428ab17abc310c17ca13eeb6f74b3c5bddff940 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 18:52:47 +0100 Subject: [PATCH 3/9] Improve suggestion for panic!(format!(..)). --- compiler/rustc_lint/src/non_fmt_panic.rs | 31 +++++++++++++++++++++++- compiler/rustc_span/src/symbol.rs | 1 + library/alloc/src/macros.rs | 1 + 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/non_fmt_panic.rs b/compiler/rustc_lint/src/non_fmt_panic.rs index 0c5456cf619d1..7432f476d7cd0 100644 --- a/compiler/rustc_lint/src/non_fmt_panic.rs +++ b/compiler/rustc_lint/src/non_fmt_panic.rs @@ -72,18 +72,38 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc // Find the span of the argument to `panic!()`, before expansion in the // case of `panic!(some_macro!())`. 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(inner) = find_inner_span(cx, arg_span) { + l.multipart_suggestion( + "remove the `format!(..)` macro call", + vec![ + (arg_span.until(inner), "".into()), + (inner.between(arg_span.shrink_to_hi()), "".into()), + ], + Applicability::MachineApplicable, + ); + } + } else { l.span_suggestion_verbose( arg_span.shrink_to_lo(), "add a \"{}\" format string to Display the message", @@ -186,6 +206,15 @@ fn check_panic_str<'tcx>( } } +/// Given the span of `some_macro!(args)`, gives the span of `args`. +fn find_inner_span<'tcx>(cx: &LateContext<'tcx>, span: Span) -> Option { + let snippet = cx.sess().parse_sess.source_map().span_to_snippet(span).ok()?; + Some(span.from_inner(InnerSpan { + start: snippet.find(&['(', '{', '['][..])? + 1, + end: snippet.rfind(&[')', '}', ']'][..])?, + })) +} + fn panic_call<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>) -> (Span, Symbol) { let mut expn = f.span.ctxt().outer_expn_data(); diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 20e4f7262acb9..831e82559adcc 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -554,6 +554,7 @@ symbols! { format_args, format_args_capture, format_args_nl, + format_macro, freeze, freg, frem_fast, diff --git a/library/alloc/src/macros.rs b/library/alloc/src/macros.rs index a64a8b32ad77f..88a6cec3a8342 100644 --- a/library/alloc/src/macros.rs +++ b/library/alloc/src/macros.rs @@ -107,6 +107,7 @@ macro_rules! vec { /// ``` #[macro_export] #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "format_macro"] macro_rules! format { ($($arg:tt)*) => {{ let res = $crate::fmt::format($crate::__export::format_args!($($arg)*)); From 9f97a0b364bcbb261f05d6ac62436b6802bfa80b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 18:53:10 +0100 Subject: [PATCH 4/9] Add test for panic!(format!(..)) lint. --- src/test/ui/non-fmt-panic.rs | 2 ++ src/test/ui/non-fmt-panic.stderr | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/test/ui/non-fmt-panic.rs b/src/test/ui/non-fmt-panic.rs index ff9e3b497f23d..c20df5ffd10a0 100644 --- a/src/test/ui/non-fmt-panic.rs +++ b/src/test/ui/non-fmt-panic.rs @@ -35,6 +35,8 @@ fn main() { panic!(a!()); //~ WARN panic message is not a string literal + panic!(format!("{}", 1)); //~ 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 { diff --git a/src/test/ui/non-fmt-panic.stderr b/src/test/ui/non-fmt-panic.stderr index 2f33f04cb19f6..043c4b0f7506c 100644 --- a/src/test/ui/non-fmt-panic.stderr +++ b/src/test/ui/non-fmt-panic.stderr @@ -199,5 +199,18 @@ help: or use std::panic::panic_any instead LL | std::panic::panic_any(a!()); | ^^^^^^^^^^^^^^^^^^^^^^ -warning: 15 warnings emitted +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: 16 warnings emitted From 37c532c010cddb8f9283fb50318d8c3816646063 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 19:43:07 +0100 Subject: [PATCH 5/9] Suggest correct replacement for panic![123]. Before this change, the suggestion was `std::panic::panic_any(123]`, changing the opening brace but not the closing one. --- compiler/rustc_lint/src/non_fmt_panic.rs | 48 ++++++++++++++++-------- src/test/ui/non-fmt-panic.stderr | 8 ++-- 2 files changed, 37 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_lint/src/non_fmt_panic.rs b/compiler/rustc_lint/src/non_fmt_panic.rs index 7432f476d7cd0..c4627745648d3 100644 --- a/compiler/rustc_lint/src/non_fmt_panic.rs +++ b/compiler/rustc_lint/src/non_fmt_panic.rs @@ -93,12 +93,12 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc 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(inner) = find_inner_span(cx, arg_span) { + if let Some((open, close, _)) = find_delimiters(cx, arg_span) { l.multipart_suggestion( "remove the `format!(..)` macro call", vec![ - (arg_span.until(inner), "".into()), - (inner.between(arg_span.shrink_to_hi()), "".into()), + (arg_span.until(open.shrink_to_hi()), "".into()), + (close.until(arg_span.shrink_to_hi()), "".into()), ], Applicability::MachineApplicable, ); @@ -111,12 +111,20 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc 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(); @@ -206,13 +214,23 @@ fn check_panic_str<'tcx>( } } -/// Given the span of `some_macro!(args)`, gives the span of `args`. -fn find_inner_span<'tcx>(cx: &LateContext<'tcx>, span: Span) -> Option { +/// 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()?; - Some(span.from_inner(InnerSpan { - start: snippet.find(&['(', '{', '['][..])? + 1, - end: snippet.rfind(&[')', '}', ']'][..])?, - })) + 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) { diff --git a/src/test/ui/non-fmt-panic.stderr b/src/test/ui/non-fmt-panic.stderr index 043c4b0f7506c..001f066ad212a 100644 --- a/src/test/ui/non-fmt-panic.stderr +++ b/src/test/ui/non-fmt-panic.stderr @@ -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 @@ -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 @@ -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 @@ -197,7 +197,7 @@ 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 From 8ad12bb9b2709678f58e780daca981aafc2810dd Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 19:44:57 +0100 Subject: [PATCH 6/9] Add ui tests for panic![123] and panic!{123}. --- src/test/ui/non-fmt-panic.rs | 3 +++ src/test/ui/non-fmt-panic.stderr | 34 +++++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/test/ui/non-fmt-panic.rs b/src/test/ui/non-fmt-panic.rs index c20df5ffd10a0..c80a90b3eaaac 100644 --- a/src/test/ui/non-fmt-panic.rs +++ b/src/test/ui/non-fmt-panic.rs @@ -37,6 +37,9 @@ fn main() { 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 { diff --git a/src/test/ui/non-fmt-panic.stderr b/src/test/ui/non-fmt-panic.stderr index 001f066ad212a..7a333b3e76abe 100644 --- a/src/test/ui/non-fmt-panic.stderr +++ b/src/test/ui/non-fmt-panic.stderr @@ -212,5 +212,37 @@ help: remove the `format!(..)` macro call LL | panic!("{}", 1); | -- -- -warning: 16 warnings emitted +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 From 2a0c42450ec34f2c4b3985c159c499560f1c8270 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 19:51:15 +0100 Subject: [PATCH 7/9] Formatting. --- compiler/rustc_lint/src/non_fmt_panic.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_lint/src/non_fmt_panic.rs b/compiler/rustc_lint/src/non_fmt_panic.rs index c4627745648d3..0c7c8b78845cf 100644 --- a/compiler/rustc_lint/src/non_fmt_panic.rs +++ b/compiler/rustc_lint/src/non_fmt_panic.rs @@ -221,14 +221,8 @@ fn find_delimiters<'tcx>(cx: &LateContext<'tcx>, span: Span) -> Option<(Span, Sp 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, - }), + span.from_inner(InnerSpan { start: open, end: open + 1 }), + span.from_inner(InnerSpan { start: close, end: close + 1 }), open_ch, )) } From daa371d1891f6833ad08542caeb3cf424483a0f9 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 14 Feb 2021 20:03:13 +0100 Subject: [PATCH 8/9] Only define rustc_diagnostic_item format_macro in not(test). --- library/alloc/src/macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/macros.rs b/library/alloc/src/macros.rs index 88a6cec3a8342..6a64587a2237f 100644 --- a/library/alloc/src/macros.rs +++ b/library/alloc/src/macros.rs @@ -107,7 +107,7 @@ macro_rules! vec { /// ``` #[macro_export] #[stable(feature = "rust1", since = "1.0.0")] -#[rustc_diagnostic_item = "format_macro"] +#[cfg_attr(not(test), rustc_diagnostic_item = "format_macro")] macro_rules! format { ($($arg:tt)*) => {{ let res = $crate::fmt::format($crate::__export::format_args!($($arg)*)); From ad93f48d770cc8cfe473e809700201c31550bc68 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 17 Feb 2021 10:51:22 +0100 Subject: [PATCH 9/9] Add comment about how we find the right span in non_fmt_panic. --- compiler/rustc_lint/src/non_fmt_panic.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/rustc_lint/src/non_fmt_panic.rs b/compiler/rustc_lint/src/non_fmt_panic.rs index 0c7c8b78845cf..bfe37ce6959e7 100644 --- a/compiler/rustc_lint/src/non_fmt_panic.rs +++ b/compiler/rustc_lint/src/non_fmt_panic.rs @@ -71,6 +71,9 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc // 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) {