feat(format_push_string): give a (possibly incomplete) suggestion

clippy_lints/src/format_push_string.rs

@@ -1,10 +1,13 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::higher;
+use clippy_utils::macros::{FormatArgsStorage, format_args_inputs_span, root_macro_call_first_node};
 use clippy_utils::res::MaybeDef;
+use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
+use clippy_utils::std_or_core;
+use rustc_errors::Applicability;
 use rustc_hir::{AssignOpKind, Expr, ExprKind, LangItem, MatchSource};
-use rustc_lint::{LateContext, LateLintPass};
-use rustc_session::declare_lint_pass;
-use rustc_span::sym;
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_session::impl_lint_pass;
+use rustc_span::{Span, sym};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -38,62 +41,155 @@ declare_clippy_lint! {
     pedantic,
     "`format!(..)` appended to existing `String`"
 }
-declare_lint_pass!(FormatPushString => [FORMAT_PUSH_STRING]);
+impl_lint_pass!(FormatPushString => [FORMAT_PUSH_STRING]);
 
-fn is_string(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
-    cx.typeck_results()
-        .expr_ty(e)
-        .peel_refs()
-        .is_lang_item(cx, LangItem::String)
+pub(crate) struct FormatPushString {
+    format_args: FormatArgsStorage,
 }
-fn is_format(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
-    let e = e.peel_blocks().peel_borrows();
 
-    if e.span.from_expansion()
-        && let Some(macro_def_id) = e.span.ctxt().outer_expn_data().macro_def_id
-    {
-        cx.tcx.get_diagnostic_name(macro_def_id) == Some(sym::format_macro)
-    } else if let Some(higher::If { then, r#else, .. }) = higher::If::hir(e) {
-        is_format(cx, then) || r#else.is_some_and(|e| is_format(cx, e))
-    } else {
-        match higher::IfLetOrMatch::parse(cx, e) {
-            Some(higher::IfLetOrMatch::Match(_, arms, MatchSource::Normal)) => {
-                arms.iter().any(|arm| is_format(cx, arm.body))
-            },
-            Some(higher::IfLetOrMatch::IfLet(_, _, then, r#else, _)) => {
-                is_format(cx, then) || r#else.is_some_and(|e| is_format(cx, e))
-            },
-            _ => false,
+enum FormatSearchResults {
+    /// The expression is itself a `format!()` invocation -- we can make a suggestion to replace it
+    Direct(Span),
+    /// The expression contains zero or more `format!()`s, e.g.:
+    /// ```ignore
+    /// if true {
+    ///     format!("hello")
+    /// } else {
+    ///     format!("world")
+    /// }
+    /// ```
+    /// or
+    /// ```ignore
+    /// match true {
+    ///     true => format!("hello"),
+    ///     false => format!("world"),
+    /// }
+    Nested(Vec<Span>),
+}
+
+impl FormatPushString {
+    pub(crate) fn new(format_args: FormatArgsStorage) -> Self {
+        Self { format_args }
+    }
+
+    fn find_formats<'tcx>(&self, cx: &LateContext<'_>, e: &'tcx Expr<'tcx>) -> FormatSearchResults {
+        let expr_as_format = |e| {
+            if let Some(macro_call) = root_macro_call_first_node(cx, e)
+                && cx.tcx.is_diagnostic_item(sym::format_macro, macro_call.def_id)
+                && let Some(format_args) = self.format_args.get(cx, e, macro_call.expn)
+            {
+                Some(format_args_inputs_span(format_args))
+            } else {
+                None
+            }
+        };
+
+        let e = e.peel_blocks().peel_borrows();
+        if let Some(fmt) = expr_as_format(e) {
+            FormatSearchResults::Direct(fmt)
+        } else {
+            fn inner<'tcx>(
+                e: &'tcx Expr<'tcx>,
+                expr_as_format: &impl Fn(&'tcx Expr<'tcx>) -> Option<Span>,
+                out: &mut Vec<Span>,
+            ) {
+                let e = e.peel_blocks().peel_borrows();
+
+                match e.kind {
+                    _ if expr_as_format(e).is_some() => out.push(e.span),
+                    ExprKind::Match(_, arms, MatchSource::Normal) => {
+                        for arm in arms {
+                            inner(arm.body, expr_as_format, out);
+                        }
+                    },
+                    ExprKind::If(_, then, els) => {
+                        inner(then, expr_as_format, out);
+                        if let Some(els) = els {
+                            inner(els, expr_as_format, out);
+                        }
+                    },
+                    _ => {},
+                }
+            }
+            let mut spans = vec![];
+            inner(e, &expr_as_format, &mut spans);
+            FormatSearchResults::Nested(spans)
         }
     }
 }
 
 impl<'tcx> LateLintPass<'tcx> for FormatPushString {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        let arg = match expr.kind {
-            ExprKind::MethodCall(_, _, [arg], _) => {
+        let (recv, arg) = match expr.kind {
+            ExprKind::MethodCall(_, recv, [arg], _) => {
                 if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
                     && cx.tcx.is_diagnostic_item(sym::string_push_str, fn_def_id)
                 {
-                    arg
+                    (recv, arg)
                 } else {
                     return;
                 }
             },
-            ExprKind::AssignOp(op, left, arg) if op.node == AssignOpKind::AddAssign && is_string(cx, left) => arg,
+            ExprKind::AssignOp(op, recv, arg) if op.node == AssignOpKind::AddAssign && is_string(cx, recv) => {
+                (recv, arg)
+            },
             _ => return,
         };
-        if is_format(cx, arg) {
-            #[expect(clippy::collapsible_span_lint_calls, reason = "rust-clippy#7797")]
-            span_lint_and_then(
-                cx,
-                FORMAT_PUSH_STRING,
-                expr.span,
-                "`format!(..)` appended to existing `String`",
-                |diag| {
-                    diag.help("consider using `write!` to avoid the extra allocation");
-                },
-            );
+        let Some(std_or_core) = std_or_core(cx) else {
+            // This can't really happen, as a no-core crate wouldn't have access to `String` in the first place
+            return;
+        };
+        match self.find_formats(cx, arg) {
+            FormatSearchResults::Direct(format_args) => {
+                span_lint_and_then(
+                    cx,
+                    FORMAT_PUSH_STRING,
+                    expr.span,
+                    "`format!(..)` appended to existing `String`",
+                    |diag| {
+                        let mut app = Applicability::MaybeIncorrect;
+                        let msg = "consider using `write!` to avoid the extra allocation";
+
+                        let sugg = format!(
+                            "let _ = write!({recv}, {format_args})",
+                            recv = snippet_with_context(cx.sess(), recv.span, expr.span.ctxt(), "_", &mut app).0,
+                            format_args = snippet_with_applicability(cx.sess(), format_args, "..", &mut app),
+                        );
+                        diag.span_suggestion_verbose(expr.span, msg, sugg, app);
+
+                        // TODO: Ideally we'd use `TyCtxt::in_scope_traits` to detect whether the trait is imported, and
+                        // either not emit this note if it is, or emit an automated suggestion to import it if it isn't.
+                        // But the method doesn't seem to work, see https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/How+to+suggest+importing+a+path/with/544260181
+                        diag.note(format!("you may need to import `{std_or_core}::fmt::Write`"));
+                    },
+                );
+            },
+            FormatSearchResults::Nested(spans) => {
+                if !spans.is_empty() {
+                    span_lint_and_then(
+                        cx,
+                        FORMAT_PUSH_STRING,
+                        expr.span,
+                        "`format!(..)` appended to existing `String`",
+                        |diag| {
+                            diag.help("consider using `write!` to avoid the extra allocation");
+                            diag.span_labels(spans, "`format!` used here");
+
+                            // TODO: Ideally we'd use `TyCtxt::in_scope_traits` to detect whether the trait is imported,
+                            // and either not emit this note if it is, or emit an automated suggestion to import it if
+                            // it isn't. But the method doesn't seem to work, see https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/How+to+suggest+importing+a+path/with/544260181
+                            diag.note(format!("you may need to import `{std_or_core}::fmt::Write`"));
+                        },
+                    );
+                }
+            },
         }
     }
 }
+
+fn is_string(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
+    cx.typeck_results()
+        .expr_ty(e)
+        .peel_refs()
+        .is_lang_item(cx, LangItem::String)
+}

clippy_lints/src/lib.rs

@@ -736,7 +736,10 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
         Box::new(move |_| Box::new(cargo::Cargo::new(conf))),
         Box::new(|_| Box::new(empty_with_brackets::EmptyWithBrackets::default())),
         Box::new(|_| Box::new(unnecessary_owned_empty_strings::UnnecessaryOwnedEmptyStrings)),
-        Box::new(|_| Box::new(format_push_string::FormatPushString)),
+        {
+            let format_args = format_args_storage.clone();
+            Box::new(move |_| Box::new(format_push_string::FormatPushString::new(format_args.clone())))
+        },
         Box::new(move |_| Box::new(large_include_file::LargeIncludeFile::new(conf))),
         Box::new(|_| Box::new(strings::TrimSplitWhitespace)),
         Box::new(|_| Box::new(rc_clone_in_vec_init::RcCloneInVecInit)),

tests/ui/format_push_string.fixed

@@ -0,0 +1,132 @@
+#![warn(clippy::format_push_string)]
+
+fn main() {
+    use std::fmt::Write;
+
+    let mut string = String::new();
+    let _ = write!(string, "{:?}", 1234);
+    //~^ format_push_string
+
+    let _ = write!(string, "{:?}", 5678);
+    //~^ format_push_string
+
+    macro_rules! string {
+        () => {
+            String::new()
+        };
+    }
+    let _ = write!(string!(), "{:?}", 5678);
+    //~^ format_push_string
+}
+
+// TODO: recognize the already imported `fmt::Write`, and don't add a note suggesting to import it
+// again
+mod import_write {
+    mod push_str {
+        mod imported_anonymously {
+            fn main(string: &mut String) {
+                use std::fmt::Write as _;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported {
+            fn main(string: &mut String) {
+                use std::fmt::Write;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported_anonymously_in_module {
+            use std::fmt::Write as _;
+
+            fn main(string: &mut String) {
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported_in_module {
+            use std::fmt::Write;
+
+            fn main(string: &mut String) {
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported_and_imported {
+            fn foo(string: &mut String) {
+                use std::fmt::Write;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+
+            fn bar(string: &mut String) {
+                use std::fmt::Write;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+    }
+
+    mod add_assign {
+        mod imported_anonymously {
+            fn main(string: &mut String) {
+                use std::fmt::Write as _;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported {
+            fn main(string: &mut String) {
+                use std::fmt::Write;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported_anonymously_in_module {
+            use std::fmt::Write as _;
+
+            fn main(string: &mut String) {
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported_in_module {
+            use std::fmt::Write;
+
+            fn main(string: &mut String) {
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+
+        mod imported_and_imported {
+            fn foo(string: &mut String) {
+                use std::fmt::Write;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+
+            fn bar(string: &mut String) {
+                use std::fmt::Write;
+
+                let _ = write!(string, "{:?}", 1234);
+                //~^ format_push_string
+            }
+        }
+    }
+}