From a46679098f4d586b4fd46a63f6594b7282ea18cb Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:24:15 +0200 Subject: [PATCH 01/28] Add lint to warn about braces in a panic message. --- compiler/rustc_lint/src/lib.rs | 3 ++ compiler/rustc_lint/src/panic_fmt.rs | 57 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 compiler/rustc_lint/src/panic_fmt.rs diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 24bfdad970a1c..81549be4b0915 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -55,6 +55,7 @@ mod levels; mod methods; mod non_ascii_idents; mod nonstandard_style; +mod panic_fmt; mod passes; mod redundant_semicolon; mod traits; @@ -80,6 +81,7 @@ use internal::*; use methods::*; use non_ascii_idents::*; use nonstandard_style::*; +use panic_fmt::PanicFmt; use redundant_semicolon::*; use traits::*; use types::*; @@ -166,6 +168,7 @@ macro_rules! late_lint_passes { ClashingExternDeclarations: ClashingExternDeclarations::new(), DropTraitConstraints: DropTraitConstraints, TemporaryCStringAsPtr: TemporaryCStringAsPtr, + PanicFmt: PanicFmt, ] ); }; diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs new file mode 100644 index 0000000000000..0d3649ec543b9 --- /dev/null +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -0,0 +1,57 @@ +use crate::{LateContext, LateLintPass, LintContext}; +use rustc_ast as ast; +use rustc_hir as hir; +use rustc_middle::ty; + +declare_lint! { + /// The `panic_fmt` lint detects `panic!("..")` with `{` or `}` in the string literal. + /// + /// ### Example + /// + /// ```rust,no_run + /// panic!("{}"); + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// `panic!("{}")` panics with the message `"{}"`, as a `panic!()` invocation + /// with a single argument does not use `format_args!()`. + /// A future version of Rust will interpret this string as format string, + /// which would break this. + PANIC_FMT, + Warn, + "detect braces in single-argument panic!() invocations", + report_in_external_macro +} + +declare_lint_pass!(PanicFmt => [PANIC_FMT]); + +impl<'tcx> LateLintPass<'tcx> for PanicFmt { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) { + if let hir::ExprKind::Call(f, [arg]) = &expr.kind { + if let &ty::FnDef(def_id, _) = cx.typeck_results().expr_ty(f).kind() { + if Some(def_id) == cx.tcx.lang_items().begin_panic_fn() + || Some(def_id) == cx.tcx.lang_items().panic_fn() + { + check_panic(cx, f, arg); + } + } + } + } +} + +fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tcx hir::Expr<'tcx>) { + if let hir::ExprKind::Lit(lit) = &arg.kind { + if let ast::LitKind::Str(sym, _) = lit.node { + if sym.as_str().contains(&['{', '}'][..]) { + cx.struct_span_lint(PANIC_FMT, f.span, |lint| { + lint.build("Panic message contains a brace") + .note("This message is not used as a format string, but will be in a future Rust version") + .emit(); + }); + } + } + } +} From 5b3b80a710bb9462541fc77f671b64e2651f1ec1 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:19:13 +0200 Subject: [PATCH 02/28] Allow #[rustc_diagnostic_item] on macros. --- compiler/rustc_passes/src/diagnostic_items.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/rustc_passes/src/diagnostic_items.rs b/compiler/rustc_passes/src/diagnostic_items.rs index 0f4aa72d5c47b..5a087c41f58b9 100644 --- a/compiler/rustc_passes/src/diagnostic_items.rs +++ b/compiler/rustc_passes/src/diagnostic_items.rs @@ -113,6 +113,10 @@ fn collect<'tcx>(tcx: TyCtxt<'tcx>) -> FxHashMap { } } + for m in tcx.hir().krate().exported_macros { + collector.observe_item(m.attrs, m.hir_id); + } + collector.items } From 462ee9c1b5eb0ae759e2fcc6f35dc5db4fc04367 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:20:19 +0200 Subject: [PATCH 03/28] Mark the panic macros as diagnostic items. --- library/core/src/macros/mod.rs | 1 + library/std/src/macros.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/library/core/src/macros/mod.rs b/library/core/src/macros/mod.rs index fe3eff04b4ae5..4999933fee54e 100644 --- a/library/core/src/macros/mod.rs +++ b/library/core/src/macros/mod.rs @@ -2,6 +2,7 @@ #[macro_export] #[allow_internal_unstable(core_panic, const_caller_location)] #[stable(feature = "core", since = "1.6.0")] +#[rustc_diagnostic_item = "core_panic_macro"] macro_rules! panic { () => ( $crate::panic!("explicit panic") diff --git a/library/std/src/macros.rs b/library/std/src/macros.rs index 57649d6f8f252..d003dd9fe84b8 100644 --- a/library/std/src/macros.rs +++ b/library/std/src/macros.rs @@ -8,6 +8,7 @@ #[macro_export] #[stable(feature = "rust1", since = "1.0.0")] #[allow_internal_unstable(libstd_sys_internals)] +#[rustc_diagnostic_item = "std_panic_macro"] macro_rules! panic { () => ({ $crate::panic!("explicit panic") }); ($msg:expr $(,)?) => ({ $crate::rt::begin_panic($msg) }); From da66a501f6423d498627453dd7d0f8bc874ee42d Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:26:36 +0200 Subject: [PATCH 04/28] Specialize panic_fmt lint for the {core,std}::panic!() macros. It now only reacts to expansion of those macros, and suggests inserting `"{}", ` in the right place. --- compiler/rustc_lint/src/panic_fmt.rs | 27 ++++++++++++++++++++++----- compiler/rustc_span/src/symbol.rs | 2 ++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 0d3649ec543b9..e0752515377fe 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -1,7 +1,9 @@ use crate::{LateContext, LateLintPass, LintContext}; use rustc_ast as ast; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_middle::ty; +use rustc_span::sym; declare_lint! { /// The `panic_fmt` lint detects `panic!("..")` with `{` or `}` in the string literal. @@ -46,11 +48,26 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc if let hir::ExprKind::Lit(lit) = &arg.kind { if let ast::LitKind::Str(sym, _) = lit.node { if sym.as_str().contains(&['{', '}'][..]) { - cx.struct_span_lint(PANIC_FMT, f.span, |lint| { - lint.build("Panic message contains a brace") - .note("This message is not used as a format string, but will be in a future Rust version") - .emit(); - }); + let expn = f.span.ctxt().outer_expn_data(); + if let Some(id) = expn.macro_def_id { + if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) + || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) + { + cx.struct_span_lint(PANIC_FMT, expn.call_site, |lint| { + let mut l = lint.build("Panic message contains a brace"); + l.note("This message is not used as a format string, but will be in a future Rust version"); + if expn.call_site.contains(arg.span) { + l.span_suggestion( + arg.span.shrink_to_lo(), + "add a \"{}\" format string to use the message literally", + "\"{}\", ".into(), + Applicability::MachineApplicable, + ); + } + l.emit(); + }); + } + } } } } diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 3a2a3adce35c9..c36d638077198 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -393,6 +393,7 @@ symbols! { copysignf64, core, core_intrinsics, + core_panic_macro, cosf32, cosf64, crate_id, @@ -1064,6 +1065,7 @@ symbols! { staticlib, std, std_inject, + std_panic_macro, stmt, stmt_expr_attributes, stop_after_dataflow, From f228efc3f56ca13a4a969d0ee72c2e0844ac6a72 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:29:40 +0200 Subject: [PATCH 05/28] Make panic_fmt lint work properly for assert!(expr, msg) too. --- compiler/rustc_lint/src/panic_fmt.rs | 12 ++++++++++++ compiler/rustc_span/src/symbol.rs | 1 + library/core/src/macros/mod.rs | 1 + 3 files changed, 14 insertions(+) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index e0752515377fe..198797974ff0c 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -53,6 +53,18 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) { + let expn = { + // Unwrap another level of macro expansion if this + // panic!() was expanded from assert!(). + let parent = expn.call_site.ctxt().outer_expn_data(); + if parent.macro_def_id.map_or(false, |id| { + cx.tcx.is_diagnostic_item(sym::assert_macro, id) + }) { + parent + } else { + expn + } + }; cx.struct_span_lint(PANIC_FMT, expn.call_site, |lint| { let mut l = lint.build("Panic message contains a brace"); l.note("This message is not used as a format string, but will be in a future Rust version"); diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index c36d638077198..c6949d9387c20 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -267,6 +267,7 @@ symbols! { asm, assert, assert_inhabited, + assert_macro, assert_receiver_is_total_eq, assert_uninit_valid, assert_zero_valid, diff --git a/library/core/src/macros/mod.rs b/library/core/src/macros/mod.rs index 4999933fee54e..9f28186a33c15 100644 --- a/library/core/src/macros/mod.rs +++ b/library/core/src/macros/mod.rs @@ -1216,6 +1216,7 @@ pub(crate) mod builtin { #[stable(feature = "rust1", since = "1.0.0")] #[rustc_builtin_macro] #[macro_export] + #[rustc_diagnostic_item = "assert_macro"] macro_rules! assert { ($cond:expr $(,)?) => {{ /* compiler built-in */ }}; ($cond:expr, $($arg:tt)+) => {{ /* compiler built-in */ }}; From 3beb2e95a98e90f43809a9ab1fb7175d4fa7aa8d Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:30:16 +0200 Subject: [PATCH 06/28] Expand assert!(expr) to panic() function instead of panic!() macro. The panic message might contain braces which should never be interpreted as format placeholders, which panic!() will do in a future edition. --- compiler/rustc_builtin_macros/src/assert.rs | 62 ++++++++++++--------- compiler/rustc_span/src/symbol.rs | 1 + library/core/src/macros/mod.rs | 1 + 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/assert.rs b/compiler/rustc_builtin_macros/src/assert.rs index 5bfd8a2bf561c..bc3276538da8f 100644 --- a/compiler/rustc_builtin_macros/src/assert.rs +++ b/compiler/rustc_builtin_macros/src/assert.rs @@ -1,8 +1,8 @@ use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc_ast::ptr::P; -use rustc_ast::token::{self, TokenKind}; -use rustc_ast::tokenstream::{DelimSpan, TokenStream, TokenTree}; +use rustc_ast::token; +use rustc_ast::tokenstream::{DelimSpan, TokenStream}; use rustc_ast::{self as ast, *}; use rustc_ast_pretty::pprust; use rustc_expand::base::*; @@ -26,31 +26,41 @@ pub fn expand_assert<'cx>( // `core::panic` and `std::panic` are different macros, so we use call-site // context to pick up whichever is currently in scope. let sp = cx.with_call_site_ctxt(sp); - let tokens = custom_message.unwrap_or_else(|| { - TokenStream::from(TokenTree::token( - TokenKind::lit( - token::Str, - Symbol::intern(&format!( - "assertion failed: {}", - pprust::expr_to_string(&cond_expr).escape_debug() - )), - None, - ), - DUMMY_SP, - )) - }); - let args = P(MacArgs::Delimited(DelimSpan::from_single(sp), MacDelimiter::Parenthesis, tokens)); - let panic_call = MacCall { - path: Path::from_ident(Ident::new(sym::panic, sp)), - args, - prior_type_ascription: None, + + let panic_call = { + if let Some(tokens) = custom_message { + // Pass the custom message to panic!(). + cx.expr( + sp, + ExprKind::MacCall(MacCall { + path: Path::from_ident(Ident::new(sym::panic, sp)), + args: P(MacArgs::Delimited( + DelimSpan::from_single(sp), + MacDelimiter::Parenthesis, + tokens, + )), + prior_type_ascription: None, + }), + ) + } else { + // Pass our own message directly to $crate::panicking::panic(), + // because it might contain `{` and `}` that should always be + // passed literally. + cx.expr_call_global( + sp, + cx.std_path(&[sym::panicking, sym::panic]), + vec![cx.expr_str( + DUMMY_SP, + Symbol::intern(&format!( + "assertion failed: {}", + pprust::expr_to_string(&cond_expr).escape_debug() + )), + )], + ) + } }; - let if_expr = cx.expr_if( - sp, - cx.expr(sp, ExprKind::Unary(UnOp::Not, cond_expr)), - cx.expr(sp, ExprKind::MacCall(panic_call)), - None, - ); + let if_expr = + cx.expr_if(sp, cx.expr(sp, ExprKind::Unary(UnOp::Not, cond_expr)), panic_call, None); MacEager::expr(if_expr) } diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index c6949d9387c20..733d2b1ef9ab2 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -791,6 +791,7 @@ symbols! { panic_runtime, panic_str, panic_unwind, + panicking, param_attrs, parent_trait, partial_cmp, diff --git a/library/core/src/macros/mod.rs b/library/core/src/macros/mod.rs index 9f28186a33c15..53e6b4598392c 100644 --- a/library/core/src/macros/mod.rs +++ b/library/core/src/macros/mod.rs @@ -1217,6 +1217,7 @@ pub(crate) mod builtin { #[rustc_builtin_macro] #[macro_export] #[rustc_diagnostic_item = "assert_macro"] + #[allow_internal_unstable(core_panic)] macro_rules! assert { ($cond:expr $(,)?) => {{ /* compiler built-in */ }}; ($cond:expr, $($arg:tt)+) => {{ /* compiler built-in */ }}; From 451c9864c498f715c39b52dbbf0c6258c608c13f Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 22:52:36 +0200 Subject: [PATCH 07/28] Add test for the panic_fmt lint. --- src/test/ui/panic-brace.rs | 9 ++++++ src/test/ui/panic-brace.stderr | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 src/test/ui/panic-brace.rs create mode 100644 src/test/ui/panic-brace.stderr diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs new file mode 100644 index 0000000000000..744ecb30037cd --- /dev/null +++ b/src/test/ui/panic-brace.rs @@ -0,0 +1,9 @@ +// build-pass (FIXME(62277): should be check-pass) + +#[allow(unreachable_code)] +fn main() { + panic!("here's a brace: {"); //~ WARN Panic message contains a brace + std::panic!("another one: }"); //~ WARN Panic message contains a brace + core::panic!("Hello { { {"); //~ WARN Panic message contains a brace + assert!(false, "} } }..."); //~ WARN Panic message contains a brace +} diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr new file mode 100644 index 0000000000000..c54044cb6c81f --- /dev/null +++ b/src/test/ui/panic-brace.stderr @@ -0,0 +1,51 @@ +warning: Panic message contains a brace + --> $DIR/panic-brace.rs:5:5 + | +LL | panic!("here's a brace: {"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(panic_fmt)]` on by default + = note: This message is not used as a format string, but will be in a future Rust version +help: add a "{}" format string to use the message literally + | +LL | panic!("{}", "here's a brace: {"); + | ^^^^^ + +warning: Panic message contains a brace + --> $DIR/panic-brace.rs:6:5 + | +LL | std::panic!("another one: }"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: This message is not used as a format string, but will be in a future Rust version +help: add a "{}" format string to use the message literally + | +LL | std::panic!("{}", "another one: }"); + | ^^^^^ + +warning: Panic message contains a brace + --> $DIR/panic-brace.rs:7:5 + | +LL | core::panic!("Hello { { {"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: This message is not used as a format string, but will be in a future Rust version +help: add a "{}" format string to use the message literally + | +LL | core::panic!("{}", "Hello { { {"); + | ^^^^^ + +warning: Panic message contains a brace + --> $DIR/panic-brace.rs:8:5 + | +LL | assert!(false, "} } }..."); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: This message is not used as a format string, but will be in a future Rust version +help: add a "{}" format string to use the message literally + | +LL | assert!(false, "{}", "} } }..."); + | ^^^^^ + +warning: 4 warnings emitted + From ded269fa102df8716a540741ee7da6ce40e9738c Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 23:25:06 +0200 Subject: [PATCH 08/28] Improve panic_fmt message for panic!("{}") with a fmt placeholder. --- compiler/rustc_lint/src/panic_fmt.rs | 62 ++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 198797974ff0c..6879244721bb2 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -47,24 +47,52 @@ impl<'tcx> LateLintPass<'tcx> for PanicFmt { fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tcx hir::Expr<'tcx>) { if let hir::ExprKind::Lit(lit) = &arg.kind { if let ast::LitKind::Str(sym, _) = lit.node { - if sym.as_str().contains(&['{', '}'][..]) { - let expn = f.span.ctxt().outer_expn_data(); - if let Some(id) = expn.macro_def_id { - if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) - || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) - { - let expn = { - // Unwrap another level of macro expansion if this - // panic!() was expanded from assert!(). - let parent = expn.call_site.ctxt().outer_expn_data(); - if parent.macro_def_id.map_or(false, |id| { - cx.tcx.is_diagnostic_item(sym::assert_macro, id) - }) { - parent - } else { - expn + let s = sym.as_str(); + let open = s.find('{'); + let close = s[open.unwrap_or(0)..].find('}'); + let looks_like_placeholder = match (open, close) { + (Some(_), Some(_)) => true, + (Some(_), None) | (None, Some(_)) => false, + (None, None) => return // OK, no braces. + }; + let expn = f.span.ctxt().outer_expn_data(); + if let Some(id) = expn.macro_def_id { + if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) + || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) + { + let expn = { + // Unwrap another level of macro expansion if this + // panic!() was expanded from assert!(). + let parent = expn.call_site.ctxt().outer_expn_data(); + if parent.macro_def_id.map_or(false, |id| { + cx.tcx.is_diagnostic_item(sym::assert_macro, id) + }) { + parent + } else { + expn + } + }; + if looks_like_placeholder { + cx.struct_span_lint(PANIC_FMT, arg.span.source_callsite(), |lint| { + let mut l = lint.build("Panic message contains an unused formatting placeholder"); + l.note("This message is not used as a format string when given without arguments, but will be in a future Rust version"); + if expn.call_site.contains(arg.span) { + l.span_suggestion( + arg.span.shrink_to_hi(), + "add the missing argument(s)", + ", argument".into(), + Applicability::HasPlaceholders, + ); + l.span_suggestion( + arg.span.shrink_to_lo(), + "or add a \"{}\" format string to use the message literally", + "\"{}\", ".into(), + Applicability::MaybeIncorrect, + ); } - }; + l.emit(); + }); + } else { cx.struct_span_lint(PANIC_FMT, expn.call_site, |lint| { let mut l = lint.build("Panic message contains a brace"); l.note("This message is not used as a format string, but will be in a future Rust version"); From b8a8b681b8ca37771a0116d7cc7c3e4e7c749e0e Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 23:25:57 +0200 Subject: [PATCH 09/28] Formatting. --- compiler/rustc_lint/src/panic_fmt.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 6879244721bb2..a615d57dfa705 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -53,7 +53,7 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc let looks_like_placeholder = match (open, close) { (Some(_), Some(_)) => true, (Some(_), None) | (None, Some(_)) => false, - (None, None) => return // OK, no braces. + (None, None) => return, // OK, no braces. }; let expn = f.span.ctxt().outer_expn_data(); if let Some(id) = expn.macro_def_id { @@ -64,9 +64,10 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc // Unwrap another level of macro expansion if this // panic!() was expanded from assert!(). let parent = expn.call_site.ctxt().outer_expn_data(); - if parent.macro_def_id.map_or(false, |id| { - cx.tcx.is_diagnostic_item(sym::assert_macro, id) - }) { + if parent + .macro_def_id + .map_or(false, |id| cx.tcx.is_diagnostic_item(sym::assert_macro, id)) + { parent } else { expn From 14dcf0a8ac632e6073ec979da72e286a57c50f4c Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 23:36:00 +0200 Subject: [PATCH 10/28] Test for formating placeholders in panic_fmt lint test. --- src/test/ui/panic-brace.rs | 4 ++-- src/test/ui/panic-brace.stderr | 36 +++++++++++++++++++++------------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index 744ecb30037cd..01ffb80076803 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -4,6 +4,6 @@ fn main() { panic!("here's a brace: {"); //~ WARN Panic message contains a brace std::panic!("another one: }"); //~ WARN Panic message contains a brace - core::panic!("Hello { { {"); //~ WARN Panic message contains a brace - assert!(false, "} } }..."); //~ WARN Panic message contains a brace + core::panic!("Hello {}"); //~ WARN Panic message contains an unused formatting placeholder + assert!(false, "{:03x} bla"); //~ WARN Panic message contains an unused formatting placeholder } diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr index c54044cb6c81f..b91ce8f603a55 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -23,28 +23,36 @@ help: add a "{}" format string to use the message literally LL | std::panic!("{}", "another one: }"); | ^^^^^ -warning: Panic message contains a brace - --> $DIR/panic-brace.rs:7:5 +warning: Panic message contains an unused formatting placeholder + --> $DIR/panic-brace.rs:7:18 | -LL | core::panic!("Hello { { {"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | core::panic!("Hello {}"); + | ^^^^^^^^^^ | - = note: This message is not used as a format string, but will be in a future Rust version -help: add a "{}" format string to use the message literally + = note: This message is not used as a format string when given without arguments, but will be in a future Rust version +help: add the missing argument(s) + | +LL | core::panic!("Hello {}", argument); + | ^^^^^^^^^^ +help: or add a "{}" format string to use the message literally | -LL | core::panic!("{}", "Hello { { {"); +LL | core::panic!("{}", "Hello {}"); | ^^^^^ -warning: Panic message contains a brace - --> $DIR/panic-brace.rs:8:5 +warning: Panic message contains an unused formatting placeholder + --> $DIR/panic-brace.rs:8:20 | -LL | assert!(false, "} } }..."); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | assert!(false, "{:03x} bla"); + | ^^^^^^^^^^^^ | - = note: This message is not used as a format string, but will be in a future Rust version -help: add a "{}" format string to use the message literally + = note: This message is not used as a format string when given without arguments, but will be in a future Rust version +help: add the missing argument(s) + | +LL | assert!(false, "{:03x} bla", argument); + | ^^^^^^^^^^ +help: or add a "{}" format string to use the message literally | -LL | assert!(false, "{}", "} } }..."); +LL | assert!(false, "{}", "{:03x} bla"); | ^^^^^ warning: 4 warnings emitted From dd262e385646fa94b85b5340f260efb01284261b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 18 Oct 2020 23:45:20 +0200 Subject: [PATCH 11/28] Add cfg(not(bootstrap)) on the new rustc_diagnostic_item attributes. The beta compiler doesn't accept rustc_diagnostic_items on macros yet. --- library/core/src/macros/mod.rs | 4 ++-- library/std/src/macros.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/core/src/macros/mod.rs b/library/core/src/macros/mod.rs index 53e6b4598392c..54bbedcd914d7 100644 --- a/library/core/src/macros/mod.rs +++ b/library/core/src/macros/mod.rs @@ -2,7 +2,7 @@ #[macro_export] #[allow_internal_unstable(core_panic, const_caller_location)] #[stable(feature = "core", since = "1.6.0")] -#[rustc_diagnostic_item = "core_panic_macro"] +#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "core_panic_macro")] macro_rules! panic { () => ( $crate::panic!("explicit panic") @@ -1216,7 +1216,7 @@ pub(crate) mod builtin { #[stable(feature = "rust1", since = "1.0.0")] #[rustc_builtin_macro] #[macro_export] - #[rustc_diagnostic_item = "assert_macro"] + #[cfg_attr(not(bootstrap), rustc_diagnostic_item = "assert_macro")] #[allow_internal_unstable(core_panic)] macro_rules! assert { ($cond:expr $(,)?) => {{ /* compiler built-in */ }}; diff --git a/library/std/src/macros.rs b/library/std/src/macros.rs index d003dd9fe84b8..f34eea1f8aca4 100644 --- a/library/std/src/macros.rs +++ b/library/std/src/macros.rs @@ -8,7 +8,7 @@ #[macro_export] #[stable(feature = "rust1", since = "1.0.0")] #[allow_internal_unstable(libstd_sys_internals)] -#[rustc_diagnostic_item = "std_panic_macro"] +#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "std_panic_macro")] macro_rules! panic { () => ({ $crate::panic!("explicit panic") }); ($msg:expr $(,)?) => ({ $crate::rt::begin_panic($msg) }); From 9615d27ab7327abdb2ba4aef5f797c693d02ef17 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 00:05:19 +0200 Subject: [PATCH 12/28] Don't see `{{}}` as placeholder in panic_fmt lint. --- compiler/rustc_lint/src/panic_fmt.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index a615d57dfa705..75ee0896510f0 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -47,19 +47,18 @@ impl<'tcx> LateLintPass<'tcx> for PanicFmt { fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tcx hir::Expr<'tcx>) { if let hir::ExprKind::Lit(lit) = &arg.kind { if let ast::LitKind::Str(sym, _) = lit.node { - let s = sym.as_str(); - let open = s.find('{'); - let close = s[open.unwrap_or(0)..].find('}'); - let looks_like_placeholder = match (open, close) { - (Some(_), Some(_)) => true, - (Some(_), None) | (None, Some(_)) => false, - (None, None) => return, // OK, no braces. - }; let expn = f.span.ctxt().outer_expn_data(); if let Some(id) = expn.macro_def_id { if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) { + let s = sym.as_str(); + if !s.contains(&['{', '}'][..]) { + return; + } + let s = s.replace("{{", "").replace("}}", ""); + let looks_like_placeholder = + s.find('{').map_or(false, |i| s[i + 1..].contains('}')); let expn = { // Unwrap another level of macro expansion if this // panic!() was expanded from assert!(). From 9a840a30d68506a8070c309e5f8389a7ad09e55c Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 00:05:45 +0200 Subject: [PATCH 13/28] Fix brace problem in panic message in rustc_expand. --- compiler/rustc_expand/src/mbe/macro_rules.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index a074af0189a28..66463eeb90713 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -1173,7 +1173,8 @@ fn quoted_tt_to_string(tt: &mbe::TokenTree) -> String { mbe::TokenTree::MetaVar(_, name) => format!("${}", name), mbe::TokenTree::MetaVarDecl(_, name, kind) => format!("${}:{}", name, kind), _ => panic!( - "unexpected mbe::TokenTree::{{Sequence or Delimited}} \ + "{}", + "unexpected mbe::TokenTree::{Sequence or Delimited} \ in follow set checker" ), } From 0a9330c7ef1c4eb99e8143c7a586b1649ede85b8 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 00:25:17 +0200 Subject: [PATCH 14/28] Ignore panic_fmt lint in macro-comma-behavior-rpass ui test. --- src/test/ui/macros/macro-comma-behavior-rpass.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/ui/macros/macro-comma-behavior-rpass.rs b/src/test/ui/macros/macro-comma-behavior-rpass.rs index 32cf59294e760..e5e656de6fa7f 100644 --- a/src/test/ui/macros/macro-comma-behavior-rpass.rs +++ b/src/test/ui/macros/macro-comma-behavior-rpass.rs @@ -57,6 +57,7 @@ fn writeln_1arg() { // // (Example: Issue #48042) #[test] +#[allow(panic_fmt)] fn to_format_or_not_to_format() { // ("{}" is the easiest string to test because if this gets // sent to format_args!, it'll simply fail to compile. From d3b41497fedc3362144aa3967b05987e278c376a Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 00:45:07 +0200 Subject: [PATCH 15/28] Also apply panic_fmt lint suggestions to debug_assert!(). --- compiler/rustc_lint/src/panic_fmt.rs | 16 +++++++--------- compiler/rustc_span/src/symbol.rs | 1 + library/core/src/macros/mod.rs | 1 + src/test/ui/panic-brace.rs | 1 + src/test/ui/panic-brace.stderr | 14 +++++++++++++- 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 75ee0896510f0..05bc272d0e0a0 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -47,7 +47,7 @@ impl<'tcx> LateLintPass<'tcx> for PanicFmt { fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tcx hir::Expr<'tcx>) { if let hir::ExprKind::Lit(lit) = &arg.kind { if let ast::LitKind::Str(sym, _) = lit.node { - let expn = f.span.ctxt().outer_expn_data(); + let mut expn = f.span.ctxt().outer_expn_data(); if let Some(id) = expn.macro_def_id { if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) @@ -59,19 +59,17 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc let s = s.replace("{{", "").replace("}}", ""); let looks_like_placeholder = s.find('{').map_or(false, |i| s[i + 1..].contains('}')); - let expn = { - // Unwrap another level of macro expansion if this - // panic!() was expanded from assert!(). + // Unwrap another level of macro expansion if this panic!() + // was expanded from assert!() or debug_assert!(). + for &assert in &[sym::assert_macro, sym::debug_assert_macro] { let parent = expn.call_site.ctxt().outer_expn_data(); if parent .macro_def_id - .map_or(false, |id| cx.tcx.is_diagnostic_item(sym::assert_macro, id)) + .map_or(false, |id| cx.tcx.is_diagnostic_item(assert, id)) { - parent - } else { - expn + expn = parent; } - }; + } if looks_like_placeholder { cx.struct_span_lint(PANIC_FMT, arg.span.source_callsite(), |lint| { let mut l = lint.build("Panic message contains an unused formatting placeholder"); diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 733d2b1ef9ab2..338ff005995d5 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -418,6 +418,7 @@ symbols! { dead_code, dealloc, debug, + debug_assert_macro, debug_assertions, debug_struct, debug_trait, diff --git a/library/core/src/macros/mod.rs b/library/core/src/macros/mod.rs index 54bbedcd914d7..0416a7614a3f6 100644 --- a/library/core/src/macros/mod.rs +++ b/library/core/src/macros/mod.rs @@ -163,6 +163,7 @@ macro_rules! assert_ne { /// ``` #[macro_export] #[stable(feature = "rust1", since = "1.0.0")] +#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "debug_assert_macro")] macro_rules! debug_assert { ($($arg:tt)*) => (if $crate::cfg!(debug_assertions) { $crate::assert!($($arg)*); }) } diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index 01ffb80076803..d6bb3222ac7e8 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -6,4 +6,5 @@ fn main() { std::panic!("another one: }"); //~ WARN Panic message contains a brace core::panic!("Hello {}"); //~ WARN Panic message contains an unused formatting placeholder assert!(false, "{:03x} bla"); //~ WARN Panic message contains an unused formatting placeholder + debug_assert!(false, "{{}} bla"); //~ WARN Panic message contains a brace } diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr index b91ce8f603a55..578731896ba5f 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -55,5 +55,17 @@ help: or add a "{}" format string to use the message literally LL | assert!(false, "{}", "{:03x} bla"); | ^^^^^ -warning: 4 warnings emitted +warning: Panic message contains a brace + --> $DIR/panic-brace.rs:9:5 + | +LL | debug_assert!(false, "{{}} bla"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: This message is not used as a format string, but will be in a future Rust version +help: add a "{}" format string to use the message literally + | +LL | debug_assert!(false, "{}", "{{}} bla"); + | ^^^^^ + +warning: 5 warnings emitted From f1fcc4dea319e1024b681f347cfb5ec307aaf743 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 00:45:42 +0200 Subject: [PATCH 16/28] Ignore panic_fmt lint in format-args-capture ui test. --- src/test/ui/fmt/format-args-capture.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/ui/fmt/format-args-capture.rs b/src/test/ui/fmt/format-args-capture.rs index 9348bb46dfe34..4e3fa9a3c589a 100644 --- a/src/test/ui/fmt/format-args-capture.rs +++ b/src/test/ui/fmt/format-args-capture.rs @@ -31,6 +31,7 @@ fn panic_with_single_argument_does_not_get_formatted() { // RFC #2795 suggests that this may need to change so that captured arguments are formatted. // For stability reasons this will need to part of an edition change. + #[allow(panic_fmt)] let msg = std::panic::catch_unwind(|| { panic!("{foo}"); }).unwrap_err(); From dd81c9102863a9d8b965628c87cb639430d233f9 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 09:44:57 +0200 Subject: [PATCH 17/28] Update mir-opt test output for new assert macro implementation. --- ...st_combine_deref.do_not_miscompile.InstCombine.diff | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/mir-opt/inst_combine_deref.do_not_miscompile.InstCombine.diff b/src/test/mir-opt/inst_combine_deref.do_not_miscompile.InstCombine.diff index 23c18bde2262b..dac9ec3b443d7 100644 --- a/src/test/mir-opt/inst_combine_deref.do_not_miscompile.InstCombine.diff +++ b/src/test/mir-opt/inst_combine_deref.do_not_miscompile.InstCombine.diff @@ -10,7 +10,7 @@ let mut _8: bool; // in scope 0 at $DIR/inst_combine_deref.rs:60:5: 60:23 let mut _9: bool; // in scope 0 at $DIR/inst_combine_deref.rs:60:13: 60:21 let mut _10: i32; // in scope 0 at $DIR/inst_combine_deref.rs:60:13: 60:15 - let mut _11: !; // in scope 0 at $SRC_DIR/std/src/macros.rs:LL:COL + let mut _11: !; // in scope 0 at $DIR/inst_combine_deref.rs:60:5: 60:23 scope 1 { debug x => _1; // in scope 1 at $DIR/inst_combine_deref.rs:55:9: 55:10 let _2: i32; // in scope 1 at $DIR/inst_combine_deref.rs:56:9: 56:10 @@ -69,11 +69,11 @@ } bb2: { - StorageLive(_11); // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL - begin_panic::<&str>(const "assertion failed: *y == 99"); // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL + StorageLive(_11); // scope 4 at $DIR/inst_combine_deref.rs:60:5: 60:23 + core::panicking::panic(const "assertion failed: *y == 99"); // scope 4 at $DIR/inst_combine_deref.rs:60:5: 60:23 // mir::Constant - // + span: $SRC_DIR/std/src/macros.rs:LL:COL - // + literal: Const { ty: fn(&str) -> ! {std::rt::begin_panic::<&str>}, val: Value(Scalar()) } + // + span: $DIR/inst_combine_deref.rs:60:5: 60:23 + // + literal: Const { ty: fn(&'static str) -> ! {core::panicking::panic}, val: Value(Scalar()) } // ty::Const // + ty: &str // + val: Value(Slice { data: Allocation { bytes: [97, 115, 115, 101, 114, 116, 105, 111, 110, 32, 102, 97, 105, 108, 101, 100, 58, 32, 42, 121, 32, 61, 61, 32, 57, 57], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [67108863], len: Size { raw: 26 } }, size: Size { raw: 26 }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 26 }) From 9e3b949b8c43b43f38dd79dd2b9d6f600eec9ddb Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 10:07:30 +0200 Subject: [PATCH 18/28] Fix braces in panic message in test. --- library/core/tests/nonzero.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/tests/nonzero.rs b/library/core/tests/nonzero.rs index ca449b4350ede..b66c482c5e5e3 100644 --- a/library/core/tests/nonzero.rs +++ b/library/core/tests/nonzero.rs @@ -85,7 +85,7 @@ fn test_match_option_string() { let five = "Five".to_string(); match Some(five) { Some(s) => assert_eq!(s, "Five"), - None => panic!("unexpected None while matching on Some(String { ... })"), + None => panic!("{}", "unexpected None while matching on Some(String { ... })"), } } From ff8df0bbe38d8b1216661c6544de4187e41eb45b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 10:57:44 +0200 Subject: [PATCH 19/28] Add cfg(not(test)) to std_panic_macro rustc_diagnostic_item. --- library/std/src/macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/macros.rs b/library/std/src/macros.rs index f34eea1f8aca4..de072e83dfc41 100644 --- a/library/std/src/macros.rs +++ b/library/std/src/macros.rs @@ -8,7 +8,7 @@ #[macro_export] #[stable(feature = "rust1", since = "1.0.0")] #[allow_internal_unstable(libstd_sys_internals)] -#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "std_panic_macro")] +#[cfg_attr(not(any(bootstrap, test)), rustc_diagnostic_item = "std_panic_macro")] macro_rules! panic { () => ({ $crate::panic!("explicit panic") }); ($msg:expr $(,)?) => ({ $crate::rt::begin_panic($msg) }); From 0f193d1a62c128ae94e1f21d7c1212d7c9e95b7d Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 19 Oct 2020 21:14:05 +0200 Subject: [PATCH 20/28] Small cleanups in assert!() and panic_fmt lint. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (From the PR feedback.) Co-authored-by: Esteban Küber --- compiler/rustc_builtin_macros/src/assert.rs | 60 ++++++++++----------- compiler/rustc_lint/src/panic_fmt.rs | 8 +-- src/test/ui/panic-brace.rs | 10 ++-- src/test/ui/panic-brace.stderr | 20 +++---- 4 files changed, 48 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/assert.rs b/compiler/rustc_builtin_macros/src/assert.rs index bc3276538da8f..bb6d3f6a0076c 100644 --- a/compiler/rustc_builtin_macros/src/assert.rs +++ b/compiler/rustc_builtin_macros/src/assert.rs @@ -27,37 +27,35 @@ pub fn expand_assert<'cx>( // context to pick up whichever is currently in scope. let sp = cx.with_call_site_ctxt(sp); - let panic_call = { - if let Some(tokens) = custom_message { - // Pass the custom message to panic!(). - cx.expr( - sp, - ExprKind::MacCall(MacCall { - path: Path::from_ident(Ident::new(sym::panic, sp)), - args: P(MacArgs::Delimited( - DelimSpan::from_single(sp), - MacDelimiter::Parenthesis, - tokens, - )), - prior_type_ascription: None, - }), - ) - } else { - // Pass our own message directly to $crate::panicking::panic(), - // because it might contain `{` and `}` that should always be - // passed literally. - cx.expr_call_global( - sp, - cx.std_path(&[sym::panicking, sym::panic]), - vec![cx.expr_str( - DUMMY_SP, - Symbol::intern(&format!( - "assertion failed: {}", - pprust::expr_to_string(&cond_expr).escape_debug() - )), - )], - ) - } + let panic_call = if let Some(tokens) = custom_message { + // Pass the custom message to panic!(). + cx.expr( + sp, + ExprKind::MacCall(MacCall { + path: Path::from_ident(Ident::new(sym::panic, sp)), + args: P(MacArgs::Delimited( + DelimSpan::from_single(sp), + MacDelimiter::Parenthesis, + tokens, + )), + prior_type_ascription: None, + }), + ) + } else { + // Pass our own message directly to $crate::panicking::panic(), + // because it might contain `{` and `}` that should always be + // passed literally. + cx.expr_call_global( + sp, + cx.std_path(&[sym::panicking, sym::panic]), + vec![cx.expr_str( + DUMMY_SP, + Symbol::intern(&format!( + "assertion failed: {}", + pprust::expr_to_string(&cond_expr).escape_debug() + )), + )], + ) }; let if_expr = cx.expr_if(sp, cx.expr(sp, ExprKind::Unary(UnOp::Not, cond_expr)), panic_call, None); diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 05bc272d0e0a0..288e1d61bbf49 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -72,8 +72,8 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc } if looks_like_placeholder { cx.struct_span_lint(PANIC_FMT, arg.span.source_callsite(), |lint| { - let mut l = lint.build("Panic message contains an unused formatting placeholder"); - l.note("This message is not used as a format string when given without arguments, but will be in a future Rust version"); + let mut l = lint.build("panic message contains an unused formatting placeholder"); + l.note("this message is not used as a format string when given without arguments, but will be in a future Rust version"); if expn.call_site.contains(arg.span) { l.span_suggestion( arg.span.shrink_to_hi(), @@ -92,8 +92,8 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc }); } else { cx.struct_span_lint(PANIC_FMT, expn.call_site, |lint| { - let mut l = lint.build("Panic message contains a brace"); - l.note("This message is not used as a format string, but will be in a future Rust version"); + let mut l = lint.build("panic message contains a brace"); + l.note("this message is not used as a format string, but will be in a future Rust version"); if expn.call_site.contains(arg.span) { l.span_suggestion( arg.span.shrink_to_lo(), diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index d6bb3222ac7e8..6ab5fafee887c 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -2,9 +2,9 @@ #[allow(unreachable_code)] fn main() { - panic!("here's a brace: {"); //~ WARN Panic message contains a brace - std::panic!("another one: }"); //~ WARN Panic message contains a brace - core::panic!("Hello {}"); //~ WARN Panic message contains an unused formatting placeholder - assert!(false, "{:03x} bla"); //~ WARN Panic message contains an unused formatting placeholder - debug_assert!(false, "{{}} bla"); //~ WARN Panic message contains a brace + panic!("here's a brace: {"); //~ WARN panic message contains a brace + std::panic!("another one: }"); //~ WARN panic message contains a brace + core::panic!("Hello {}"); //~ WARN panic message contains an unused formatting placeholder + assert!(false, "{:03x} bla"); //~ WARN panic message contains an unused formatting placeholder + debug_assert!(false, "{{}} bla"); //~ WARN panic message contains a brace } diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr index 578731896ba5f..00b005a59d8c6 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -1,35 +1,35 @@ -warning: Panic message contains a brace +warning: panic message contains a brace --> $DIR/panic-brace.rs:5:5 | LL | panic!("here's a brace: {"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(panic_fmt)]` on by default - = note: This message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust version help: add a "{}" format string to use the message literally | LL | panic!("{}", "here's a brace: {"); | ^^^^^ -warning: Panic message contains a brace +warning: panic message contains a brace --> $DIR/panic-brace.rs:6:5 | LL | std::panic!("another one: }"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: This message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust version help: add a "{}" format string to use the message literally | LL | std::panic!("{}", "another one: }"); | ^^^^^ -warning: Panic message contains an unused formatting placeholder +warning: panic message contains an unused formatting placeholder --> $DIR/panic-brace.rs:7:18 | LL | core::panic!("Hello {}"); | ^^^^^^^^^^ | - = note: This message is not used as a format string when given without arguments, but will be in a future Rust version + = note: this message is not used as a format string when given without arguments, but will be in a future Rust version help: add the missing argument(s) | LL | core::panic!("Hello {}", argument); @@ -39,13 +39,13 @@ help: or add a "{}" format string to use the message literally LL | core::panic!("{}", "Hello {}"); | ^^^^^ -warning: Panic message contains an unused formatting placeholder +warning: panic message contains an unused formatting placeholder --> $DIR/panic-brace.rs:8:20 | LL | assert!(false, "{:03x} bla"); | ^^^^^^^^^^^^ | - = note: This message is not used as a format string when given without arguments, but will be in a future Rust version + = note: this message is not used as a format string when given without arguments, but will be in a future Rust version help: add the missing argument(s) | LL | assert!(false, "{:03x} bla", argument); @@ -55,13 +55,13 @@ help: or add a "{}" format string to use the message literally LL | assert!(false, "{}", "{:03x} bla"); | ^^^^^ -warning: Panic message contains a brace +warning: panic message contains a brace --> $DIR/panic-brace.rs:9:5 | LL | debug_assert!(false, "{{}} bla"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: This message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust version help: add a "{}" format string to use the message literally | LL | debug_assert!(false, "{}", "{{}} bla"); From 6b44662669cf1680fe097e593eae20ca5dbed2ee Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Tue, 20 Oct 2020 22:25:42 +0200 Subject: [PATCH 21/28] Parse the format string for the panic_fmt lint for better warnings. --- Cargo.lock | 1 + compiler/rustc_lint/Cargo.toml | 1 + compiler/rustc_lint/src/panic_fmt.rs | 42 ++++++++++++++++++++++------ src/test/ui/panic-brace.rs | 2 +- src/test/ui/panic-brace.stderr | 22 +++++++-------- 5 files changed, 47 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 928d19b1e2c3f..4c84e7bd8c9f1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3831,6 +3831,7 @@ dependencies = [ "rustc_hir", "rustc_index", "rustc_middle", + "rustc_parse_format", "rustc_session", "rustc_span", "rustc_target", diff --git a/compiler/rustc_lint/Cargo.toml b/compiler/rustc_lint/Cargo.toml index 760a8e385d680..c56eb09b63471 100644 --- a/compiler/rustc_lint/Cargo.toml +++ b/compiler/rustc_lint/Cargo.toml @@ -20,3 +20,4 @@ rustc_feature = { path = "../rustc_feature" } rustc_index = { path = "../rustc_index" } rustc_session = { path = "../rustc_session" } rustc_trait_selection = { path = "../rustc_trait_selection" } +rustc_parse_format = { path = "../rustc_parse_format" } diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 288e1d61bbf49..cff50ff9912af 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -3,6 +3,7 @@ use rustc_ast as ast; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_middle::ty; +use rustc_parse_format::{ParseMode, Parser, Piece}; use rustc_span::sym; declare_lint! { @@ -52,13 +53,28 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc if cx.tcx.is_diagnostic_item(sym::std_panic_macro, id) || cx.tcx.is_diagnostic_item(sym::core_panic_macro, id) { - let s = sym.as_str(); - if !s.contains(&['{', '}'][..]) { + let fmt = sym.as_str(); + if !fmt.contains(&['{', '}'][..]) { return; } - let s = s.replace("{{", "").replace("}}", ""); - let looks_like_placeholder = - s.find('{').map_or(false, |i| s[i + 1..].contains('}')); + + let fmt_span = arg.span.source_callsite(); + + let (snippet, style) = + match cx.sess().parse_sess.source_map().span_to_snippet(fmt_span) { + Ok(snippet) => { + // Count the number of `#`s between the `r` and `"`. + let style = snippet.strip_prefix('r').and_then(|s| s.find('"')); + (Some(snippet), style) + } + Err(_) => (None, None), + }; + + let mut fmt_parser = + Parser::new(fmt.as_ref(), style, snippet, false, ParseMode::Format); + let n_arguments = + (&mut fmt_parser).filter(|a| matches!(a, Piece::NextArgument(_))).count(); + // Unwrap another level of macro expansion if this panic!() // was expanded from assert!() or debug_assert!(). for &assert in &[sym::assert_macro, sym::debug_assert_macro] { @@ -70,15 +86,23 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc expn = parent; } } - if looks_like_placeholder { - cx.struct_span_lint(PANIC_FMT, arg.span.source_callsite(), |lint| { - let mut l = lint.build("panic message contains an unused formatting placeholder"); + + if n_arguments > 0 && fmt_parser.errors.is_empty() { + let arg_spans: Vec<_> = match &fmt_parser.arg_places[..] { + [] => vec![fmt_span], + v => v.iter().map(|span| fmt_span.from_inner(*span)).collect(), + }; + cx.struct_span_lint(PANIC_FMT, arg_spans, |lint| { + let mut l = lint.build(match n_arguments { + 1 => "panic message contains an unused formatting placeholder", + _ => "panic message contains unused formatting placeholders", + }); l.note("this message is not used as a format string when given without arguments, but will be in a future Rust version"); if expn.call_site.contains(arg.span) { l.span_suggestion( arg.span.shrink_to_hi(), "add the missing argument(s)", - ", argument".into(), + ", ...".into(), Applicability::HasPlaceholders, ); l.span_suggestion( diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index 6ab5fafee887c..e74b6ad96c2c0 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -5,6 +5,6 @@ fn main() { panic!("here's a brace: {"); //~ WARN panic message contains a brace std::panic!("another one: }"); //~ WARN panic message contains a brace core::panic!("Hello {}"); //~ WARN panic message contains an unused formatting placeholder - assert!(false, "{:03x} bla"); //~ WARN panic message contains an unused formatting placeholder + assert!(false, "{:03x} {test} bla"); //~ WARN panic message contains unused formatting placeholders debug_assert!(false, "{{}} bla"); //~ WARN panic message contains a brace } diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr index 00b005a59d8c6..23ae31d00ebd2 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -24,35 +24,35 @@ LL | std::panic!("{}", "another one: }"); | ^^^^^ warning: panic message contains an unused formatting placeholder - --> $DIR/panic-brace.rs:7:18 + --> $DIR/panic-brace.rs:7:25 | LL | core::panic!("Hello {}"); - | ^^^^^^^^^^ + | ^^ | = note: this message is not used as a format string when given without arguments, but will be in a future Rust version help: add the missing argument(s) | -LL | core::panic!("Hello {}", argument); - | ^^^^^^^^^^ +LL | core::panic!("Hello {}", ...); + | ^^^^^ help: or add a "{}" format string to use the message literally | LL | core::panic!("{}", "Hello {}"); | ^^^^^ -warning: panic message contains an unused formatting placeholder - --> $DIR/panic-brace.rs:8:20 +warning: panic message contains unused formatting placeholders + --> $DIR/panic-brace.rs:8:21 | -LL | assert!(false, "{:03x} bla"); - | ^^^^^^^^^^^^ +LL | assert!(false, "{:03x} {test} bla"); + | ^^^^^^ ^^^^^^ | = note: this message is not used as a format string when given without arguments, but will be in a future Rust version help: add the missing argument(s) | -LL | assert!(false, "{:03x} bla", argument); - | ^^^^^^^^^^ +LL | assert!(false, "{:03x} {test} bla", ...); + | ^^^^^ help: or add a "{}" format string to use the message literally | -LL | assert!(false, "{}", "{:03x} bla"); +LL | assert!(false, "{}", "{:03x} {test} bla"); | ^^^^^ warning: panic message contains a brace From 190c3ad64ee9b73f0e616b0ec7ec7e63c831a218 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Tue, 20 Oct 2020 22:59:53 +0200 Subject: [PATCH 22/28] Improve panic_fmt error messages for invalid format strings too. --- compiler/rustc_lint/src/panic_fmt.rs | 22 ++++++++++--- src/test/ui/panic-brace.rs | 12 +++++-- src/test/ui/panic-brace.stderr | 48 ++++++++++++++++++++++------ 3 files changed, 66 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index cff50ff9912af..7428a9d13ff79 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -4,7 +4,7 @@ use rustc_errors::Applicability; use rustc_hir as hir; use rustc_middle::ty; use rustc_parse_format::{ParseMode, Parser, Piece}; -use rustc_span::sym; +use rustc_span::{sym, InnerSpan}; declare_lint! { /// The `panic_fmt` lint detects `panic!("..")` with `{` or `}` in the string literal. @@ -71,7 +71,7 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc }; let mut fmt_parser = - Parser::new(fmt.as_ref(), style, snippet, false, ParseMode::Format); + Parser::new(fmt.as_ref(), style, snippet.clone(), false, ParseMode::Format); let n_arguments = (&mut fmt_parser).filter(|a| matches!(a, Piece::NextArgument(_))).count(); @@ -115,8 +115,22 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc l.emit(); }); } else { - cx.struct_span_lint(PANIC_FMT, expn.call_site, |lint| { - let mut l = lint.build("panic message contains a brace"); + let brace_spans: Option> = snippet + .filter(|s| s.starts_with('"') || s.starts_with("r#")) + .map(|s| { + s.char_indices() + .filter(|&(_, c)| c == '{' || c == '}') + .map(|(i, _)| { + fmt_span.from_inner(InnerSpan { start: i, end: i + 1 }) + }) + .collect() + }); + let msg = match &brace_spans { + Some(v) if v.len() == 1 => "panic message contains a brace", + _ => "panic message contains braces", + }; + cx.struct_span_lint(PANIC_FMT, brace_spans.unwrap_or(vec![expn.call_site]), |lint| { + let mut l = lint.build(msg); l.note("this message is not used as a format string, but will be in a future Rust version"); if expn.call_site.contains(arg.span) { l.span_suggestion( diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index e74b6ad96c2c0..d38d8ac4deb32 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -1,10 +1,18 @@ // build-pass (FIXME(62277): should be check-pass) +const C: &str = "abc {}"; +static S: &str = "{bla}"; + #[allow(unreachable_code)] fn main() { panic!("here's a brace: {"); //~ WARN panic message contains a brace std::panic!("another one: }"); //~ WARN panic message contains a brace core::panic!("Hello {}"); //~ WARN panic message contains an unused formatting placeholder - assert!(false, "{:03x} {test} bla"); //~ WARN panic message contains unused formatting placeholders - debug_assert!(false, "{{}} bla"); //~ WARN panic message contains a brace + assert!(false, "{:03x} {test} bla"); + //~^ WARN panic message contains unused formatting placeholders + debug_assert!(false, "{{}} bla"); //~ WARN panic message contains braces + panic!(C); // No warning (yet) + panic!(S); // No warning (yet) + panic!(concat!("{", "}")); //~ WARN panic message contains an unused formatting placeholder + panic!(concat!("{", "{")); //~ WARN panic message contains braces } diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr index 23ae31d00ebd2..16795ed3d36e8 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -1,8 +1,8 @@ warning: panic message contains a brace - --> $DIR/panic-brace.rs:5:5 + --> $DIR/panic-brace.rs:8:29 | LL | panic!("here's a brace: {"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ | = note: `#[warn(panic_fmt)]` on by default = note: this message is not used as a format string, but will be in a future Rust version @@ -12,10 +12,10 @@ LL | panic!("{}", "here's a brace: {"); | ^^^^^ warning: panic message contains a brace - --> $DIR/panic-brace.rs:6:5 + --> $DIR/panic-brace.rs:9:31 | LL | std::panic!("another one: }"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ | = note: this message is not used as a format string, but will be in a future Rust version help: add a "{}" format string to use the message literally @@ -24,7 +24,7 @@ LL | std::panic!("{}", "another one: }"); | ^^^^^ warning: panic message contains an unused formatting placeholder - --> $DIR/panic-brace.rs:7:25 + --> $DIR/panic-brace.rs:10:25 | LL | core::panic!("Hello {}"); | ^^ @@ -40,7 +40,7 @@ LL | core::panic!("{}", "Hello {}"); | ^^^^^ warning: panic message contains unused formatting placeholders - --> $DIR/panic-brace.rs:8:21 + --> $DIR/panic-brace.rs:11:21 | LL | assert!(false, "{:03x} {test} bla"); | ^^^^^^ ^^^^^^ @@ -55,11 +55,11 @@ help: or add a "{}" format string to use the message literally LL | assert!(false, "{}", "{:03x} {test} bla"); | ^^^^^ -warning: panic message contains a brace - --> $DIR/panic-brace.rs:9:5 +warning: panic message contains braces + --> $DIR/panic-brace.rs:13:27 | LL | debug_assert!(false, "{{}} bla"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^ | = note: this message is not used as a format string, but will be in a future Rust version help: add a "{}" format string to use the message literally @@ -67,5 +67,33 @@ help: add a "{}" format string to use the message literally LL | debug_assert!(false, "{}", "{{}} bla"); | ^^^^^ -warning: 5 warnings emitted +warning: panic message contains an unused formatting placeholder + --> $DIR/panic-brace.rs:16:12 + | +LL | panic!(concat!("{", "}")); + | ^^^^^^^^^^^^^^^^^ + | + = note: this message is not used as a format string when given without arguments, but will be in a future Rust version +help: add the missing argument(s) + | +LL | panic!(concat!("{", "}"), ...); + | ^^^^^ +help: or add a "{}" format string to use the message literally + | +LL | panic!("{}", concat!("{", "}")); + | ^^^^^ + +warning: panic message contains braces + --> $DIR/panic-brace.rs:17:5 + | +LL | panic!(concat!("{", "{")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this message is not used as a format string, but will be in a future Rust version +help: add a "{}" format string to use the message literally + | +LL | panic!("{}", concat!("{", "{")); + | ^^^^^ + +warning: 7 warnings emitted From 1993f1efe717680479242e711848a4d38f35cf1b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 24 Oct 2020 21:13:04 +0200 Subject: [PATCH 23/28] Test that panic_fmt lint doesn't trigger for custom panic macro. --- src/test/ui/panic-brace.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index d38d8ac4deb32..e3ea7a8bf0f75 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -15,4 +15,11 @@ fn main() { panic!(S); // No warning (yet) panic!(concat!("{", "}")); //~ WARN panic message contains an unused formatting placeholder panic!(concat!("{", "{")); //~ WARN panic message contains braces + + // Check that the lint only triggers for std::panic and core::panic, + // not any panic macro: + macro_rules! panic { + ($e:expr) => (); + } + panic!("{}"); // OK } From 5cefc3ce416dc435e2356dfa0e8f3cbe086a7619 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 28 Oct 2020 11:00:28 +0100 Subject: [PATCH 24/28] Mark panic_fmt suggestion as machine applicable. Co-authored-by: bjorn3 --- compiler/rustc_lint/src/panic_fmt.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index 7428a9d13ff79..a08fae1f34e22 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -109,7 +109,7 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc arg.span.shrink_to_lo(), "or add a \"{}\" format string to use the message literally", "\"{}\", ".into(), - Applicability::MaybeIncorrect, + Applicability::MachineApplicable, ); } l.emit(); From 9743f676843cd8e25292a673d8c1c3296547ecfc Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 29 Oct 2020 19:44:06 +0100 Subject: [PATCH 25/28] Improve panic_fmt lint messages. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (From the PR feedback.) Co-authored-by: Esteban Küber --- compiler/rustc_lint/src/panic_fmt.rs | 10 +++++----- src/test/ui/panic-brace.stderr | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_lint/src/panic_fmt.rs b/compiler/rustc_lint/src/panic_fmt.rs index a08fae1f34e22..0d2b20989b0c3 100644 --- a/compiler/rustc_lint/src/panic_fmt.rs +++ b/compiler/rustc_lint/src/panic_fmt.rs @@ -1,6 +1,6 @@ use crate::{LateContext, LateLintPass, LintContext}; use rustc_ast as ast; -use rustc_errors::Applicability; +use rustc_errors::{pluralize, Applicability}; use rustc_hir as hir; use rustc_middle::ty; use rustc_parse_format::{ParseMode, Parser, Piece}; @@ -21,7 +21,7 @@ declare_lint! { /// /// `panic!("{}")` panics with the message `"{}"`, as a `panic!()` invocation /// with a single argument does not use `format_args!()`. - /// A future version of Rust will interpret this string as format string, + /// A future edition of Rust will interpret this string as format string, /// which would break this. PANIC_FMT, Warn, @@ -97,11 +97,11 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc 1 => "panic message contains an unused formatting placeholder", _ => "panic message contains unused formatting placeholders", }); - l.note("this message is not used as a format string when given without arguments, but will be in a future Rust version"); + l.note("this message is not used as a format string when given without arguments, but will be in a future Rust edition"); if expn.call_site.contains(arg.span) { l.span_suggestion( arg.span.shrink_to_hi(), - "add the missing argument(s)", + &format!("add the missing argument{}", pluralize!(n_arguments)), ", ...".into(), Applicability::HasPlaceholders, ); @@ -131,7 +131,7 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc }; cx.struct_span_lint(PANIC_FMT, brace_spans.unwrap_or(vec![expn.call_site]), |lint| { let mut l = lint.build(msg); - l.note("this message is not used as a format string, but will be in a future Rust version"); + l.note("this message is not used as a format string, but will be in a future Rust edition"); if expn.call_site.contains(arg.span) { l.span_suggestion( arg.span.shrink_to_lo(), diff --git a/src/test/ui/panic-brace.stderr b/src/test/ui/panic-brace.stderr index 16795ed3d36e8..0520ab2a38f31 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -5,7 +5,7 @@ LL | panic!("here's a brace: {"); | ^ | = note: `#[warn(panic_fmt)]` on by default - = note: this message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust edition help: add a "{}" format string to use the message literally | LL | panic!("{}", "here's a brace: {"); @@ -17,7 +17,7 @@ warning: panic message contains a brace LL | std::panic!("another one: }"); | ^ | - = note: this message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust edition help: add a "{}" format string to use the message literally | LL | std::panic!("{}", "another one: }"); @@ -29,8 +29,8 @@ warning: panic message contains an unused formatting placeholder LL | core::panic!("Hello {}"); | ^^ | - = note: this message is not used as a format string when given without arguments, but will be in a future Rust version -help: add the missing argument(s) + = note: this message is not used as a format string when given without arguments, but will be in a future Rust edition +help: add the missing argument | LL | core::panic!("Hello {}", ...); | ^^^^^ @@ -45,8 +45,8 @@ warning: panic message contains unused formatting placeholders LL | assert!(false, "{:03x} {test} bla"); | ^^^^^^ ^^^^^^ | - = note: this message is not used as a format string when given without arguments, but will be in a future Rust version -help: add the missing argument(s) + = note: this message is not used as a format string when given without arguments, but will be in a future Rust edition +help: add the missing arguments | LL | assert!(false, "{:03x} {test} bla", ...); | ^^^^^ @@ -61,7 +61,7 @@ warning: panic message contains braces LL | debug_assert!(false, "{{}} bla"); | ^^^^ | - = note: this message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust edition help: add a "{}" format string to use the message literally | LL | debug_assert!(false, "{}", "{{}} bla"); @@ -73,8 +73,8 @@ warning: panic message contains an unused formatting placeholder LL | panic!(concat!("{", "}")); | ^^^^^^^^^^^^^^^^^ | - = note: this message is not used as a format string when given without arguments, but will be in a future Rust version -help: add the missing argument(s) + = note: this message is not used as a format string when given without arguments, but will be in a future Rust edition +help: add the missing argument | LL | panic!(concat!("{", "}"), ...); | ^^^^^ @@ -89,7 +89,7 @@ warning: panic message contains braces LL | panic!(concat!("{", "{")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: this message is not used as a format string, but will be in a future Rust version + = note: this message is not used as a format string, but will be in a future Rust edition help: add a "{}" format string to use the message literally | LL | panic!("{}", concat!("{", "{")); From a922c6b410e65065942771cf866cfac6ffd65437 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 29 Oct 2020 22:21:40 +0100 Subject: [PATCH 26/28] Add test for panic_fmt lint with external panic!()-calling macro. --- src/test/ui/auxiliary/fancy-panic.rs | 6 ++++++ src/test/ui/panic-brace.rs | 6 ++++++ src/test/ui/panic-brace.stderr | 24 ++++++++++++++++-------- 3 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 src/test/ui/auxiliary/fancy-panic.rs diff --git a/src/test/ui/auxiliary/fancy-panic.rs b/src/test/ui/auxiliary/fancy-panic.rs new file mode 100644 index 0000000000000..e5a25a171fbe0 --- /dev/null +++ b/src/test/ui/auxiliary/fancy-panic.rs @@ -0,0 +1,6 @@ +#[macro_export] +macro_rules! fancy_panic { + ($msg:expr) => { + panic!($msg) + }; +} diff --git a/src/test/ui/panic-brace.rs b/src/test/ui/panic-brace.rs index e3ea7a8bf0f75..754dcc287d0f9 100644 --- a/src/test/ui/panic-brace.rs +++ b/src/test/ui/panic-brace.rs @@ -1,4 +1,7 @@ // build-pass (FIXME(62277): should be check-pass) +// aux-build:fancy-panic.rs + +extern crate fancy_panic; const C: &str = "abc {}"; static S: &str = "{bla}"; @@ -16,6 +19,9 @@ fn main() { panic!(concat!("{", "}")); //~ WARN panic message contains an unused formatting placeholder panic!(concat!("{", "{")); //~ WARN panic message contains braces + fancy_panic::fancy_panic!("test {} 123"); + //~^ WARN panic message contains an unused formatting placeholder + // 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/panic-brace.stderr b/src/test/ui/panic-brace.stderr index 0520ab2a38f31..93808891c3c37 100644 --- a/src/test/ui/panic-brace.stderr +++ b/src/test/ui/panic-brace.stderr @@ -1,5 +1,5 @@ warning: panic message contains a brace - --> $DIR/panic-brace.rs:8:29 + --> $DIR/panic-brace.rs:11:29 | LL | panic!("here's a brace: {"); | ^ @@ -12,7 +12,7 @@ LL | panic!("{}", "here's a brace: {"); | ^^^^^ warning: panic message contains a brace - --> $DIR/panic-brace.rs:9:31 + --> $DIR/panic-brace.rs:12:31 | LL | std::panic!("another one: }"); | ^ @@ -24,7 +24,7 @@ LL | std::panic!("{}", "another one: }"); | ^^^^^ warning: panic message contains an unused formatting placeholder - --> $DIR/panic-brace.rs:10:25 + --> $DIR/panic-brace.rs:13:25 | LL | core::panic!("Hello {}"); | ^^ @@ -40,7 +40,7 @@ LL | core::panic!("{}", "Hello {}"); | ^^^^^ warning: panic message contains unused formatting placeholders - --> $DIR/panic-brace.rs:11:21 + --> $DIR/panic-brace.rs:14:21 | LL | assert!(false, "{:03x} {test} bla"); | ^^^^^^ ^^^^^^ @@ -56,7 +56,7 @@ LL | assert!(false, "{}", "{:03x} {test} bla"); | ^^^^^ warning: panic message contains braces - --> $DIR/panic-brace.rs:13:27 + --> $DIR/panic-brace.rs:16:27 | LL | debug_assert!(false, "{{}} bla"); | ^^^^ @@ -68,7 +68,7 @@ LL | debug_assert!(false, "{}", "{{}} bla"); | ^^^^^ warning: panic message contains an unused formatting placeholder - --> $DIR/panic-brace.rs:16:12 + --> $DIR/panic-brace.rs:19:12 | LL | panic!(concat!("{", "}")); | ^^^^^^^^^^^^^^^^^ @@ -84,7 +84,7 @@ LL | panic!("{}", concat!("{", "}")); | ^^^^^ warning: panic message contains braces - --> $DIR/panic-brace.rs:17:5 + --> $DIR/panic-brace.rs:20:5 | LL | panic!(concat!("{", "{")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -95,5 +95,13 @@ help: add a "{}" format string to use the message literally LL | panic!("{}", concat!("{", "{")); | ^^^^^ -warning: 7 warnings emitted +warning: panic message contains an unused formatting placeholder + --> $DIR/panic-brace.rs:22:37 + | +LL | fancy_panic::fancy_panic!("test {} 123"); + | ^^ + | + = note: this message is not used as a format string when given without arguments, but will be in a future Rust edition + +warning: 8 warnings emitted From 454eaec1dc0875614a59ce6b074e91eccb5ab96a Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 19 Nov 2020 18:13:32 +0100 Subject: [PATCH 27/28] Remove the clippy::panic-params lint. Rustc itself now warns for all cases that triggered this lint. --- src/tools/clippy/clippy_lints/src/lib.rs | 3 - .../clippy_lints/src/panic_unimplemented.rs | 46 ++------------ src/tools/clippy/tests/ui/panic.rs | 61 ------------------- src/tools/clippy/tests/ui/panic.stderr | 28 --------- 4 files changed, 4 insertions(+), 134 deletions(-) delete mode 100644 src/tools/clippy/tests/ui/panic.rs delete mode 100644 src/tools/clippy/tests/ui/panic.stderr diff --git a/src/tools/clippy/clippy_lints/src/lib.rs b/src/tools/clippy/clippy_lints/src/lib.rs index 126852df502eb..19bf67d80c428 100644 --- a/src/tools/clippy/clippy_lints/src/lib.rs +++ b/src/tools/clippy/clippy_lints/src/lib.rs @@ -788,7 +788,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, &panic_in_result_fn::PANIC_IN_RESULT_FN, &panic_unimplemented::PANIC, - &panic_unimplemented::PANIC_PARAMS, &panic_unimplemented::TODO, &panic_unimplemented::UNIMPLEMENTED, &panic_unimplemented::UNREACHABLE, @@ -1499,7 +1498,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP), LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), - LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL), LintId::of(&precedence::PRECEDENCE), LintId::of(&ptr::CMP_NULL), @@ -1666,7 +1664,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST), LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), - LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&ptr::CMP_NULL), LintId::of(&ptr::PTR_ARG), LintId::of(&ptr_eq::PTR_EQ), diff --git a/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs b/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs index 3d888fe732573..8b10d07164712 100644 --- a/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs +++ b/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs @@ -1,30 +1,10 @@ -use crate::utils::{is_direct_expn_of, is_expn_of, match_panic_call, span_lint}; +use crate::utils::{is_expn_of, match_panic_call, span_lint}; use if_chain::if_chain; -use rustc_ast::ast::LitKind; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::Expr; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; -declare_clippy_lint! { - /// **What it does:** Checks for missing parameters in `panic!`. - /// - /// **Why is this bad?** Contrary to the `format!` family of macros, there are - /// two forms of `panic!`: if there are no parameters given, the first argument - /// is not a format string and used literally. So while `format!("{}")` will - /// fail to compile, `panic!("{}")` will not. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```no_run - /// panic!("This `panic!` is probably missing a parameter there: {}"); - /// ``` - pub PANIC_PARAMS, - style, - "missing parameters in `panic!` calls" -} - declare_clippy_lint! { /// **What it does:** Checks for usage of `panic!`. /// @@ -89,11 +69,11 @@ declare_clippy_lint! { "`unreachable!` should not be present in production code" } -declare_lint_pass!(PanicUnimplemented => [PANIC_PARAMS, UNIMPLEMENTED, UNREACHABLE, TODO, PANIC]); +declare_lint_pass!(PanicUnimplemented => [UNIMPLEMENTED, UNREACHABLE, TODO, PANIC]); impl<'tcx> LateLintPass<'tcx> for PanicUnimplemented { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(params) = match_panic_call(cx, expr) { + if let Some(_) = match_panic_call(cx, expr) { let span = get_outer_span(expr); if is_expn_of(expr.span, "unimplemented").is_some() { span_lint( @@ -113,7 +93,6 @@ impl<'tcx> LateLintPass<'tcx> for PanicUnimplemented { ); } else if is_expn_of(expr.span, "panic").is_some() { span_lint(cx, PANIC, span, "`panic` should not be present in production code"); - match_panic(params, expr, cx); } } } @@ -132,20 +111,3 @@ fn get_outer_span(expr: &Expr<'_>) -> Span { } } } - -fn match_panic(params: &[Expr<'_>], expr: &Expr<'_>, cx: &LateContext<'_>) { - if_chain! { - if let ExprKind::Lit(ref lit) = params[0].kind; - if is_direct_expn_of(expr.span, "panic").is_some(); - if let LitKind::Str(ref string, _) = lit.node; - let string = string.as_str().replace("{{", "").replace("}}", ""); - if let Some(par) = string.find('{'); - if string[par..].contains('}'); - if params[0].span.source_callee().is_none(); - if params[0].span.lo() != params[0].span.hi(); - then { - span_lint(cx, PANIC_PARAMS, params[0].span, - "you probably are missing some parameter in your format string"); - } - } -} diff --git a/src/tools/clippy/tests/ui/panic.rs b/src/tools/clippy/tests/ui/panic.rs deleted file mode 100644 index 6e004aa9a924f..0000000000000 --- a/src/tools/clippy/tests/ui/panic.rs +++ /dev/null @@ -1,61 +0,0 @@ -#![warn(clippy::panic_params)] -#![allow(clippy::assertions_on_constants)] -fn missing() { - if true { - panic!("{}"); - } else if false { - panic!("{:?}"); - } else { - assert!(true, "here be missing values: {}"); - } - - panic!("{{{this}}}"); -} - -fn ok_single() { - panic!("foo bar"); -} - -fn ok_inner() { - // Test for #768 - assert!("foo bar".contains(&format!("foo {}", "bar"))); -} - -fn ok_multiple() { - panic!("{}", "This is {ok}"); -} - -fn ok_bracket() { - match 42 { - 1337 => panic!("{so is this"), - 666 => panic!("so is this}"), - _ => panic!("}so is that{"), - } -} - -const ONE: u32 = 1; - -fn ok_nomsg() { - assert!({ 1 == ONE }); - assert!(if 1 == ONE { ONE == 1 } else { false }); -} - -fn ok_escaped() { - panic!("{{ why should this not be ok? }}"); - panic!(" or {{ that ?"); - panic!(" or }} this ?"); - panic!(" {or {{ that ?"); - panic!(" }or }} this ?"); - panic!("{{ test }"); - panic!("{case }}"); -} - -fn main() { - missing(); - ok_single(); - ok_multiple(); - ok_bracket(); - ok_inner(); - ok_nomsg(); - ok_escaped(); -} diff --git a/src/tools/clippy/tests/ui/panic.stderr b/src/tools/clippy/tests/ui/panic.stderr deleted file mode 100644 index 1f8ff8ccf5575..0000000000000 --- a/src/tools/clippy/tests/ui/panic.stderr +++ /dev/null @@ -1,28 +0,0 @@ -error: you probably are missing some parameter in your format string - --> $DIR/panic.rs:5:16 - | -LL | panic!("{}"); - | ^^^^ - | - = note: `-D clippy::panic-params` implied by `-D warnings` - -error: you probably are missing some parameter in your format string - --> $DIR/panic.rs:7:16 - | -LL | panic!("{:?}"); - | ^^^^^^ - -error: you probably are missing some parameter in your format string - --> $DIR/panic.rs:9:23 - | -LL | assert!(true, "here be missing values: {}"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: you probably are missing some parameter in your format string - --> $DIR/panic.rs:12:12 - | -LL | panic!("{{{this}}}"); - | ^^^^^^^^^^^^ - -error: aborting due to 4 previous errors - From a125ef2e8ec27e8fedc119ddfdef638d09a69ba2 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 19 Nov 2020 19:47:25 +0100 Subject: [PATCH 28/28] Clippy: Match on assert!() expansions without an inner block. --- .../clippy/clippy_lints/src/assertions_on_constants.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs b/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs index a2ccb0369c4a4..a52f0997d439d 100644 --- a/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs +++ b/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs @@ -129,8 +129,11 @@ fn match_assert_with_message<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) if let ExprKind::Block(ref block, _) = arms[0].body.kind; if block.stmts.is_empty(); if let Some(block_expr) = &block.expr; - if let ExprKind::Block(ref inner_block, _) = block_expr.kind; - if let Some(begin_panic_call) = &inner_block.expr; + // inner block is optional. unwarp it if it exists, or use the expression as is otherwise. + if let Some(begin_panic_call) = match block_expr.kind { + ExprKind::Block(ref inner_block, _) => &inner_block.expr, + _ => &block.expr, + }; // function call if let Some(args) = match_panic_call(cx, begin_panic_call); if args.len() == 1;