Skip to content

Commit 576f561

Browse files
committed
Auto merge of #8626 - pitaj:format_add_string, r=xFrednet
New lint `format_add_strings` Closes #6261 changelog: Added [`format_add_string`]: recommend using `write!` instead of appending the result of `format!`
2 parents ecb3c3f + 67badbe commit 576f561

16 files changed

+163
-45
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3314,6 +3314,7 @@ Released 2018-09-13
33143314
[`forget_non_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_non_drop
33153315
[`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
33163316
[`format_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args
3317+
[`format_push_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_push_string
33173318
[`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
33183319
[`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
33193320
[`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10

clippy_dev/src/new_lint.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::clippy_project_root;
22
use indoc::indoc;
3+
use std::fmt::Write as _;
34
use std::fs::{self, OpenOptions};
45
use std::io::prelude::*;
56
use std::io::{self, ErrorKind};
@@ -232,7 +233,8 @@ fn get_lint_file_contents(lint: &LintData<'_>, enable_msrv: bool) -> String {
232233
)
233234
});
234235

235-
result.push_str(&format!(
236+
let _ = write!(
237+
result,
236238
indoc! {r#"
237239
declare_clippy_lint! {{
238240
/// ### What it does
@@ -256,7 +258,7 @@ fn get_lint_file_contents(lint: &LintData<'_>, enable_msrv: bool) -> String {
256258
version = version,
257259
name_upper = name_upper,
258260
category = category,
259-
));
261+
);
260262

261263
result.push_str(&if enable_msrv {
262264
format!(

clippy_dev/src/update_lints.rs

+9-7
Original file line numberDiff line numberDiff line change
@@ -217,12 +217,13 @@ fn gen_lint_group_list<'a>(group_name: &str, lints: impl Iterator<Item = &'a Lin
217217

218218
let mut output = GENERATED_FILE_COMMENT.to_string();
219219

220-
output.push_str(&format!(
221-
"store.register_group(true, \"clippy::{0}\", Some(\"clippy_{0}\"), vec![\n",
220+
let _ = writeln!(
221+
output,
222+
"store.register_group(true, \"clippy::{0}\", Some(\"clippy_{0}\"), vec![",
222223
group_name
223-
));
224+
);
224225
for (module, name) in details {
225-
output.push_str(&format!(" LintId::of({}::{}),\n", module, name));
226+
let _ = writeln!(output, " LintId::of({}::{}),", module, name);
226227
}
227228
output.push_str("])\n");
228229

@@ -235,15 +236,16 @@ fn gen_deprecated(lints: &[DeprecatedLint]) -> String {
235236
let mut output = GENERATED_FILE_COMMENT.to_string();
236237
output.push_str("{\n");
237238
for lint in lints {
238-
output.push_str(&format!(
239+
let _ = write!(
240+
output,
239241
concat!(
240242
" store.register_removed(\n",
241243
" \"clippy::{}\",\n",
242244
" \"{}\",\n",
243245
" );\n"
244246
),
245247
lint.name, lint.reason,
246-
));
248+
);
247249
}
248250
output.push_str("}\n");
249251

@@ -269,7 +271,7 @@ fn gen_register_lint_list<'a>(
269271
if !is_public {
270272
output.push_str(" #[cfg(feature = \"internal\")]\n");
271273
}
272-
output.push_str(&format!(" {}::{},\n", module_name, lint_name));
274+
let _ = writeln!(output, " {}::{},", module_name, lint_name);
273275
}
274276
output.push_str("])\n");
275277

+77
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
use clippy_utils::diagnostics::span_lint_and_help;
2+
use clippy_utils::ty::is_type_diagnostic_item;
3+
use clippy_utils::{match_def_path, paths, peel_hir_expr_refs};
4+
use rustc_hir::{BinOpKind, Expr, ExprKind};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_session::{declare_lint_pass, declare_tool_lint};
7+
use rustc_span::sym;
8+
9+
declare_clippy_lint! {
10+
/// ### What it does
11+
/// Detects cases where the result of a `format!` call is
12+
/// appended to an existing `String`.
13+
///
14+
/// ### Why is this bad?
15+
/// Introduces an extra, avoidable heap allocation.
16+
///
17+
/// ### Example
18+
/// ```rust
19+
/// let mut s = String::new();
20+
/// s += &format!("0x{:X}", 1024);
21+
/// s.push_str(&format!("0x{:X}", 1024));
22+
/// ```
23+
/// Use instead:
24+
/// ```rust
25+
/// use std::fmt::Write as _; // import without risk of name clashing
26+
///
27+
/// let mut s = String::new();
28+
/// let _ = write!(s, "0x{:X}", 1024);
29+
/// ```
30+
#[clippy::version = "1.61.0"]
31+
pub FORMAT_PUSH_STRING,
32+
perf,
33+
"`format!(..)` appended to existing `String`"
34+
}
35+
declare_lint_pass!(FormatPushString => [FORMAT_PUSH_STRING]);
36+
37+
fn is_string(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
38+
is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(e).peel_refs(), sym::String)
39+
}
40+
fn is_format(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
41+
if let Some(macro_def_id) = e.span.ctxt().outer_expn_data().macro_def_id {
42+
cx.tcx.get_diagnostic_name(macro_def_id) == Some(sym::format_macro)
43+
} else {
44+
false
45+
}
46+
}
47+
48+
impl<'tcx> LateLintPass<'tcx> for FormatPushString {
49+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
50+
let arg = match expr.kind {
51+
ExprKind::MethodCall(_, [_, arg], _) => {
52+
if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) &&
53+
match_def_path(cx, fn_def_id, &paths::PUSH_STR) {
54+
arg
55+
} else {
56+
return;
57+
}
58+
}
59+
ExprKind::AssignOp(op, left, arg)
60+
if op.node == BinOpKind::Add && is_string(cx, left) => {
61+
arg
62+
},
63+
_ => return,
64+
};
65+
let (arg, _) = peel_hir_expr_refs(arg);
66+
if is_format(cx, arg) {
67+
span_lint_and_help(
68+
cx,
69+
FORMAT_PUSH_STRING,
70+
expr.span,
71+
"`format!(..)` appended to existing `String`",
72+
None,
73+
"consider using `write!` to avoid the extra allocation",
74+
);
75+
}
76+
}
77+
}

