WIP: feat(format_push_string): add a suggestion

Author: ada4a

clippy_lints/src/format_push_string.rs

@@ -1,9 +1,13 @@
 use clippy_utils::diagnostics::span_lint_and_then;
+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
@@ -37,55 +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,
+}
+
+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>),
 }
-fn is_format(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
- let e = e.peel_blocks().peel_borrows();
 
- match e.kind {
- _ if e.span.from_expansion()
- && let Some(macro_def_id) = e.span.ctxt().outer_expn_data().macro_def_id =>
- {
- cx.tcx.is_diagnostic_item(sym::format_macro, macro_def_id)
- },
- ExprKind::Match(_, arms, MatchSource::Normal) => arms.iter().any(|arm| is_format(cx, arm.body)),
- ExprKind::If(_, then, els) => is_format(cx, then) || els.is_some_and(|e| is_format(cx, e)),
- _ => false,
+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
+ }
+ }
+ }
+}