mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
New lint `format_add_strings`

Closes #6261

changelog: Added [`format_add_string`]: recommend using `write!` instead of appending the result of  `format!`
This commit is contained in:
bors 2022-04-14 14:29:22 +00:00
commit aade96f902
16 changed files with 163 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -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

6
clippy_dev/src/new_lint.rs
View File

@ -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!(

16
clippy_dev/src/update_lints.rs
View File

@ -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,7 +236,8 @@ 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",
@ -243,7 +245,7 @@ fn gen_deprecated(lints: &[DeprecatedLint]) -> String {
" );\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");

77
clippy_lints/src/format_push_string.rs Normal file
View File

@ -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",
);
}
}
}

3
clippy_lints/src/inconsistent_struct_constructor.rs
View File

@ -7,6 +7,7 @@
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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
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());

1
clippy_lints/src/lib.register_all.rs
View File

@ -77,6 +77,7 @@
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),

1
clippy_lints/src/lib.register_lints.rs
View File

@ -165,6 +165,7 @@
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,

1
clippy_lints/src/lib.register_perf.rs
View File

@ -7,6 +7,7 @@
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),

2
clippy_lints/src/lib.rs
View File

@ -231,6 +231,7 @@ macro_rules! declare_clippy_lint {
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`
}

9
clippy_lints/src/trait_bounds.rs
View File

@ -14,6 +14,7 @@
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 Eq for SpanlessTy<'_, '_> {}
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);

10
clippy_utils/src/sugg.rs
View File

@ -18,7 +18,7 @@
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 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
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 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
// 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 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
}
}
self.suggestion_start
.push_str(&format!("{}{}", start_snip, replacement_str));
let _ = write!(self.suggestion_start, "{}{}", start_snip, replacement_str);
}
self.next_pos = span.hi();
}

15
lintcheck/src/main.rs
View File

@ -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 @@ fn to_output(&self, markdown: bool) -> String {
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();

7
tests/ui/format_push_string.rs Normal file
View File

@ -0,0 +1,7 @@
#![warn(clippy::format_push_string)]
fn main() {
let mut string = String::new();
string += &format!("{:?}", 1234);
string.push_str(&format!("{:?}", 5678));
}

19
tests/ui/format_push_string.stderr Normal file
View File

@ -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

4
tests/ui/identity_op.rs
View File

@ -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 @@
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
}
}

36
tests/ui/identity_op.stderr
View File

@ -1,5 +1,5 @@
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:37:5
--> $DIR/identity_op.rs:39:5
|
LL | x + 0;
| ^^^^^
@ -7,103 +7,103 @@ LL | x + 0;
= note: `-D clippy::identity-op` implied by `-D warnings`
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:38:5
--> $DIR/identity_op.rs:40:5
|
LL | x + (1 - 1);
| ^^^^^^^^^^^
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:40:5
--> $DIR/identity_op.rs:42:5
|
LL | 0 + x;
| ^^^^^
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:43:5
--> $DIR/identity_op.rs:45:5
|
LL | x | (0);
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:46:5
--> $DIR/identity_op.rs:48:5
|
LL | x * 1;
| ^^^^^
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:47:5
--> $DIR/identity_op.rs:49:5
|
LL | 1 * x;
| ^^^^^
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:53:5
--> $DIR/identity_op.rs:55:5
|
LL | -1 & x;
| ^^^^^^
error: the operation is ineffective. Consider reducing it to `u`
--> $DIR/identity_op.rs:56:5
--> $DIR/identity_op.rs:58:5
|
LL | u & 255;
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `42`
--> $DIR/identity_op.rs:59:5
--> $DIR/identity_op.rs:61:5
|
LL | 42 << 0;
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `1`
--> $DIR/identity_op.rs:60:5
--> $DIR/identity_op.rs:62:5
|
LL | 1 >> 0;
| ^^^^^^
error: the operation is ineffective. Consider reducing it to `42`
--> $DIR/identity_op.rs:61:5
--> $DIR/identity_op.rs:63:5
|
LL | 42 >> 0;
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `&x`
--> $DIR/identity_op.rs:62:5
--> $DIR/identity_op.rs:64:5
|
LL | &x >> 0;
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `x`
--> $DIR/identity_op.rs:63:5
--> $DIR/identity_op.rs:65:5
|
LL | x >> &0;
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `2`
--> $DIR/identity_op.rs:70:5
--> $DIR/identity_op.rs:72:5
|
LL | 2 % 3;
| ^^^^^
error: the operation is ineffective. Consider reducing it to `-2`
--> $DIR/identity_op.rs:71:5
--> $DIR/identity_op.rs:73:5
|
LL | -2 % 3;
| ^^^^^^
error: the operation is ineffective. Consider reducing it to `2`
--> $DIR/identity_op.rs:72:5
--> $DIR/identity_op.rs:74:5
|
LL | 2 % -3 + x;
| ^^^^^^
error: the operation is ineffective. Consider reducing it to `-2`
--> $DIR/identity_op.rs:73:5
--> $DIR/identity_op.rs:75:5
|
LL | -2 % -3 + x;
| ^^^^^^^
error: the operation is ineffective. Consider reducing it to `1`
--> $DIR/identity_op.rs:74:9
--> $DIR/identity_op.rs:76:9
|
LL | x + 1 % 3;
| ^^^^^