diff --git a/clippy_lints/src/format_push_string.rs b/clippy_lints/src/format_push_string.rs index a23ba9ab837a..c971559deaf1 100644 --- a/clippy_lints/src/format_push_string.rs +++ b/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), +} + +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, + out: &mut Vec, + ) { + 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) +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4542105d3277..6cfc43b251b4 100644 --- a/clippy_lints/src/lib.rs +++ b/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)), diff --git a/tests/ui/format_push_string.fixed b/tests/ui/format_push_string.fixed new file mode 100644 index 000000000000..f6396d9982a3 --- /dev/null +++ b/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 + } + } + } +} diff --git a/tests/ui/format_push_string.rs b/tests/ui/format_push_string.rs index 056ef59ff0e2..1ed0f5b3ac59 100644 --- a/tests/ui/format_push_string.rs +++ b/tests/ui/format_push_string.rs @@ -1,44 +1,132 @@ #![warn(clippy::format_push_string)] fn main() { + use std::fmt::Write; + let mut string = String::new(); string += &format!("{:?}", 1234); //~^ format_push_string string.push_str(&format!("{:?}", 5678)); //~^ format_push_string + + macro_rules! string { + () => { + String::new() + }; + } + string!().push_str(&format!("{:?}", 5678)); + //~^ format_push_string } -mod issue9493 { - pub fn u8vec_to_hex(vector: &Vec, upper: bool) -> String { - let mut hex = String::with_capacity(vector.len() * 2); - for byte in vector { - hex += &(if upper { +// 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 _; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported { + fn main(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); //~^ format_push_string + } + } + + mod imported_anonymously_in_module { + use std::fmt::Write as _; - format!("{byte:02X}") - } else { - format!("{byte:02x}") - }); + fn main(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported_in_module { + use std::fmt::Write; + + fn main(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported_and_imported { + fn foo(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } } - hex } - pub fn other_cases() { - let mut s = String::new(); - // if let - s += &(if let Some(_a) = Some(1234) { - //~^ format_push_string - - format!("{}", 1234) - } else { - format!("{}", 1234) - }); - // match - s += &(match Some(1234) { - //~^ format_push_string - Some(_) => format!("{}", 1234), - None => format!("{}", 1234), - }); + mod add_assign { + mod imported_anonymously { + fn main(string: &mut String) { + use std::fmt::Write as _; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported { + fn main(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported_anonymously_in_module { + use std::fmt::Write as _; + + fn main(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported_in_module { + use std::fmt::Write; + + fn main(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + mod imported_and_imported { + fn foo(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } } } diff --git a/tests/ui/format_push_string.stderr b/tests/ui/format_push_string.stderr index bba2a8947c43..80331ee9d8a5 100644 --- a/tests/ui/format_push_string.stderr +++ b/tests/ui/format_push_string.stderr @@ -1,60 +1,199 @@ error: `format!(..)` appended to existing `String` - --> tests/ui/format_push_string.rs:5:5 + --> tests/ui/format_push_string.rs:7:5 | LL | string += &format!("{:?}", 1234); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using `write!` to avoid the extra allocation + = note: you may need to import `std::fmt::Write` = note: `-D clippy::format-push-string` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::format_push_string)]` +help: consider using `write!` to avoid the extra allocation + | +LL - string += &format!("{:?}", 1234); +LL + let _ = write!(string, "{:?}", 1234); + | error: `format!(..)` appended to existing `String` - --> tests/ui/format_push_string.rs:8:5 + --> tests/ui/format_push_string.rs:10:5 | LL | string.push_str(&format!("{:?}", 5678)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using `write!` to avoid the extra allocation + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 5678)); +LL + let _ = write!(string, "{:?}", 5678); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:18:5 + | +LL | string!().push_str(&format!("{:?}", 5678)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string!().push_str(&format!("{:?}", 5678)); +LL + let _ = write!(string!(), "{:?}", 5678); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:30:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:39:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:48:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:57:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:66:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:73:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | error: `format!(..)` appended to existing `String` - --> tests/ui/format_push_string.rs:16:13 + --> tests/ui/format_push_string.rs:84:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -LL | / hex += &(if upper { -LL | | -LL | | -LL | | format!("{byte:02X}") -LL | | } else { -LL | | format!("{byte:02x}") -LL | | }); - | |______________^ + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); | - = help: consider using `write!` to avoid the extra allocation error: `format!(..)` appended to existing `String` - --> tests/ui/format_push_string.rs:30:9 + --> tests/ui/format_push_string.rs:93:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -LL | / s += &(if let Some(_a) = Some(1234) { -LL | | -LL | | -LL | | format!("{}", 1234) -LL | | } else { -LL | | format!("{}", 1234) -LL | | }); - | |__________^ + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); | - = help: consider using `write!` to avoid the extra allocation error: `format!(..)` appended to existing `String` - --> tests/ui/format_push_string.rs:38:9 + --> tests/ui/format_push_string.rs:102:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:111:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:120:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string.rs:127:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation | -LL | / s += &(match Some(1234) { -LL | | -LL | | Some(_) => format!("{}", 1234), -LL | | None => format!("{}", 1234), -LL | | }); - | |__________^ +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); | - = help: consider using `write!` to avoid the extra allocation -error: aborting due to 5 previous errors +error: aborting due to 15 previous errors diff --git a/tests/ui/format_push_string_no_std.fixed b/tests/ui/format_push_string_no_std.fixed new file mode 100644 index 000000000000..32d8659dcbd5 --- /dev/null +++ b/tests/ui/format_push_string_no_std.fixed @@ -0,0 +1,15 @@ +#![warn(clippy::format_push_string)] +#![no_std] + +extern crate alloc; + +use alloc::format; +use alloc::string::String; + +fn foo(string: &mut String) { + use core::fmt::Write; + + // TODO: recognize the already imported `fmt::Write`, and don't suggest importing it again + let _ = write!(string, "{:?}", 1234); + //~^ format_push_string +} diff --git a/tests/ui/format_push_string_no_std.rs b/tests/ui/format_push_string_no_std.rs new file mode 100644 index 000000000000..a74189abe528 --- /dev/null +++ b/tests/ui/format_push_string_no_std.rs @@ -0,0 +1,15 @@ +#![warn(clippy::format_push_string)] +#![no_std] + +extern crate alloc; + +use alloc::format; +use alloc::string::String; + +fn foo(string: &mut String) { + use core::fmt::Write; + + // TODO: recognize the already imported `fmt::Write`, and don't suggest importing it again + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string +} diff --git a/tests/ui/format_push_string_no_std.stderr b/tests/ui/format_push_string_no_std.stderr new file mode 100644 index 000000000000..e32370818119 --- /dev/null +++ b/tests/ui/format_push_string_no_std.stderr @@ -0,0 +1,17 @@ +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_no_std.rs:13:5 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `core::fmt::Write` + = note: `-D clippy::format-push-string` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::format_push_string)]` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: aborting due to 1 previous error + diff --git a/tests/ui/format_push_string_no_std_unfixable.rs b/tests/ui/format_push_string_no_std_unfixable.rs new file mode 100644 index 000000000000..f5ed5e435b5a --- /dev/null +++ b/tests/ui/format_push_string_no_std_unfixable.rs @@ -0,0 +1,13 @@ +//@no-rustfix +#![warn(clippy::format_push_string)] +#![no_std] + +extern crate alloc; + +use alloc::format; +use alloc::string::String; + +fn foo(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string +} diff --git a/tests/ui/format_push_string_no_std_unfixable.stderr b/tests/ui/format_push_string_no_std_unfixable.stderr new file mode 100644 index 000000000000..e3705972079e --- /dev/null +++ b/tests/ui/format_push_string_no_std_unfixable.stderr @@ -0,0 +1,17 @@ +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_no_std_unfixable.rs:11:5 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `core::fmt::Write` + = note: `-D clippy::format-push-string` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::format_push_string)]` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: aborting due to 1 previous error + diff --git a/tests/ui/format_push_string_unfixable.rs b/tests/ui/format_push_string_unfixable.rs new file mode 100644 index 000000000000..ff6c13fe4a49 --- /dev/null +++ b/tests/ui/format_push_string_unfixable.rs @@ -0,0 +1,144 @@ +//@no-rustfix +#![warn(clippy::format_push_string)] + +mod issue9493 { + pub fn u8vec_to_hex(vector: &Vec, upper: bool) -> String { + let mut hex = String::with_capacity(vector.len() * 2); + for byte in vector { + hex += &(if upper { + format!("{byte:02X}") + //~^ format_push_string + } else { + format!("{byte:02x}") + }); + } + hex + } + + pub fn other_cases() { + let mut s = String::new(); + // if let + s += &(if let Some(_a) = Some(1234) { + format!("{}", 1234) + //~^ format_push_string + } else { + format!("{}", 1234) + }); + // match + s += &(match Some(1234) { + Some(_) => format!("{}", 1234), + //~^ format_push_string + None => format!("{}", 1234), + }); + } +} + +mod import_write { + mod push_str { + // TODO: suggest importing `std::fmt::Write`; + mod not_imported { + fn main(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + // TODO: suggest importing the first time, but not again + mod not_imported_and_not_imported { + fn foo(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + // TODO: suggest importing the first time, but not again + mod not_imported_and_imported { + fn foo(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + // TODO: suggest importing, but only for `bar` + mod imported_and_not_imported { + fn foo(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + } + + mod add_assign { + // TODO: suggest importing `std::fmt::Write`; + mod not_imported { + fn main(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + // TODO: suggest importing the first time, but not again + mod not_imported_and_not_imported { + fn foo(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + // TODO: suggest importing the first time, but not again + mod not_imported_and_imported { + fn foo(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + + // TODO: suggest importing, but only for `bar` + mod imported_and_not_imported { + fn foo(string: &mut String) { + use std::fmt::Write; + + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + + fn bar(string: &mut String) { + string.push_str(&format!("{:?}", 1234)); + //~^ format_push_string + } + } + } +} + +fn main() {} diff --git a/tests/ui/format_push_string_unfixable.stderr b/tests/ui/format_push_string_unfixable.stderr new file mode 100644 index 000000000000..01f20b49ab7b --- /dev/null +++ b/tests/ui/format_push_string_unfixable.stderr @@ -0,0 +1,233 @@ +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:8:13 + | +LL | / hex += &(if upper { +LL | | format!("{byte:02X}") + | | --------------------- `format!` used here +LL | | +LL | | } else { +LL | | format!("{byte:02x}") + | | --------------------- `format!` used here +LL | | }); + | |______________^ + | + = help: consider using `write!` to avoid the extra allocation + = note: you may need to import `std::fmt::Write` + = note: `-D clippy::format-push-string` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::format_push_string)]` + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:21:9 + | +LL | / s += &(if let Some(_a) = Some(1234) { +LL | | format!("{}", 1234) + | | ------------------- `format!` used here +LL | | +LL | | } else { +LL | | format!("{}", 1234) + | | ------------------- `format!` used here +LL | | }); + | |__________^ + | + = help: consider using `write!` to avoid the extra allocation + = note: you may need to import `std::fmt::Write` + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:28:9 + | +LL | / s += &(match Some(1234) { +LL | | Some(_) => format!("{}", 1234), + | | ------------------- `format!` used here +LL | | +LL | | None => format!("{}", 1234), + | | ------------------- `format!` used here +LL | | }); + | |__________^ + | + = help: consider using `write!` to avoid the extra allocation + = note: you may need to import `std::fmt::Write` + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:41:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:49:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:54:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:62:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:69:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:79:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:84:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:94:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:102:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:107:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:115:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:122:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:132:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: `format!(..)` appended to existing `String` + --> tests/ui/format_push_string_unfixable.rs:137:17 + | +LL | string.push_str(&format!("{:?}", 1234)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: you may need to import `std::fmt::Write` +help: consider using `write!` to avoid the extra allocation + | +LL - string.push_str(&format!("{:?}", 1234)); +LL + let _ = write!(string, "{:?}", 1234); + | + +error: aborting due to 17 previous errors +