clippy_lints/src/inconsistent_struct_constructor.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use rustc_hir::{self as hir, ExprKind};
77
use rustc_lint::{LateContext, LateLintPass};
88
use rustc_session::{declare_lint_pass, declare_tool_lint};
99
use rustc_span::symbol::Symbol;
10+
use std::fmt::Write as _;
1011

1112
declare_clippy_lint! {
1213
/// ### What it does
@@ -89,7 +90,7 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
8990
let mut fields_snippet = String::new();
9091
let (last_ident, idents) = ordered_fields.split_last().unwrap();
9192
for ident in idents {
92-
fields_snippet.push_str(&format!("{}, ", ident));
93+
let _ = write!(fields_snippet, "{}, ", ident);
9394
}
9495
fields_snippet.push_str(&last_ident.to_string());
9596

clippy_lints/src/lib.register_all.rs

+1
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
7777
LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
7878
LintId::of(format_impl::PRINT_IN_FORMAT_IMPL),
7979
LintId::of(format_impl::RECURSIVE_FORMAT_IMPL),
80+
LintId::of(format_push_string::FORMAT_PUSH_STRING),
8081
LintId::of(formatting::POSSIBLE_MISSING_COMMA),
8182
LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
8283
LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING),

clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ store.register_lints(&[
165165
format_args::TO_STRING_IN_FORMAT_ARGS,
166166
format_impl::PRINT_IN_FORMAT_IMPL,
167167
format_impl::RECURSIVE_FORMAT_IMPL,
168+
format_push_string::FORMAT_PUSH_STRING,
168169
formatting::POSSIBLE_MISSING_COMMA,
169170
formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
170171
formatting::SUSPICIOUS_ELSE_FORMATTING,

clippy_lints/src/lib.register_perf.rs

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
77
LintId::of(escape::BOXED_LOCAL),
88
LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
99
LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
10+
LintId::of(format_push_string::FORMAT_PUSH_STRING),
1011
LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
1112
LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
1213
LintId::of(loops::MANUAL_MEMCPY),

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ mod floating_point_arithmetic;
231231
mod format;
232232
mod format_args;
233233
mod format_impl;
234+
mod format_push_string;
234235
mod formatting;
235236
mod from_over_into;
236237
mod from_str_radix_10;
@@ -873,6 +874,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
873874
store.register_early_pass(|| Box::new(empty_structs_with_brackets::EmptyStructsWithBrackets));
874875
store.register_late_pass(|| Box::new(unnecessary_owned_empty_strings::UnnecessaryOwnedEmptyStrings));
875876
store.register_early_pass(|| Box::new(pub_use::PubUse));
877+
store.register_late_pass(|| Box::new(format_push_string::FormatPushString));
876878
// add lints here, do not remove this comment, it's used in `new_lint`
877879
}
878880

clippy_lints/src/trait_bounds.rs

+5-4
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ use rustc_hir::{
1414
use rustc_lint::{LateContext, LateLintPass};
1515
use rustc_session::{declare_tool_lint, impl_lint_pass};
1616
use rustc_span::Span;
17+
use std::fmt::Write as _;
1718

1819
declare_clippy_lint! {
1920
/// ### What it does
@@ -184,19 +185,19 @@ impl TraitBounds {
184185
for b in v.iter() {
185186
if let GenericBound::Trait(ref poly_trait_ref, _) = b {
186187
let path = &poly_trait_ref.trait_ref.path;
187-
hint_string.push_str(&format!(
188+
let _ = write!(hint_string,
188189
" {} +",
189190
snippet_with_applicability(cx, path.span, "..", &mut applicability)
190-
));
191+
);
191192
}
192193
}
193194
for b in p.bounds.iter() {
194195
if let GenericBound::Trait(ref poly_trait_ref, _) = b {
195196
let path = &poly_trait_ref.trait_ref.path;
196-
hint_string.push_str(&format!(
197+
let _ = write!(hint_string,
197198
" {} +",
198199
snippet_with_applicability(cx, path.span, "..", &mut applicability)
199-
));
200+
);
200201
}
201202
}
202203
hint_string.truncate(hint_string.len() - 2);

clippy_utils/src/sugg.rs

+4-6
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use rustc_span::source_map::{BytePos, CharPos, Pos, Span, SyntaxContext};
1818
use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
1919
use std::borrow::Cow;
2020
use std::convert::TryInto;
21-
use std::fmt::Display;
21+
use std::fmt::{Display, Write as _};
2222
use std::iter;
2323
use std::ops::{Add, Neg, Not, Sub};
2424

@@ -901,7 +901,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
901901
if cmt.place.projections.is_empty() {
902902
// handle item without any projection, that needs an explicit borrowing
903903
// i.e.: suggest `&x` instead of `x`
904-
self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str));
904+
let _ = write!(self.suggestion_start, "{}&{}", start_snip, ident_str);
905905
} else {
906906
// cases where a parent `Call` or `MethodCall` is using the item
907907
// 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> {
916916
// given expression is the self argument and will be handled completely by the compiler
917917
// i.e.: `|x| x.is_something()`
918918
ExprKind::MethodCall(_, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => {
919-
self.suggestion_start
920-
.push_str(&format!("{}{}", start_snip, ident_str_with_proj));
919+
let _ = write!(self.suggestion_start, "{}{}", start_snip, ident_str_with_proj);
921920
self.next_pos = span.hi();
922921
return;
923922
},
@@ -1025,8 +1024,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
10251024
}
10261025
}
10271026

1028-
self.suggestion_start
1029-
.push_str(&format!("{}{}", start_snip, replacement_str));
1027+
let _ = write!(self.suggestion_start, "{}{}", start_snip, replacement_str);
10301028
}
10311029
self.next_pos = span.hi();
10321030
}

