From c3881569afd049d391d6943bff9ee7317fe98211 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Fri, 7 Jul 2023 00:45:37 +0200 Subject: [PATCH] new lint: `format_collect` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/format_collect.rs | 33 ++++++++++++++++ clippy_lints/src/methods/mod.rs | 35 ++++++++++++++++- src/main.rs | 3 +- tests/ui/format_collect.rs | 26 +++++++++++++ tests/ui/format_collect.stderr | 44 ++++++++++++++++++++++ 7 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/methods/format_collect.rs create mode 100644 tests/ui/format_collect.rs create mode 100644 tests/ui/format_collect.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index b3b6e3b865f..ea8450ed5c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4865,6 +4865,7 @@ Released 2018-09-13 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy [`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_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_collect [`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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 498d657b31f..7472158a81c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -338,6 +338,7 @@ crate::methods::FILTER_NEXT_INFO, crate::methods::FLAT_MAP_IDENTITY_INFO, crate::methods::FLAT_MAP_OPTION_INFO, + crate::methods::FORMAT_COLLECT_INFO, crate::methods::FROM_ITER_INSTEAD_OF_COLLECT_INFO, crate::methods::GET_FIRST_INFO, crate::methods::GET_LAST_WITH_LEN_INFO, diff --git a/clippy_lints/src/methods/format_collect.rs b/clippy_lints/src/methods/format_collect.rs new file mode 100644 index 00000000000..74eb80782d6 --- /dev/null +++ b/clippy_lints/src/methods/format_collect.rs @@ -0,0 +1,33 @@ +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::ty::is_type_lang_item; +use rustc_hir::Expr; +use rustc_hir::ExprKind; +use rustc_hir::LangItem; +use rustc_lint::LateContext; +use rustc_span::Span; + +fn tail_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + match expr.kind { + ExprKind::Block(block, _) if !expr.span.from_expansion() => block.expr, + _ => Some(expr), + } +} + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, map_arg: &Expr<'_>, map_span: Span) { + 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(mac) = root_macro_call_first_node(cx, value) + && is_format_macro(cx, mac.def_id) + { + span_lint_and_then(cx, FORMAT_COLLECT, expr.span, "use of `format!` to build up a string from an iterator", |diag| { + diag.span_help(map_span, "call `fold` instead") + .span_help(value.span.source_callsite(), "... and use the `write!` macro here") + .note("this can be written more efficiently by appending to a `String` directly"); + }); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 2747ab5615c..f55a7727fad 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -26,6 +26,7 @@ mod filter_next; mod flat_map_identity; mod flat_map_option; +mod format_collect; mod from_iter_instead_of_collect; mod get_first; mod get_last_with_len; @@ -3378,6 +3379,36 @@ "calling `Stdin::read_line`, then trying to parse it without first trimming" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `.map(|_| format!(..)).collect::()`. + /// + /// ### 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`. + /// + /// ### Example + /// ```rust + /// fn hex_encode(bytes: &[u8]) -> String { + /// bytes.iter().map(|b| format!("{b:02X}")).collect() + /// } + /// ``` + /// Use instead: + /// ```rust + /// use std::fmt::Write; + /// fn hex_encode(bytes: &[u8]) -> String { + /// bytes.iter().fold(String::new(), |mut output, b| { + /// let _ = write!(output, "{b:02X}"); + /// output + /// }) + /// } + /// ``` + #[clippy::version = "1.72.0"] + pub FORMAT_COLLECT, + perf, + "`format!`ing every element in a collection, then collecting the strings into a new `String`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3512,6 +3543,7 @@ pub fn new( UNNECESSARY_LITERAL_UNWRAP, DRAIN_COLLECT, MANUAL_TRY_FOLD, + FORMAT_COLLECT, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3733,8 +3765,9 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { Some((name @ ("cloned" | "copied"), recv2, [], _, _)) => { iter_cloned_collect::check(cx, name, expr, recv2); }, - Some(("map", m_recv, [m_arg], _, _)) => { + Some(("map", m_recv, [m_arg], m_ident_span, _)) => { map_collect_result_unit::check(cx, expr, m_recv, m_arg); + format_collect::check(cx, expr, m_arg, m_ident_span); }, Some(("take", take_self_arg, [take_arg], _, _)) => { if self.msrv.meets(msrvs::STR_REPEAT) { diff --git a/src/main.rs b/src/main.rs index cdc85cb33ca..26b655076cf 100644 --- a/src/main.rs +++ b/src/main.rs @@ -132,8 +132,7 @@ fn into_std_cmd(self) -> Command { let clippy_args: String = self .clippy_args .iter() - .map(|arg| format!("{arg}__CLIPPY_HACKERY__")) - .collect(); + .fold(String::new(), |s, arg| s + arg + "__CLIPPY_HACKERY__"); // Currently, `CLIPPY_TERMINAL_WIDTH` is used only to format "unknown field" error messages. let terminal_width = termize::dimensions().map_or(0, |(w, _)| w); diff --git a/tests/ui/format_collect.rs b/tests/ui/format_collect.rs new file mode 100644 index 00000000000..ecadb43ecb4 --- /dev/null +++ b/tests/ui/format_collect.rs @@ -0,0 +1,26 @@ +#![allow(unused, dead_code)] +#![warn(clippy::format_collect)] + +fn hex_encode(bytes: &[u8]) -> String { + bytes.iter().map(|b| format!("{b:02X}")).collect() +} + +macro_rules! fmt { + ($x:ident) => { + format!("{x:02X}", x = $x) + }; +} + +fn from_macro(bytes: &[u8]) -> String { + bytes.iter().map(|x| fmt!(x)).collect() +} + +fn with_block() -> String { + (1..10) + .map(|s| { + let y = 1; + format!("{s} {y}") + }) + .collect() +} +fn main() {} diff --git a/tests/ui/format_collect.stderr b/tests/ui/format_collect.stderr new file mode 100644 index 00000000000..cd71a853762 --- /dev/null +++ b/tests/ui/format_collect.stderr @@ -0,0 +1,44 @@ +error: use of `format!` to build up a string from an iterator + --> $DIR/format_collect.rs:5:5 + | +LL | bytes.iter().map(|b| format!("{b:02X}")).collect() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: call `fold` instead + --> $DIR/format_collect.rs:5:18 + | +LL | bytes.iter().map(|b| format!("{b:02X}")).collect() + | ^^^ +help: ... and use the `write!` macro here + --> $DIR/format_collect.rs:5:26 + | +LL | bytes.iter().map(|b| format!("{b:02X}")).collect() + | ^^^^^^^^^^^^^^^^^^ + = note: this can be written more efficiently by appending to a `String` directly + = 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 + | +LL | / (1..10) +LL | | .map(|s| { +LL | | let y = 1; +LL | | format!("{s} {y}") +LL | | }) +LL | | .collect() + | |__________________^ + | +help: call `fold` instead + --> $DIR/format_collect.rs:20:10 + | +LL | .map(|s| { + | ^^^ +help: ... and use the `write!` macro here + --> $DIR/format_collect.rs:22: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 +