Implement a lint for .clone().into_iter()

Author: GnomedDev

CHANGELOG.md

@@ -6056,6 +6056,7 @@ Released 2018-09-13
 [`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns
 [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
 [`unnecessary_clippy_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_clippy_cfg
+[`unnecessary_collection_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_collection_clone
 [`unnecessary_fallible_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
 [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
 [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
 You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.

book/src/README.md

@@ -6,7 +6,7 @@
 A collection of lints to catch common mistakes and improve your
 [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint
 level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how

clippy_lints/src/declared_lints.rs

@@ -468,6 +468,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::methods::TYPE_ID_ON_BOX_INFO,
     crate::methods::UNINIT_ASSUMED_INIT_INFO,
     crate::methods::UNIT_HASH_INFO,
+    crate::methods::UNNECESSARY_COLLECTION_CLONE_INFO,
     crate::methods::UNNECESSARY_FALLIBLE_CONVERSIONS_INFO,
     crate::methods::UNNECESSARY_FILTER_MAP_INFO,
     crate::methods::UNNECESSARY_FIND_MAP_INFO,

clippy_lints/src/methods/mod.rs

@@ -109,6 +109,7 @@ mod suspicious_to_owned;
 mod type_id_on_box;
 mod uninit_assumed_init;
 mod unit_hash;
+mod unnecessary_collection_clone;
 mod unnecessary_fallible_conversions;
 mod unnecessary_filter_map;
 mod unnecessary_first_then_check;
@@ -4166,6 +4167,36 @@ declare_clippy_lint! {
     "calling `.first().is_some()` or `.first().is_none()` instead of `.is_empty()`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Detects when an entire collection is being cloned eagerly, instead of each item lazily.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// Cloning a collection requires allocating space for all elements and cloning each element into this new space,
+    /// whereas using `Iterator::cloned` does not allocate any more space and only requires cloning each element as they are consumed.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// fn process_string(val: String) -> String { val }
+    /// fn process_strings(strings: &Vec<String>) -> Vec<String> {
+    ///     strings.clone().into_iter().filter(|s| s.len() < 10).map(process_string).collect()
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn process_string(val: String) -> String { val }
+    /// fn process_strings(strings: &Vec<String>) -> Vec<String> {
+    ///     strings.iter().cloned().filter(|s| s.len() < 10).map(process_string).collect()
+    /// }
+    /// ```
+    #[clippy::version = "1.84.0"]
+    pub UNNECESSARY_COLLECTION_CLONE,
+    perf,
+    "calling `.clone().into_iter()` instead of `.iter().cloned()`"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4327,6 +4358,7 @@ impl_lint_pass!(Methods => [
     NEEDLESS_CHARACTER_ITERATION,
     MANUAL_INSPECT,
     UNNECESSARY_MIN_OR_MAX,
+    UNNECESSARY_COLLECTION_CLONE,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4774,6 +4806,10 @@ impl Methods {
                 ("is_none", []) => check_is_some_is_none(cx, expr, recv, call_span, false),
                 ("is_some", []) => check_is_some_is_none(cx, expr, recv, call_span, true),
                 ("iter" | "iter_mut" | "into_iter", []) => {
+                    if name == "into_iter" {
+                        unnecessary_collection_clone::check(cx, expr, recv);
+                    }
+
                     iter_on_single_or_empty_collections::check(cx, expr, name, recv);
                 },
                 ("join", [join_arg]) => {

clippy_lints/src/methods/unnecessary_collection_clone.rs

@@ -0,0 +1,59 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_path_mutable;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::{deref_chain, get_inherent_method, implements_trait, make_normalized_projection};
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_lint::LateContext;
+use rustc_middle::ty::{self, Ty};
+use rustc_span::sym;
+
+use super::{UNNECESSARY_COLLECTION_CLONE, method_call};
+
+// FIXME: This does not check if the iter method is actually compatible with the replacement, but
+// you have to be actively evil to have an `IntoIterator` impl that returns one type and an `iter`
+// method that returns something other than references of that type.... and it is a massive
+// complicated hassle to check this
+fn has_iter_method<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    deref_chain(cx, ty).any(|ty| match ty.kind() {
+        ty::Adt(adt_def, _) => get_inherent_method(cx, adt_def.did(), sym::iter).is_some(),
+        ty::Slice(_) => true,
+        _ => false,
+    })
+}
+
+/// Check for `x.clone().into_iter()` to suggest `x.iter().cloned()`.
+//             ^^^^^^^^^ is recv
+//             ^^^^^^^^^^^^^^^^^^^^^ is expr
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>) {
+    let typeck_results = cx.typeck_results();
+    let diagnostic_items = cx.tcx.all_diagnostic_items(());
+
+    // If the call before `into_iter` is `.clone()`
+    if let Some(("clone", collection_expr, [], _, _)) = method_call(recv)
+        // and the binding being cloned is not mutable
+        && let Some(false) = is_path_mutable(cx, collection_expr)
+        // and the result of `into_iter` is an Iterator
+        && let Some(&iterator_def_id) = diagnostic_items.name_to_id.get(&sym::Iterator)
+        && let expr_ty = typeck_results.expr_ty(expr)
+        && implements_trait(cx, expr_ty, iterator_def_id, &[])
+        // with an item that implements clone
+        && let Some(&clone_def_id) = diagnostic_items.name_to_id.get(&sym::Clone)
+        && let Some(item_ty) = make_normalized_projection(cx.tcx, cx.param_env, iterator_def_id, sym::Item, [expr_ty])
+        && implements_trait(cx, item_ty, clone_def_id, &[])
+        // and the type has an `iter` method
+        && has_iter_method(cx, typeck_results.expr_ty(collection_expr))
+    {
+        let mut applicability = Applicability::MachineApplicable;
+        let collection_expr_snippet = snippet_with_applicability(cx, collection_expr.span, "...", &mut applicability);
+        span_lint_and_sugg(
+            cx,
+            UNNECESSARY_COLLECTION_CLONE,
+            expr.span,
+            "using clone on collection to own iterated items",
+            "replace with",
+            format!("{collection_expr_snippet}.iter().cloned()"),
+            applicability,
+        );
+    }
+}

clippy_lints/src/methods/unnecessary_iter_cloned.rs

@@ -4,11 +4,11 @@ use clippy_utils::higher::ForLoop;
 use clippy_utils::source::SpanRangeExt;
 use clippy_utils::ty::{get_iterator_item_ty, implements_trait};
 use clippy_utils::visitors::for_each_expr_without_closures;
-use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local};
+use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, is_path_mutable};
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind};
+use rustc_hir::{Expr, ExprKind};
 use rustc_lint::LateContext;
 use rustc_span::{Symbol, sym};
 
@@ -42,22 +42,7 @@ pub fn check_for_loop_iter(
         && !clone_or_copy_needed
         && let Some(receiver_snippet) = receiver.span.get_source_text(cx)
     {
-        // Issue 12098
-        // https://github.com/rust-lang/rust-clippy/issues/12098
-        // if the assignee have `mut borrow` conflict with the iteratee
-        // the lint should not execute, former didn't consider the mut case
-
         // check whether `expr` is mutable
-        fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-            if let Some(hir_id) = path_to_local(expr)
-                && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
-            {
-                matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
-            } else {
-                true
-            }
-        }
-
         fn is_caller_or_fields_change(cx: &LateContext<'_>, body: &Expr<'_>, caller: &Expr<'_>) -> bool {
             let mut change = false;
             if let ExprKind::Block(block, ..) = body.kind {
@@ -82,7 +67,12 @@ pub fn check_for_loop_iter(
             while let ExprKind::MethodCall(_, caller, _, _) = child.kind {
                 child = caller;
             }
-            if is_mutable(cx, child) && is_caller_or_fields_change(cx, body, child) {
+
+            // Issue 12098
+            // https://github.com/rust-lang/rust-clippy/issues/12098
+            // if the assignee have `mut borrow` conflict with the iteratee
+            // the lint should not execute, former didn't consider the mut case
+            if is_path_mutable(cx, child).unwrap_or(true) && is_caller_or_fields_change(cx, body, child) {
                 // skip lint
                 return true;
             }

clippy_lints/src/unnecessary_struct_initialization.rs

@@ -1,8 +1,8 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::is_copy;
-use clippy_utils::{get_parent_expr, path_to_local};
-use rustc_hir::{BindingMode, Expr, ExprField, ExprKind, Node, PatKind, Path, QPath, UnOp};
+use clippy_utils::{get_parent_expr, is_path_mutable, path_to_local};
+use rustc_hir::{Expr, ExprField, ExprKind, Path, QPath, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 
@@ -155,16 +155,6 @@ fn same_path_in_all_fields<'tcx>(
     }
 }
 
-fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    if let Some(hir_id) = path_to_local(expr)
-        && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
-    {
-        matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
-    } else {
-        true
-    }
-}
-
 fn check_references(cx: &LateContext<'_>, expr_a: &Expr<'_>, expr_b: &Expr<'_>) -> bool {
     if let Some(parent) = get_parent_expr(cx, expr_a)
         && let parent_ty = cx.typeck_results().expr_ty_adjusted(parent)
@@ -176,7 +166,7 @@ fn check_references(cx: &LateContext<'_>, expr_a: &Expr<'_>, expr_b: &Expr<'_>)
             return false;
         }
 
-        if parent_ty.is_mutable_ptr() && !is_mutable(cx, expr_b) {
+        if parent_ty.is_mutable_ptr() && !is_path_mutable(cx, expr_b).unwrap_or(true) {
             // The original can be used in a mutable reference context only if it is mutable.
             return false;
         }

clippy_utils/src/lib.rs

@@ -412,6 +412,19 @@ pub fn is_trait_item(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) -
     }
 }
 
+/// Checks if `expr` is a path to a mutable binding.
+pub fn is_path_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<bool> {
+    if let Some(hir_id) = path_to_local(expr)
+        && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
+    {
+        Some(matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..)))
+    } else if let ExprKind::Field(recv, _) = expr.kind {
+        is_path_mutable(cx, recv)
+    } else {
+        None
+    }
+}
+
 pub fn last_path_segment<'tcx>(path: &QPath<'tcx>) -> &'tcx PathSegment<'tcx> {
     match *path {
         QPath::Resolved(_, path) => path.segments.last().expect("A path must have at least one segment"),

clippy_utils/src/ty.rs

@@ -1323,22 +1323,29 @@ pub fn deref_chain<'cx, 'tcx>(cx: &'cx LateContext<'tcx>, ty: Ty<'tcx>) -> impl
 
 /// Checks if a Ty<'_> has some inherent method Symbol.
 ///
-/// This does not look for impls in the type's `Deref::Target` type.
-/// If you need this, you should wrap this call in `clippy_utils::ty::deref_chain().any(...)`.
+/// This is a helper for [`get_inherent_method`].
 pub fn get_adt_inherent_method<'a>(cx: &'a LateContext<'_>, ty: Ty<'_>, method_name: Symbol) -> Option<&'a AssocItem> {
     if let Some(ty_did) = ty.ty_adt_def().map(AdtDef::did) {
-        cx.tcx.inherent_impls(ty_did).iter().find_map(|&did| {
-            cx.tcx
-                .associated_items(did)
-                .filter_by_name_unhygienic(method_name)
-                .next()
-                .filter(|item| item.kind == AssocKind::Fn)
-        })
+        get_inherent_method(cx, ty_did, method_name)
     } else {
         None
     }
 }
 
+/// Checks if the [`DefId`] of a Ty has some inherent method Symbol.
+///
+/// This does not look for impls in the type's `Deref::Target` type.
+/// If you need this, you should wrap this call in `clippy_utils::ty::deref_chain().any(...)`.
+pub fn get_inherent_method<'a>(cx: &'a LateContext<'_>, ty_did: DefId, method_name: Symbol) -> Option<&'a AssocItem> {
+    cx.tcx.inherent_impls(ty_did).iter().find_map(|&did| {
+        cx.tcx
+            .associated_items(did)
+            .filter_by_name_unhygienic(method_name)
+            .next()
+            .filter(|item| item.kind == AssocKind::Fn)
+    })
+}
+
 /// Get's the type of a field by name.
 pub fn get_field_by_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, name: Symbol) -> Option<Ty<'tcx>> {
     match *ty.kind() {

tests/ui/filter_map_bool_then.fixed

@@ -4,6 +4,7 @@
     clippy::map_identity,
     clippy::unnecessary_lazy_evaluations,
     clippy::unnecessary_filter_map,
+    clippy::unnecessary_collection_clone,
     unused
 )]
 #![warn(clippy::filter_map_bool_then)]

tests/ui/filter_map_bool_then.rs

@@ -4,6 +4,7 @@
     clippy::map_identity,
     clippy::unnecessary_lazy_evaluations,
     clippy::unnecessary_filter_map,
+    clippy::unnecessary_collection_clone,
     unused
 )]
 #![warn(clippy::filter_map_bool_then)]

tests/ui/filter_map_bool_then.stderr

@@ -1,5 +1,5 @@
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:19:22
+  --> tests/ui/filter_map_bool_then.rs:20:22
    |
 LL |     v.clone().iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
@@ -8,55 +8,55 @@ LL |     v.clone().iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
    = help: to override `-D warnings` add `#[allow(clippy::filter_map_bool_then)]`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:20:27
+  --> tests/ui/filter_map_bool_then.rs:21:27
    |
 LL |     v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:23:10
+  --> tests/ui/filter_map_bool_then.rs:24:10
    |
 LL |         .filter_map(|i| -> Option<_> { (i % 2 == 0).then(|| i + 1) });
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:27:10
+  --> tests/ui/filter_map_bool_then.rs:28:10
    |
 LL |         .filter_map(|i| (i % 2 == 0).then(|| i + 1));
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:31:10
+  --> tests/ui/filter_map_bool_then.rs:32:10
    |
 LL |         .filter_map(|i| (i.clone() % 2 == 0).then(|| i + 1));
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i.clone() % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:37:22
+  --> tests/ui/filter_map_bool_then.rs:38:22
    |
 LL |     v.clone().iter().filter_map(|i| (i == &NonCopy).then(|| i));
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i == &NonCopy)).map(|i| i)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:61:50
+  --> tests/ui/filter_map_bool_then.rs:62:50
    |
 LL |     let _: Vec<usize> = bools.iter().enumerate().filter_map(|(i, b)| b.then(|| i)).collect();
    |                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&(i, b)| *b).map(|(i, b)| i)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:65:50
+  --> tests/ui/filter_map_bool_then.rs:66:50
    |
 LL |     let _: Vec<usize> = bools.iter().enumerate().filter_map(|(i, b)| b.then(|| i)).collect();
    |                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&(i, b)| ***b).map(|(i, b)| i)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:69:50
+  --> tests/ui/filter_map_bool_then.rs:70:50
    |
 LL |     let _: Vec<usize> = bools.iter().enumerate().filter_map(|(i, b)| b.then(|| i)).collect();
    |                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&(i, b)| **b).map(|(i, b)| i)`
 
 error: usage of `bool::then` in `filter_map`
-  --> tests/ui/filter_map_bool_then.rs:80:50
+  --> tests/ui/filter_map_bool_then.rs:81:50
    |
 LL |     let _: Vec<usize> = bools.iter().enumerate().filter_map(|(i, b)| b.then(|| i)).collect();
    |                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&(i, b)| ****b).map(|(i, b)| i)`