From 84f89f30ebda5485badac09b34d666fbab243a19 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sat, 13 May 2023 11:47:05 +0200 Subject: [PATCH] enhance `needless_collect` Updates `needless_collect` to lint for collecting into a method or function argument thats taking an `IntoIterator` (for example `extend`). Every `Iterator` trivially implements `IntoIterator` and colleting it only causes an unnecessary allocation. --- clippy_lints/src/methods/needless_collect.rs | 68 +++++++++++++++++++- tests/ui/needless_collect.fixed | 12 ++++ tests/ui/needless_collect.rs | 12 ++++ tests/ui/needless_collect.stderr | 26 +++++++- 4 files changed, 115 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs index 0b0c6adc504..6841aaf626c 100644 --- a/clippy_lints/src/methods/needless_collect.rs +++ b/clippy_lints/src/methods/needless_collect.rs @@ -1,6 +1,5 @@ use super::NEEDLESS_COLLECT; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; -use clippy_utils::higher; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{is_type_diagnostic_item, make_normalized_projection, make_projection}; @@ -8,6 +7,7 @@ use clippy_utils::{ can_move_expr_to_closure, get_enclosing_block, get_parent_node, is_trait_method, path_to_local, path_to_local_id, CaptureKind, }; +use clippy_utils::{fn_def_id, higher}; use rustc_data_structures::fx::FxHashMap; use rustc_errors::{Applicability, MultiSpan}; use rustc_hir::intravisit::{walk_block, walk_expr, Visitor}; @@ -16,7 +16,7 @@ use rustc_hir::{ }; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; -use rustc_middle::ty::{self, AssocKind, EarlyBinder, GenericArg, GenericArgKind, Ty}; +use rustc_middle::ty::{self, AssocKind, Clause, EarlyBinder, GenericArg, GenericArgKind, PredicateKind, Ty}; use rustc_span::symbol::Ident; use rustc_span::{sym, Span, Symbol}; @@ -32,6 +32,8 @@ pub(super) fn check<'tcx>( if let Some(parent) = get_parent_node(cx.tcx, collect_expr.hir_id) { match parent { Node::Expr(parent) => { + check_collect_into_intoiterator(cx, parent, collect_expr, call_span, iter_expr); + if let ExprKind::MethodCall(name, _, args @ ([] | [_]), _) = parent.kind { let mut app = Applicability::MachineApplicable; let name = name.ident.as_str(); @@ -134,6 +136,68 @@ pub(super) fn check<'tcx>( } } +/// checks for for collecting into a (generic) method or function argument +/// taking an `IntoIterator` +fn check_collect_into_intoiterator<'tcx>( + cx: &LateContext<'tcx>, + parent: &'tcx Expr<'tcx>, + collect_expr: &'tcx Expr<'tcx>, + call_span: Span, + iter_expr: &'tcx Expr<'tcx>, +) { + if let Some(id) = fn_def_id(cx, parent) { + let args = match parent.kind { + ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => args, + _ => &[], + }; + // find the argument index of the `collect_expr` in the + // function / method call + if let Some(arg_idx) = args.iter().position(|e| e.hir_id == collect_expr.hir_id).map(|i| { + if matches!(parent.kind, ExprKind::MethodCall(_, _, _, _)) { + i + 1 + } else { + i + } + }) { + // extract the input types of the function/method call + // that contains `collect_expr` + let inputs = cx + .tcx + .liberate_late_bound_regions(id, cx.tcx.fn_sig(id).subst_identity()) + .inputs(); + + // map IntoIterator generic bounds to their signature + // types and check whether the argument type is an + // `IntoIterator` + if cx + .tcx + .param_env(id) + .caller_bounds() + .into_iter() + .filter_map(|p| { + if let PredicateKind::Clause(Clause::Trait(t)) = p.kind().skip_binder() + && cx.tcx.is_diagnostic_item(sym::IntoIterator,t.trait_ref.def_id) { + Some(t.self_ty()) + } else { + None + } + }) + .any(|ty| ty == inputs[arg_idx]) + { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + call_span.with_lo(iter_expr.span.hi()), + NEEDLESS_COLLECT_MSG, + "remove this call", + String::new(), + Applicability::MachineApplicable, + ); + } + } + } +} + /// Checks if the given method call matches the expected signature of `([&[mut]] self) -> bool` fn is_is_empty_sig(cx: &LateContext<'_>, call_id: HirId) -> bool { cx.typeck_results().type_dependent_def_id(call_id).map_or(false, |id| { diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index 024c22de225..b7e80af5015 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -62,4 +62,16 @@ fn main() { let _ = sample.iter().next().is_none(); let _ = sample.iter().any(|x| x == &0); + + #[allow(clippy::double_parens)] + { + Vec::::new().extend((0..10)); + foo((0..10)); + bar((0..10).collect::>(), (0..10)); + baz((0..10), (), ('a'..='z')) + } } + +fn foo(_: impl IntoIterator) {} +fn bar>(_: Vec, _: I) {} +fn baz>(_: I, _: (), _: impl IntoIterator) {} diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 7ed7babec30..680b6fa5b55 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -62,4 +62,16 @@ fn main() { let _ = sample.iter().collect::>().is_empty(); let _ = sample.iter().collect::>().contains(&&0); + + #[allow(clippy::double_parens)] + { + Vec::::new().extend((0..10).collect::>()); + foo((0..10).collect::>()); + bar((0..10).collect::>(), (0..10).collect::>()); + baz((0..10), (), ('a'..='z').collect::>()) + } } + +fn foo(_: impl IntoIterator) {} +fn bar>(_: Vec, _: I) {} +fn baz>(_: I, _: (), _: impl IntoIterator) {} diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr index 584d2a1d835..ad22a7b057e 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -90,5 +90,29 @@ error: avoid using `collect()` when not needed LL | let _ = sample.iter().collect::>().contains(&&0); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `any(|x| x == &0)` -error: aborting due to 15 previous errors +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:68:40 + | +LL | Vec::::new().extend((0..10).collect::>()); + | ^^^^^^^^^^^^^^^^^^^^ help: remove this call + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:69:20 + | +LL | foo((0..10).collect::>()); + | ^^^^^^^^^^^^^^^^^^^^ help: remove this call + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:70:49 + | +LL | bar((0..10).collect::>(), (0..10).collect::>()); + | ^^^^^^^^^^^^^^^^^^^^ help: remove this call + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:71:37 + | +LL | baz((0..10), (), ('a'..='z').collect::>()) + | ^^^^^^^^^^^^^^^^^^^^ help: remove this call + +error: aborting due to 19 previous errors