Auto merge of rust-lang#10777 - Icxolu:unnecessary_collect, r=xFrednet

bors · bors · commit 2a1dbbaec574 · 2023-05-19T12:26:43.000Z
Enhance `needless_collect`: lint in method/function arguments that take an `IntoIterator` Updates `needless_collect` to also lint `collect` calls in method/function arguments that take an `IntoIterator` (for example `Extend::extend`). Every `Iterator` trivially implements `IntoIterator` and collecting it only causes an unnecessary allocation. --- changelog: Enhancement: [`needless_collect`]: Now also detects function arguments, taking a generic `IntoIterator` [rust-lang#10777](rust-lang/rust-clippy#10777)  fixes rust-lang#10762
diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs
@@ -1,13 +1,13 @@
 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};
 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
@@ -62,4 +62,16 @@ fn main() {
 
     let _ = sample.iter().next().is_none();
     let _ = sample.iter().any(|x| x == &0);
+
+    #[allow(clippy::double_parens)]
+    {
+        Vec::<u8>::new().extend((0..10));
+        foo((0..10));
+        bar((0..10).collect::<Vec<_>>(), (0..10));
+        baz((0..10), (), ('a'..='z'))
+    }
 }
+
+fn foo(_: impl IntoIterator<Item = usize>) {}
+fn bar<I: IntoIterator<Item = usize>>(_: Vec<usize>, _: I) {}
+fn baz<I: IntoIterator<Item = usize>>(_: I, _: (), _: impl IntoIterator<Item = char>) {}
diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs
@@ -62,4 +62,16 @@ fn main() {
 
     let _ = sample.iter().collect::<VecWrapper<_>>().is_empty();
     let _ = sample.iter().collect::<VecWrapper<_>>().contains(&&0);
+
+    #[allow(clippy::double_parens)]
+    {
+        Vec::<u8>::new().extend((0..10).collect::<Vec<_>>());
+        foo((0..10).collect::<Vec<_>>());
+        bar((0..10).collect::<Vec<_>>(), (0..10).collect::<Vec<_>>());
+        baz((0..10), (), ('a'..='z').collect::<Vec<_>>())
+    }
 }
+
+fn foo(_: impl IntoIterator<Item = usize>) {}
+fn bar<I: IntoIterator<Item = usize>>(_: Vec<usize>, _: I) {}
+fn baz<I: IntoIterator<Item = usize>>(_: I, _: (), _: impl IntoIterator<Item = char>) {}
diff --git 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::<VecWrapper<_>>().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::<u8>::new().extend((0..10).collect::<Vec<_>>());
+   |                                        ^^^^^^^^^^^^^^^^^^^^ help: remove this call
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:69:20
+   |
+LL |         foo((0..10).collect::<Vec<_>>());
+   |                    ^^^^^^^^^^^^^^^^^^^^ help: remove this call
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:70:49
+   |
+LL |         bar((0..10).collect::<Vec<_>>(), (0..10).collect::<Vec<_>>());
+   |                                                 ^^^^^^^^^^^^^^^^^^^^ help: remove this call
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:71:37
+   |
+LL |         baz((0..10), (), ('a'..='z').collect::<Vec<_>>())
+   |                                     ^^^^^^^^^^^^^^^^^^^^ help: remove this call
+
+error: aborting due to 19 previous errors
 
