Restore write_literal suggestions, lint FormatSpec args

Author: Alexendoo

clippy_lints/src/format.rs

@@ -76,7 +76,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
                     ty::Str => true,
                     _ => false,
                 };
-                if let Some(args) = format_args.args();
+                if let Some(args) = format_args.args(cx);
                 if args.iter().all(|arg| arg.format_trait == sym::Display && !arg.has_string_formatting());
                 then {
                     let is_new_string = match value.kind {

clippy_lints/src/format_args.rs

@@ -74,7 +74,7 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
             if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
             if is_format_macro(cx, macro_def_id);
             if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
-            if let Some(args) = format_args.args();
+            if let Some(args) = format_args.args(cx);
             then {
                 for (i, arg) in args.iter().enumerate() {
                     if arg.format_trait != sym::Display {

clippy_lints/src/format_impl.rs

@@ -168,7 +168,7 @@ fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>,
         if let macro_def_id = outer_macro.def_id;
         if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
         if is_format_macro(cx, macro_def_id);
-        if let Some(args) = format_args.args();
+        if let Some(args) = format_args.args(cx);
         then {
             for arg in args {
                 if arg.format_trait != impl_trait.name {

clippy_lints/src/write.rs

@@ -3,7 +3,7 @@ use clippy_utils::macros::{root_macro_call_first_node, FormatArgsExpn, MacroCall
 use clippy_utils::source::snippet_opt;
 use rustc_ast::LitKind;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, Impl, Item, ItemKind};
+use rustc_hir::{Expr, ExprKind, HirIdMap, Impl, Item, ItemKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{sym, BytePos, Span};
@@ -128,10 +128,6 @@ declare_clippy_lint! {
     /// (c.f., https://github.com/matthiaskrgr/rust-str-bench) and unnecessary
     /// (i.e., just put the literal in the format string)
     ///
-    /// ### Known problems
-    /// Format strings with format specifiers in are not linted, e.g.
-    /// `print!("{}, {:#?}", "foo", bar)`
-    ///
     /// ### Example
     /// ```rust
     /// println!("{}", "foo");
@@ -206,10 +202,6 @@ declare_clippy_lint! {
     /// (c.f., https://github.com/matthiaskrgr/rust-str-bench) and unnecessary
     /// (i.e., just put the literal in the format string)
     ///
-    /// ### Known problems
-    /// Format strings with format specifiers in are not linted, e.g.
-    /// `print!("{}, {:#?}", "foo", bar)`
-    ///
     /// ### Example
     /// ```rust
     /// # use std::fmt::Write;
@@ -440,38 +432,76 @@ fn check_empty_string(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, ma
     }
 }
 
-// Expand from `writeln!(o, "")` to `writeln!(o, "")`
-//                          ^^                 ^^^^
-fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
-    let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
-    extended.with_lo(extended.lo() - BytePos(1))
-}
-
-fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, name: &str) {
-    if !format_args.specs.is_empty() {
+fn check_literal<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsExpn<'tcx>, name: &str) {
+    if format_args.format_string_span.from_expansion() {
         return;
     }
+    let raw = format_args.is_raw(cx);
+
+    let Some(args) = format_args.args(cx) else { return };
+    let mut counts = HirIdMap::<usize>::default();
+    for arg in &args {
+        *counts.entry(arg.value.hir_id).or_default() += 1;
+    }
 
-    for &(idx, formatter) in &format_args.formatters {
+    for arg in args {
         if_chain! {
-            if formatter == sym::Display;
-            let expr = format_args.value_args[idx];
-            if !expr.span.from_expansion();
-            if !format_args.format_string_span.from_expansion();
-            if let ExprKind::Lit(lit) = &expr.kind;
-            if let LitKind::Str(..)
-                | LitKind::Byte(_)
-                | LitKind::Char(_)
-                | LitKind::Bool(_) = lit.node;
+            if counts[&arg.value.hir_id] == 1;
+            if arg.format_trait == sym::Display;
+            if !arg.has_primitive_formatting();
+            if !arg.value.span.from_expansion();
+            if let ExprKind::Lit(lit) = &arg.value.kind;
             then {
+                let replacement = match lit.node {
+                    LitKind::Str(s, _) => s.to_string(),
+                    LitKind::Char(c) => c.to_string(),
+                    LitKind::Bool(b) => b.to_string(),
+                    _ => continue,
+                };
+
                 let lint = if name.starts_with("write") {
                     WRITE_LITERAL
                 } else {
                     PRINT_LITERAL
                 };
 
-                span_lint(cx, lint, expr.span, "literal with an empty format string")
+                span_lint_and_then(cx, lint, arg.value.span, "literal with an empty format string", |diag| {
+                    if raw && replacement.contains(&['"', '#']) {
+                        return;
+                    }
+
+                    let backslash = if raw {
+                        r"\"
+                    } else {
+                        r"\\"
+                    };
+                    let replacement = replacement
+                        .replace('{', "{{")
+                        .replace('}', "}}")
+                        .replace('"', "\\\"")
+                        .replace('\\', backslash);
+
+                    // `format!("{}", "a")`, `format!("{named}", named = "b")
+                    //              ~~~~~                      ~~~~~~~~~~~~~
+                    let value_span = expand_past_previous_comma(cx, arg.value.span);
+
+                    diag.multipart_suggestion(
+                        "try this",
+                        vec![
+                            (arg.span, replacement),
+                            (value_span, String::new()),
+                        ],
+                        Applicability::MachineApplicable,
+                    );
+                });
             }
         }
     }
 }
+
+// Expand from `writeln!(o, "")` to `writeln!(o, "")`
+//                          ^^                 ^^^^
+fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
+    let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
+    extended.with_lo(extended.lo() - BytePos(1))
+}

clippy_utils/src/macros.rs

@@ -1,16 +1,19 @@
 #![allow(clippy::similar_names)] // `expr` and `expn`
 
+use crate::source::snippet_opt;
 use crate::visitors::expr_visitor_no_bodies;
 
 use arrayvec::ArrayVec;
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
 use rustc_hir::intravisit::Visitor;
-use rustc_hir::{self as hir, Expr, ExprKind, HirId, Node, QPath};
+use rustc_hir::{self as hir, Expr, ExprField, ExprKind, HirId, Node, QPath};
 use rustc_lint::LateContext;
 use rustc_span::def_id::DefId;
 use rustc_span::hygiene::{self, MacroKind, SyntaxContext};
-use rustc_span::{sym, ExpnData, ExpnId, ExpnKind, Span, Symbol};
+use rustc_span::source_map::Spanned;
+use rustc_span::{sym, BytePos, ExpnData, ExpnId, ExpnKind, Pos, Span, Symbol};
+use std::iter;
 use std::ops::ControlFlow;
 
 const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[
@@ -66,7 +69,7 @@ impl MacroCall {
 
 /// Returns an iterator of expansions that created the given span
 pub fn expn_backtrace(mut span: Span) -> impl Iterator<Item = (ExpnId, ExpnData)> {
-    std::iter::from_fn(move || {
+    iter::from_fn(move || {
         let ctxt = span.ctxt();
         if ctxt == SyntaxContext::root() {
             return None;
@@ -90,7 +93,7 @@ pub fn expn_is_local(expn: ExpnId) -> bool {
     }
     let data = expn.expn_data();
     let backtrace = expn_backtrace(data.call_site);
-    std::iter::once((expn, data))
+    iter::once((expn, data))
         .chain(backtrace)
         .find_map(|(_, data)| data.macro_def_id)
         .map_or(true, DefId::is_local)
@@ -467,20 +470,24 @@ impl<'tcx> FormatArgsExpn<'tcx> {
     }
 
     /// Returns a vector of `FormatArgsArg`.
-    pub fn args(&self) -> Option<Vec<FormatArgsArg<'tcx>>> {
+    pub fn args(&self, cx: &LateContext<'tcx>) -> Option<Vec<FormatArgsArg<'tcx>>> {
+        let spans = self.format_argument_spans(cx)?;
+
         if self.specs.is_empty() {
-            let args = std::iter::zip(&self.value_args, &self.formatters)
-                .map(|(value, &(_, format_trait))| FormatArgsArg {
+            let args = iter::zip(&self.value_args, &self.formatters)
+                .zip(spans)
+                .map(|((value, &(_, format_trait)), span)| FormatArgsArg {
                     value,
                     format_trait,
+                    span,
                     spec: None,
                 })
                 .collect();
             return Some(args);
         }
-        self.specs
-            .iter()
-            .map(|spec| {
+
+        iter::zip(&self.specs, spans)
+            .map(|(spec, span)| {
                 if_chain! {
                     // struct `core::fmt::rt::v1::Argument`
                     if let ExprKind::Struct(_, fields, _) = spec.kind;
@@ -493,6 +500,7 @@ impl<'tcx> FormatArgsExpn<'tcx> {
                         Some(FormatArgsArg {
                             value: self.value_args[j],
                             format_trait,
+                            span,
                             spec: Some(spec),
                         })
                     } else {
@@ -503,6 +511,14 @@ impl<'tcx> FormatArgsExpn<'tcx> {
             .collect()
     }
 
+    pub fn is_raw(&self, cx: &LateContext<'tcx>) -> bool {
+        if let Some(format_string) = snippet_opt(cx, self.format_string_span) {
+            format_string.starts_with("r")
+        } else {
+            false
+        }
+    }
+
     /// Source callsite span of all inputs
     pub fn inputs_span(&self) -> Span {
         match *self.value_args {
@@ -512,6 +528,36 @@ impl<'tcx> FormatArgsExpn<'tcx> {
                 .to(hygiene::walk_chain(last.span, self.format_string_span.ctxt())),
         }
     }
+
+    /// Returns the spans covering the `{}`s in the format string
+    fn format_argument_spans(&self, cx: &LateContext<'tcx>) -> Option<Vec<Span>> {
+        let format_string = snippet_opt(cx, self.format_string_span)?.into_bytes();
+        let lo = self.format_string_span.lo();
+        let get = |i| format_string.get(i).copied();
+
+        let mut spans = Vec::new();
+        let mut i = 0;
+        let mut arg_start = 0;
+        loop {
+            match (get(i), get(i + 1)) {
+                (Some(b'{'), Some(b'{')) | (Some(b'}'), Some(b'}')) => {
+                    i += 1;
+                },
+                (Some(b'{'), _) => arg_start = i,
+                (Some(b'}'), _) => spans.push(
+                    self.format_string_span
+                        .with_lo(lo + BytePos::from_usize(arg_start))
+                        .with_hi(lo + BytePos::from_usize(i + 1)),
+                ),
+                (None, _) => break,
+                _ => {},
+            }
+
+            i += 1;
+        }
+
+        Some(spans)
+    }
 }
 
 /// Type representing a `FormatArgsExpn`'s format arguments
@@ -522,12 +568,36 @@ pub struct FormatArgsArg<'tcx> {
     pub format_trait: Symbol,
     /// An element of `specs`
     pub spec: Option<&'tcx Expr<'tcx>>,
+    /// Span of the `{}`
+    pub span: Span,
 }
 
 impl<'tcx> FormatArgsArg<'tcx> {
     /// Returns true if any formatting parameters are used that would have an effect on strings,
-    /// like `{:+2}` instead of just `{}`.
+    /// like `{:<2}` instead of just `{}`.
+    ///
+    /// `{:+}` would return false for `has_string_formatting`, but true for
+    /// `has_primitive_formatting` as it effects numbers but not strings
     pub fn has_string_formatting(&self) -> bool {
+        self.has_formatting(field_effects_string)
+    }
+
+    /// Returns true if any formatting parameters are used that would have an effect on primitives,
+    /// like `{:+2}` instead of just `{}`.
+    pub fn has_primitive_formatting(&self) -> bool {
+        self.has_formatting(|field| match field.ident.name {
+            sym::flags => matches!(
+                field.expr.kind,
+                ExprKind::Lit(Spanned {
+                    node: LitKind::Int(0, _),
+                    ..
+                }),
+            ),
+            _ => field_effects_string(field),
+        })
+    }
+
+    fn has_formatting(&self, callback: impl FnMut(&ExprField<'_>) -> bool) -> bool {
         self.spec.map_or(false, |spec| {
             // `!` because these conditions check that `self` is unformatted.
             !if_chain! {
@@ -536,21 +606,23 @@ impl<'tcx> FormatArgsArg<'tcx> {
                 if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym::format);
                 // struct `core::fmt::rt::v1::FormatSpec`
                 if let ExprKind::Struct(_, subfields, _) = format_field.expr.kind;
-                if subfields.iter().all(|field| match field.ident.name {
-                    sym::precision | sym::width => match field.expr.kind {
-                        ExprKind::Path(QPath::Resolved(_, path)) => {
-                            path.segments.last().unwrap().ident.name == sym::Implied
-                        }
-                        _ => false,
-                    }
-                    _ => true,
-                });
+                if subfields.iter().all(callback);
                 then { true } else { false }
             }
         })
     }
 }
 
+fn field_effects_string(field: &ExprField<'_>) -> bool {
+    match field.ident.name {
+        sym::precision | sym::width => match field.expr.kind {
+            ExprKind::Path(QPath::Resolved(_, path)) => path.segments.last().unwrap().ident.name == sym::Implied,
+            _ => false,
+        },
+        _ => true,
+    }
+}
+
 /// A node with a `HirId` and a `Span`
 pub trait HirNode {
     fn hir_id(&self) -> HirId;

tests/ui/print_literal.rs

@@ -26,17 +26,15 @@ fn main() {
     print!("Hello {}", "world");
     println!("Hello {} {}", world, "world");
     println!("Hello {}", "world");
+    println!("{} {:.4}", "a literal", 5);
 
     // positional args don't change the fact
     // that we're using a literal -- this should
     // throw a warning
     println!("{0} {1}", "hello", "world");
+    println!("{1} {0}", "hello", "world");
 
     // named args shouldn't change anything either
     println!("{foo} {bar}", foo = "hello", bar = "world");
-
-    // expansions containing FormatSpecs are not currently supported
-    println!("{1} {0}", "hello", "world");
     println!("{bar} {foo}", foo = "hello", bar = "world");
-    println!("{} {:.4}", "a literal", 5);
 }