Auto merge of rust-lang#8626 - pitaj:format_add_string, r=llogiq

New lint `format_add_strings` Closes rust-lang#6261 changelog: Added [`format_add_string`]: recommend using `write!` instead of appending the result of `format!`
OneSignal · Apr 14, 2022 · aade96f · aade96f
2 parents ecb3c3f + 67badbe
commit aade96f
Show file tree

Hide file tree

Showing 16 changed files with 163 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3314,6 +3314,7 @@ Released 2018-09-13
 [`forget_non_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_non_drop
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
 [`format_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args
+[`format_push_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_push_string
 [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
 [`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
 [`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10

diff --git a/clippy_dev/src/new_lint.rs b/clippy_dev/src/new_lint.rs
@@ -1,5 +1,6 @@
 use crate::clippy_project_root;
 use indoc::indoc;
+use std::fmt::Write as _;
 use std::fs::{self, OpenOptions};
 use std::io::prelude::*;
 use std::io::{self, ErrorKind};
@@ -232,7 +233,8 @@ fn get_lint_file_contents(lint: &LintData<'_>, enable_msrv: bool) -> String {
         )
     });
 
-    result.push_str(&format!(
+    let _ = write!(
+        result,
         indoc! {r#"
             declare_clippy_lint! {{
                 /// ### What it does
@@ -256,7 +258,7 @@ fn get_lint_file_contents(lint: &LintData<'_>, enable_msrv: bool) -> String {
         version = version,
         name_upper = name_upper,
         category = category,
-    ));
+    );
 
     result.push_str(&if enable_msrv {
         format!(

diff --git a/clippy_dev/src/update_lints.rs b/clippy_dev/src/update_lints.rs
@@ -217,12 +217,13 @@ fn gen_lint_group_list<'a>(group_name: &str, lints: impl Iterator<Item = &'a Lin
 
     let mut output = GENERATED_FILE_COMMENT.to_string();
 
-    output.push_str(&format!(
-        "store.register_group(true, \"clippy::{0}\", Some(\"clippy_{0}\"), vec![\n",
+    let _ = writeln!(
+        output,
+        "store.register_group(true, \"clippy::{0}\", Some(\"clippy_{0}\"), vec![",
         group_name
-    ));
+    );
     for (module, name) in details {
-        output.push_str(&format!("    LintId::of({}::{}),\n", module, name));
+        let _ = writeln!(output, "    LintId::of({}::{}),", module, name);
     }
     output.push_str("])\n");
 
@@ -235,15 +236,16 @@ fn gen_deprecated(lints: &[DeprecatedLint]) -> String {
     let mut output = GENERATED_FILE_COMMENT.to_string();
     output.push_str("{\n");
     for lint in lints {
-        output.push_str(&format!(
+        let _ = write!(
+            output,
             concat!(
                 "    store.register_removed(\n",
                 "        \"clippy::{}\",\n",
                 "        \"{}\",\n",
                 "    );\n"
             ),
             lint.name, lint.reason,
-        ));
+        );
     }
     output.push_str("}\n");
 
@@ -269,7 +271,7 @@ fn gen_register_lint_list<'a>(
         if !is_public {
             output.push_str("    #[cfg(feature = \"internal\")]\n");
         }
-        output.push_str(&format!("    {}::{},\n", module_name, lint_name));
+        let _ = writeln!(output, "    {}::{},", module_name, lint_name);
     }
     output.push_str("])\n");
 

diff --git a/clippy_lints/src/format_push_string.rs b/clippy_lints/src/format_push_string.rs
@@ -0,0 +1,77 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{match_def_path, paths, peel_hir_expr_refs};
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detects cases where the result of a `format!` call is
+    /// appended to an existing `String`.
+    ///
+    /// ### Why is this bad?
+    /// Introduces an extra, avoidable heap allocation.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let mut s = String::new();
+    /// s += &format!("0x{:X}", 1024);
+    /// s.push_str(&format!("0x{:X}", 1024));
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::fmt::Write as _; // import without risk of name clashing
+    ///
+    /// let mut s = String::new();
+    /// let _ = write!(s, "0x{:X}", 1024);
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub FORMAT_PUSH_STRING,
+    perf,
+    "`format!(..)` appended to existing `String`"
+}
+declare_lint_pass!(FormatPushString => [FORMAT_PUSH_STRING]);
+
+fn is_string(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
+    is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(e).peel_refs(), sym::String)
+}
+fn is_format(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
+    if 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 {
+        false
+    }
+}
+
+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], _) => {
+                if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) &&
+                match_def_path(cx, fn_def_id, &paths::PUSH_STR) {
+                    arg
+                } else {
+                    return;
+                }
+            }
+            ExprKind::AssignOp(op, left, arg)
+            if op.node == BinOpKind::Add && is_string(cx, left) => {
+                arg
+            },
+            _ => return,
+        };
+        let (arg, _) = peel_hir_expr_refs(arg);
+        if is_format(cx, arg) {
+            span_lint_and_help(
+                cx,
+                FORMAT_PUSH_STRING,
+                expr.span,
+                "`format!(..)` appended to existing `String`",
+                None,
+                "consider using `write!` to avoid the extra allocation",
+            );
+        }
+    }
+}
diff --git a/clippy_lints/src/inconsistent_struct_constructor.rs b/clippy_lints/src/inconsistent_struct_constructor.rs
@@ -7,6 +7,7 @@ use rustc_hir::{self as hir, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::Symbol;
+use std::fmt::Write as _;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -89,7 +90,7 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
                 let mut fields_snippet = String::new();
                 let (last_ident, idents) = ordered_fields.split_last().unwrap();
                 for ident in idents {
-                    fields_snippet.push_str(&format!("{}, ", ident));
+                    let _ = write!(fields_snippet, "{}, ", ident);
                 }
                 fields_snippet.push_str(&last_ident.to_string());
 

diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs
@@ -77,6 +77,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
     LintId::of(format_impl::PRINT_IN_FORMAT_IMPL),
     LintId::of(format_impl::RECURSIVE_FORMAT_IMPL),
+    LintId::of(format_push_string::FORMAT_PUSH_STRING),
     LintId::of(formatting::POSSIBLE_MISSING_COMMA),
     LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
     LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING),

diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -165,6 +165,7 @@ store.register_lints(&[
     format_args::TO_STRING_IN_FORMAT_ARGS,
     format_impl::PRINT_IN_FORMAT_IMPL,
     format_impl::RECURSIVE_FORMAT_IMPL,
+    format_push_string::FORMAT_PUSH_STRING,
     formatting::POSSIBLE_MISSING_COMMA,
     formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
     formatting::SUSPICIOUS_ELSE_FORMATTING,

diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs
@@ -7,6 +7,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(escape::BOXED_LOCAL),
     LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
     LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
+    LintId::of(format_push_string::FORMAT_PUSH_STRING),
     LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
     LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
     LintId::of(loops::MANUAL_MEMCPY),

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -231,6 +231,7 @@ mod floating_point_arithmetic;
 mod format;
 mod format_args;
 mod format_impl;
+mod format_push_string;
 mod formatting;
 mod from_over_into;
 mod from_str_radix_10;
@@ -873,6 +874,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| Box::new(empty_structs_with_brackets::EmptyStructsWithBrackets));
     store.register_late_pass(|| Box::new(unnecessary_owned_empty_strings::UnnecessaryOwnedEmptyStrings));
     store.register_early_pass(|| Box::new(pub_use::PubUse));
+    store.register_late_pass(|| Box::new(format_push_string::FormatPushString));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs
@@ -14,6 +14,7 @@ use rustc_hir::{
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::Span;
+use std::fmt::Write as _;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -184,19 +185,19 @@ impl TraitBounds {
                     for b in v.iter() {
                         if let GenericBound::Trait(ref poly_trait_ref, _) = b {
                             let path = &poly_trait_ref.trait_ref.path;
-                            hint_string.push_str(&format!(
+                            let _ = write!(hint_string,
                                 " {} +",
                                 snippet_with_applicability(cx, path.span, "..", &mut applicability)
-                            ));
+                            );
                         }
                     }
                     for b in p.bounds.iter() {
                         if let GenericBound::Trait(ref poly_trait_ref, _) = b {
                             let path = &poly_trait_ref.trait_ref.path;
-                            hint_string.push_str(&format!(
+                            let _ = write!(hint_string,
                                 " {} +",
                                 snippet_with_applicability(cx, path.span, "..", &mut applicability)
-                            ));
+                            );
                         }
                     }
                     hint_string.truncate(hint_string.len() - 2);

diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs
@@ -18,7 +18,7 @@ use rustc_span::source_map::{BytePos, CharPos, Pos, Span, SyntaxContext};
 use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 use std::borrow::Cow;
 use std::convert::TryInto;
-use std::fmt::Display;
+use std::fmt::{Display, Write as _};
 use std::iter;
 use std::ops::{Add, Neg, Not, Sub};
 
@@ -901,7 +901,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
             if cmt.place.projections.is_empty() {
                 // handle item without any projection, that needs an explicit borrowing
                 // i.e.: suggest `&x` instead of `x`
-                self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str));
+                let _ = write!(self.suggestion_start, "{}&{}", start_snip, ident_str);
             } else {
                 // cases where a parent `Call` or `MethodCall` is using the item
                 // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()`
@@ -916,8 +916,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
                         // given expression is the self argument and will be handled completely by the compiler
                         // i.e.: `|x| x.is_something()`
                         ExprKind::MethodCall(_, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => {
-                            self.suggestion_start
-                                .push_str(&format!("{}{}", start_snip, ident_str_with_proj));
+                            let _ = write!(self.suggestion_start, "{}{}", start_snip, ident_str_with_proj);
                             self.next_pos = span.hi();
                             return;
                         },
@@ -1025,8 +1024,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
                     }
                 }
 
-                self.suggestion_start
-                    .push_str(&format!("{}{}", start_snip, replacement_str));
+                let _ = write!(self.suggestion_start, "{}{}", start_snip, replacement_str);
             }
             self.next_pos = span.hi();
         }

diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs
@@ -8,6 +8,7 @@
 #![allow(clippy::collapsible_else_if)]
 
 use std::ffi::OsStr;
+use std::fmt::Write as _;
 use std::process::Command;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::{collections::HashMap, io::ErrorKind};
@@ -110,11 +111,12 @@ impl ClippyWarning {
             let lint = format!("`{}`", self.linttype);
 
             let mut output = String::from("| ");
-            output.push_str(&format!(
+            let _ = write!(
+                output,
                 "[`{}`](../target/lintcheck/sources/{}#L{})",
                 file_with_pos, file, self.line
-            ));
-            output.push_str(&format!(r#" | {:<50} | "{}" |"#, lint, self.message));
+            );
+            let _ = write!(output, r#" | {:<50} | "{}" |"#, lint, self.message);
             output.push('\n');
             output
         } else {
@@ -835,10 +837,11 @@ pub fn main() {
         text.push_str("| file | lint | message |\n");
         text.push_str("| --- | --- | --- |\n");
     }
-    text.push_str(&format!("{}", all_msgs.join("")));
+    write!(text, "{}", all_msgs.join(""));
     text.push_str("\n\n### ICEs:\n");
-    ices.iter()
-        .for_each(|(cratename, msg)| text.push_str(&format!("{}: '{}'", cratename, msg)));
+    for (cratename, msg) in ices.iter() {
+        let _ = write!(text, "{}: '{}'", cratename, msg);
+    }
 
     println!("Writing logs to {}", config.lintcheck_results_path.display());
     std::fs::create_dir_all(config.lintcheck_results_path.parent().unwrap()).unwrap();

diff --git a/tests/ui/format_push_string.rs b/tests/ui/format_push_string.rs
@@ -0,0 +1,7 @@
+#![warn(clippy::format_push_string)]
+
+fn main() {
+    let mut string = String::new();
+    string += &format!("{:?}", 1234);
+    string.push_str(&format!("{:?}", 5678));
+}
diff --git a/tests/ui/format_push_string.stderr b/tests/ui/format_push_string.stderr
@@ -0,0 +1,19 @@
+error: `format!(..)` appended to existing `String`
+  --> $DIR/format_push_string.rs:5:5
+   |
+LL |     string += &format!("{:?}", 1234);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::format-push-string` implied by `-D warnings`
+   = help: consider using `write!` to avoid the extra allocation
+
+error: `format!(..)` appended to existing `String`
+  --> $DIR/format_push_string.rs:6:5
+   |
+LL |     string.push_str(&format!("{:?}", 5678));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using `write!` to avoid the extra allocation
+
+error: aborting due to 2 previous errors
+
diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs
@@ -1,3 +1,5 @@
+use std::fmt::Write as _;
+
 const ONE: i64 = 1;
 const NEG_ONE: i64 = -1;
 const ZERO: i64 = 0;
@@ -7,7 +9,7 @@ struct A(String);
 impl std::ops::Shl<i32> for A {
     type Output = A;
     fn shl(mut self, other: i32) -> Self {
-        self.0.push_str(&format!("{}", other));
+        let _ = write!(self.0, "{}", other);
         self
     }
 }