lintcheck/src/main.rs

+9-6
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#![allow(clippy::collapsible_else_if)]
99

1010
use std::ffi::OsStr;
11+
use std::fmt::Write as _;
1112
use std::process::Command;
1213
use std::sync::atomic::{AtomicUsize, Ordering};
1314
use std::{collections::HashMap, io::ErrorKind};
@@ -110,11 +111,12 @@ impl ClippyWarning {
110111
let lint = format!("`{}`", self.linttype);
111112

112113
let mut output = String::from("| ");
113-
output.push_str(&format!(
114+
let _ = write!(
115+
output,
114116
"[`{}`](../target/lintcheck/sources/{}#L{})",
115117
file_with_pos, file, self.line
116-
));
117-
output.push_str(&format!(r#" | {:<50} | "{}" |"#, lint, self.message));
118+
);
119+
let _ = write!(output, r#" | {:<50} | "{}" |"#, lint, self.message);
118120
output.push('\n');
119121
output
120122
} else {
@@ -835,10 +837,11 @@ pub fn main() {
835837
text.push_str("| file | lint | message |\n");
836838
text.push_str("| --- | --- | --- |\n");
837839
}
838-
text.push_str(&format!("{}", all_msgs.join("")));
840+
write!(text, "{}", all_msgs.join(""));
839841
text.push_str("\n\n### ICEs:\n");
840-
ices.iter()
841-
.for_each(|(cratename, msg)| text.push_str(&format!("{}: '{}'", cratename, msg)));
842+
for (cratename, msg) in ices.iter() {
843+
let _ = write!(text, "{}: '{}'", cratename, msg);
844+
}
842845

843846
println!("Writing logs to {}", config.lintcheck_results_path.display());
844847
std::fs::create_dir_all(config.lintcheck_results_path.parent().unwrap()).unwrap();

tests/ui/format_push_string.rs

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
#![warn(clippy::format_push_string)]
2+
3+
fn main() {
4+
let mut string = String::new();
5+
string += &format!("{:?}", 1234);
6+
string.push_str(&format!("{:?}", 5678));
7+
}

tests/ui/format_push_string.stderr

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
error: `format!(..)` appended to existing `String`
2+
--> $DIR/format_push_string.rs:5:5
3+
|
4+
LL | string += &format!("{:?}", 1234);
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::format-push-string` implied by `-D warnings`
8+
= help: consider using `write!` to avoid the extra allocation
9+
10+
error: `format!(..)` appended to existing `String`
11+
--> $DIR/format_push_string.rs:6:5
12+
|
13+
LL | string.push_str(&format!("{:?}", 5678));
14+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15+
|
16+
= help: consider using `write!` to avoid the extra allocation
17+
18+
error: aborting due to 2 previous errors
19+

tests/ui/identity_op.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use std::fmt::Write as _;
2+
13
const ONE: i64 = 1;
24
const NEG_ONE: i64 = -1;
35
const ZERO: i64 = 0;
@@ -7,7 +9,7 @@ struct A(String);
79
impl std::ops::Shl<i32> for A {
810
type Output = A;
911
fn shl(mut self, other: i32) -> Self {
10-
self.0.push_str(&format!("{}", other));
12+
let _ = write!(self.0, "{}", other);
1113
self
1214
}
1315
}

0 commit comments

Comments
 (0)