Auto merge of rust-lang#78088 - fusion-engineering-forks:panic-fmt-lint, r=estebank

Add lint for panic!("{}")

This adds a lint that warns about `panic!("{}")`.

`panic!(msg)` invocations with a single argument use their argument as panic payload literally, without using it as a format string. The same holds for `assert!(expr, msg)`.

This lints checks if `msg` is a string literal (after expansion), and warns in case it contained braces. It suggests to insert `"{}", ` to use the message literally, or to add arguments to use it as a format string.

![image](https://user-images.githubusercontent.com/783247/96643867-79eb1080-1328-11eb-8d4e-a5586837c70a.png)

This lint is also a good starting point for adding warnings about `panic!(not_a_string)` later, once [`panic_any()`](rust-lang#74622) becomes a stable alternative.

Author: bors

Cargo.lock

@@ -3831,6 +3831,7 @@ dependencies = [
  "rustc_hir",
  "rustc_index",
  "rustc_middle",
+ "rustc_parse_format",
  "rustc_session",
  "rustc_span",
  "rustc_target",

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,39 @@ 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,
+
+    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()
                 )),
-                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 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)
 }
 

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"
         ),
     }

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" }

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,
             ]
         );
     };

compiler/rustc_lint/src/panic_fmt.rs

@@ -0,0 +1,150 @@
+use crate::{LateContext, LateLintPass, LintContext};
+use rustc_ast as ast;
+use rustc_errors::{pluralize, Applicability};
+use rustc_hir as hir;
+use rustc_middle::ty;
+use rustc_parse_format::{ParseMode, Parser, Piece};
+use rustc_span::{sym, InnerSpan};
+
+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 edition 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 {
+            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)
+                {
+                    let fmt = sym.as_str();
+                    if !fmt.contains(&['{', '}'][..]) {
+                        return;
+                    }
+
+                    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.clone(), 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] {
+                        let parent = expn.call_site.ctxt().outer_expn_data();
+                        if parent
+                            .macro_def_id
+                            .map_or(false, |id| cx.tcx.is_diagnostic_item(assert, id))
+                        {
+                            expn = parent;
+                        }
+                    }
+
+                    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 edition");
+                            if expn.call_site.contains(arg.span) {
+                                l.span_suggestion(
+                                    arg.span.shrink_to_hi(),
+                                    &format!("add the missing argument{}", pluralize!(n_arguments)),
+                                    ", ...".into(),
+                                    Applicability::HasPlaceholders,
+                                );
+                                l.span_suggestion(
+                                    arg.span.shrink_to_lo(),
+                                    "or add a \"{}\" format string to use the message literally",
+                                    "\"{}\", ".into(),
+                                    Applicability::MachineApplicable,
+                                );
+                            }
+                            l.emit();
+                        });
+                    } else {
+                        let brace_spans: Option<Vec<_>> = 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 edition");
+                            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();
+                        });
+                    }
+                }
+            }
+        }
+    }
+}

compiler/rustc_passes/src/diagnostic_items.rs

@@ -113,6 +113,10 @@ fn collect<'tcx>(tcx: TyCtxt<'tcx>) -> FxHashMap<Symbol, DefId> {
         }
     }
 
+    for m in tcx.hir().krate().exported_macros {
+        collector.observe_item(m.attrs, m.hir_id);
+    }
+
     collector.items
 }
 

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,
@@ -393,6 +394,7 @@ symbols! {
         copysignf64,
         core,
         core_intrinsics,
+        core_panic_macro,
         cosf32,
         cosf64,
         crate_id,
@@ -416,6 +418,7 @@ symbols! {
         dead_code,
         dealloc,
         debug,
+        debug_assert_macro,
         debug_assertions,
         debug_struct,
         debug_trait,
@@ -789,6 +792,7 @@ symbols! {
         panic_runtime,
         panic_str,
         panic_unwind,
+        panicking,
         param_attrs,
         parent_trait,
         partial_cmp,
@@ -1064,6 +1068,7 @@ symbols! {
         staticlib,
         std,
         std_inject,
+        std_panic_macro,
         stmt,
         stmt_expr_attributes,
         stop_after_dataflow,

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")]
+#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "core_panic_macro")]
 macro_rules! panic {
     () => (
         $crate::panic!("explicit panic")
@@ -162,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)*); })
 }
@@ -1215,6 +1217,8 @@ pub(crate) mod builtin {
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_builtin_macro]
     #[macro_export]
+    #[cfg_attr(not(bootstrap), 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 */ }};

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 { ... })"),
     }
 }