diff --git a/clippy_lints/src/methods/format_collect.rs b/clippy_lints/src/methods/format_collect.rs index 74eb80782d61..1f8863f85218 100644 --- a/clippy_lints/src/methods/format_collect.rs +++ b/clippy_lints/src/methods/format_collect.rs @@ -1,17 +1,17 @@ use super::FORMAT_COLLECT; use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::macros::is_format_macro; -use clippy_utils::macros::root_macro_call_first_node; +use clippy_utils::macros::{is_format_macro, root_macro_call_first_node}; use clippy_utils::ty::is_type_lang_item; -use rustc_hir::Expr; -use rustc_hir::ExprKind; -use rustc_hir::LangItem; +use rustc_hir::{Expr, ExprKind, LangItem}; use rustc_lint::LateContext; use rustc_span::Span; -fn tail_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { +/// Same as `peel_blocks` but only actually considers blocks that are not from an expansion. +/// This is needed because always calling `peel_blocks` would otherwise remove parts of the +/// `format!` macro, which would cause `root_macro_call_first_node` to return `None`. +fn peel_non_expn_blocks<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { match expr.kind { - ExprKind::Block(block, _) if !expr.span.from_expansion() => block.expr, + ExprKind::Block(block, _) if !expr.span.from_expansion() => peel_non_expn_blocks(block.expr?), _ => Some(expr), } } @@ -20,7 +20,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, map_arg: &Expr<'_>, m if is_type_lang_item(cx, cx.typeck_results().expr_ty(expr), LangItem::String) && let ExprKind::Closure(closure) = map_arg.kind && let body = cx.tcx.hir().body(closure.body) - && let Some(value) = tail_expr(body.value) + && let Some(value) = peel_non_expn_blocks(body.value) && let Some(mac) = root_macro_call_first_node(cx, value) && is_format_macro(cx, mac.def_id) { diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f55a7727fad6..81f59f68ec6f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3385,7 +3385,12 @@ declare_clippy_lint! { /// /// ### Why is this bad? /// This allocates a new string for every element in the iterator. - /// This can be done more efficiently by creating the `String` once and appending to it using `Iterator::fold`. + /// This can be done more efficiently by creating the `String` once and appending to it in `Iterator::fold`, + /// using either the `write!` macro which supports exactly the same syntax as the `format!` macro, + /// or concatenating with `+` in case the iterator yields `&str`/`String`. + /// + /// Note also that `write!`-ing into a `String` can never fail, despite the return type of `write!` being `std::fmt::Result`, + /// so it can be safely ignored or unwrapped. /// /// ### Example /// ```rust diff --git a/tests/ui/format_collect.rs b/tests/ui/format_collect.rs index ecadb43ecb47..c7f2b7b69507 100644 --- a/tests/ui/format_collect.rs +++ b/tests/ui/format_collect.rs @@ -5,6 +5,11 @@ fn hex_encode(bytes: &[u8]) -> String { bytes.iter().map(|b| format!("{b:02X}")).collect() } +#[rustfmt::skip] +fn hex_encode_deep(bytes: &[u8]) -> String { + bytes.iter().map(|b| {{{{{ format!("{b:02X}") }}}}}).collect() +} + macro_rules! fmt { ($x:ident) => { format!("{x:02X}", x = $x) diff --git a/tests/ui/format_collect.stderr b/tests/ui/format_collect.stderr index cd71a8537621..d918f1ed466b 100644 --- a/tests/ui/format_collect.stderr +++ b/tests/ui/format_collect.stderr @@ -18,7 +18,25 @@ LL | bytes.iter().map(|b| format!("{b:02X}")).collect() = note: `-D clippy::format-collect` implied by `-D warnings` error: use of `format!` to build up a string from an iterator - --> $DIR/format_collect.rs:19:5 + --> $DIR/format_collect.rs:10:5 + | +LL | bytes.iter().map(|b| {{{{{ format!("{b:02X}") }}}}}).collect() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: call `fold` instead + --> $DIR/format_collect.rs:10:18 + | +LL | bytes.iter().map(|b| {{{{{ format!("{b:02X}") }}}}}).collect() + | ^^^ +help: ... and use the `write!` macro here + --> $DIR/format_collect.rs:10:32 + | +LL | bytes.iter().map(|b| {{{{{ format!("{b:02X}") }}}}}).collect() + | ^^^^^^^^^^^^^^^^^^ + = note: this can be written more efficiently by appending to a `String` directly + +error: use of `format!` to build up a string from an iterator + --> $DIR/format_collect.rs:24:5 | LL | / (1..10) LL | | .map(|s| { @@ -29,16 +47,16 @@ LL | | .collect() | |__________________^ | help: call `fold` instead - --> $DIR/format_collect.rs:20:10 + --> $DIR/format_collect.rs:25:10 | LL | .map(|s| { | ^^^ help: ... and use the `write!` macro here - --> $DIR/format_collect.rs:22:13 + --> $DIR/format_collect.rs:27:13 | LL | format!("{s} {y}") | ^^^^^^^^^^^^^^^^^^ = note: this can be written more efficiently by appending to a `String` directly -error: aborting due to 2 previous errors +error: aborting due to 3 previous errors