New lint: iter_overeager_cloned

CHANGELOG.md

@@ -3049,6 +3049,7 @@ Released 2018-09-13
 [`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator
 [`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth
 [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
+[`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned
 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
 [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero
 [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits

clippy_lints/src/lib.register_all.rs

@@ -156,6 +156,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::ITER_NEXT_SLICE),
     LintId::of(methods::ITER_NTH),
     LintId::of(methods::ITER_NTH_ZERO),
+    LintId::of(methods::ITER_OVEREAGER_CLONED),
     LintId::of(methods::ITER_SKIP_NEXT),
     LintId::of(methods::MANUAL_FILTER_MAP),
     LintId::of(methods::MANUAL_FIND_MAP),

clippy_lints/src/lib.register_lints.rs

@@ -288,6 +288,7 @@ store.register_lints(&[
     methods::ITER_NEXT_SLICE,
     methods::ITER_NTH,
     methods::ITER_NTH_ZERO,
+    methods::ITER_OVEREAGER_CLONED,
     methods::ITER_SKIP_NEXT,
     methods::MANUAL_FILTER_MAP,
     methods::MANUAL_FIND_MAP,

clippy_lints/src/lib.register_perf.rs

@@ -14,6 +14,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(methods::EXPECT_FUN_CALL),
     LintId::of(methods::EXTEND_WITH_DRAIN),
     LintId::of(methods::ITER_NTH),
+    LintId::of(methods::ITER_OVEREAGER_CLONED),
     LintId::of(methods::MANUAL_STR_REPEAT),
     LintId::of(methods::OR_FUN_CALL),
     LintId::of(methods::SINGLE_CHAR_PATTERN),

clippy_lints/src/methods/iter_overeager_cloned.rs

@@ -0,0 +1,62 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet;
+use clippy_utils::ty::{get_iterator_item_ty, is_copy};
+use itertools::Itertools;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use std::ops::Not;
+
+use super::ITER_OVEREAGER_CLONED;
+use crate::redundant_clone::REDUNDANT_CLONE;
+
+/// lint overeager use of `cloned()` for `Iterator`s
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'_>,
+    recv: &'tcx hir::Expr<'_>,
+    name: &str,
+    map_arg: &[hir::Expr<'_>],
+) {
+    // Check if it's iterator and get type associated with `Item`.
+    let inner_ty = match get_iterator_item_ty(cx, cx.typeck_results().expr_ty_adjusted(recv)) {
+        Some(ty) => ty,
+        _ => return,
+    };
+
+    match inner_ty.kind() {
+        ty::Ref(_, ty, _) if !is_copy(cx, ty) => {},
+        _ => return,
+    };
+
+    let (lint, preserve_cloned) = match name {
+        "count" => (REDUNDANT_CLONE, false),
+        _ => (ITER_OVEREAGER_CLONED, true),
+    };
+    let wildcard_params = map_arg.is_empty().not().then(|| "...").unwrap_or_default();
+    let msg = format!(
+        "called `cloned().{}({})` on an `Iterator`. It may be more efficient to call `{}({}){}` instead",
+        name,
+        wildcard_params,
+        name,
+        wildcard_params,
+        preserve_cloned.then(|| ".cloned()").unwrap_or_default(),
+    );
+
+    span_lint_and_sugg(
+        cx,
+        lint,
+        expr.span,
+        &msg,
+        "try this",
+        format!(
+            "{}.{}({}){}",
+            snippet(cx, recv.span, ".."),
+            name,
+            map_arg.iter().map(|a| snippet(cx, a.span, "..")).join(", "),
+            preserve_cloned.then(|| ".cloned()").unwrap_or_default(),
+        ),
+        Applicability::MachineApplicable,
+    );
+}

clippy_lints/src/methods/mod.rs

@@ -30,6 +30,7 @@ mod iter_count;
 mod iter_next_slice;
 mod iter_nth;
 mod iter_nth_zero;
+mod iter_overeager_cloned;
 mod iter_skip_next;
 mod iterator_step_by_zero;
 mod manual_saturating_arithmetic;
@@ -106,6 +107,41 @@ declare_clippy_lint! {
     "used `cloned` where `copied` could be used instead"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `_.cloned().<func>()` where call to `.cloned()` can be postponed.
+    ///
+    /// ### Why is this bad?
+    /// It's often inefficient to clone all elements of an iterator, when eventually, only some
+    /// of them will be consumed.
+    ///
+    /// ### Examples
+    /// ```rust
+    /// # let vec = vec!["string".to_string()];
+    ///
+    /// // Bad
+    /// vec.iter().cloned().take(10);
+    ///
+    /// // Good
+    /// vec.iter().take(10).cloned();
+    ///
+    /// // Bad
+    /// vec.iter().cloned().last();
+    ///
+    /// // Good
+    /// vec.iter().last().cloned();
+    ///
+    /// ```
+    /// ### Known Problems
+    /// This `lint` removes the side of effect of cloning items in the iterator.
+    /// A code that relies on that side-effect could fail.
+    ///
+    #[clippy::version = "1.59.0"]
+    pub ITER_OVEREAGER_CLONED,
+    perf,
+    "using `cloned()` early with `Iterator::iter()` can lead to some performance inefficiencies"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usages of `Iterator::flat_map()` where `filter_map()` could be
@@ -1950,6 +1986,7 @@ impl_lint_pass!(Methods => [
     CLONE_ON_COPY,
     CLONE_ON_REF_PTR,
     CLONE_DOUBLE_REF,
+    ITER_OVEREAGER_CLONED,
     CLONED_INSTEAD_OF_COPIED,
     FLAT_MAP_OPTION,
     INEFFICIENT_TO_STRING,
@@ -2243,9 +2280,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 },
                 _ => {},
             },
-            ("count", []) => match method_call(recv) {
-                Some((name @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => {
-                    iter_count::check(cx, expr, recv2, name);
+            (name @ "count", args @ []) => match method_call(recv) {
+                Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
+                Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => {
+                    iter_count::check(cx, expr, recv2, name2);
                 },
                 Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg),
                 _ => {},
@@ -2266,10 +2304,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 flat_map_identity::check(cx, expr, arg, span);
                 flat_map_option::check(cx, expr, arg, span);
             },
-            ("flatten", []) => {
-                if let Some(("map", [recv, map_arg], _)) = method_call(recv) {
-                    map_flatten::check(cx, expr, recv, map_arg);
-                }
+            (name @ "flatten", args @ []) => match method_call(recv) {
+                Some(("map", [recv, map_arg], _)) => map_flatten::check(cx, expr, recv, map_arg),
+                Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
+                _ => {},
             },
             ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span),
             ("for_each", [_]) => {
@@ -2281,6 +2319,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
             ("is_file", []) => filetype_is_file::check(cx, expr, recv),
             ("is_none", []) => check_is_some_is_none(cx, expr, recv, false),
             ("is_some", []) => check_is_some_is_none(cx, expr, recv, true),
+            ("last", args @ []) | ("skip", args @ [_]) => {
+                if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
+                    if let ("cloned", []) = (name2, args2) {
+                        iter_overeager_cloned::check(cx, expr, recv2, name, args);
+                    }
+                }
+            },
             ("map", [m_arg]) => {
                 if let Some((name, [recv2, args @ ..], span2)) = method_call(recv) {
                     match (name, args) {
@@ -2296,20 +2341,22 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 map_identity::check(cx, expr, recv, m_arg, span);
             },
             ("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map),
-            ("next", []) => {
-                if let Some((name, [recv, args @ ..], _)) = method_call(recv) {
-                    match (name, args) {
-                        ("filter", [arg]) => filter_next::check(cx, expr, recv, arg),
-                        ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv, arg, msrv),
-                        ("iter", []) => iter_next_slice::check(cx, expr, recv),
-                        ("skip", [arg]) => iter_skip_next::check(cx, expr, recv, arg),
+            (name @ "next", args @ []) => {
+                if let Some((name2, [recv2, args2 @ ..], _)) = method_call(recv) {
+                    match (name2, args2) {
+                        ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
+                        ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg),
+                        ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, msrv),
+                        ("iter", []) => iter_next_slice::check(cx, expr, recv2),
+                        ("skip", [arg]) => iter_skip_next::check(cx, expr, recv2, arg),
                         ("skip_while", [_]) => skip_while_next::check(cx, expr),
                         _ => {},
                     }
                 }
             },
-            ("nth", [n_arg]) => match method_call(recv) {
+            ("nth", args @ [n_arg]) => match method_call(recv) {
                 Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg),
+                Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
                 Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false),
                 Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true),
                 _ => iter_nth_zero::check(cx, expr, recv, n_arg),
@@ -2337,6 +2384,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 }
             },
             ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
+            ("take", args @ [_arg]) => {
+                if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
+                    if let ("cloned", []) = (name2, args2) {
+                        iter_overeager_cloned::check(cx, expr, recv2, name, args);
+                    }
+                }
+            },
             ("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => {
                 implicit_clone::check(cx, name, expr, recv);
             },

tests/ui/iter_overeager_cloned.fixed

@@ -0,0 +1,45 @@
+// run-rustfix
+#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)]
+
+fn main() {
+    let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()];
+
+    let _: Option<String> = vec.iter().last().cloned();
+
+    let _: Option<String> = vec.iter().chain(vec.iter()).next().cloned();
+
+    let _: usize = vec.iter().filter(|x| x == &"2").count();
+
+    let _: Vec<_> = vec.iter().take(2).cloned().collect();
+
+    let _: Vec<_> = vec.iter().skip(2).cloned().collect();
+
+    let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned();
+
+    let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
+        .iter().flatten().cloned();
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().map(|x| x.len());
+
+    // This would fail if changed.
+    let _ = vec.iter().cloned().map(|x| x + "2");
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().find(|x| x == "2");
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().all(|x| x.len() == 1);
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().any(|x| x.len() == 1);
+
+    // Should probably stay as it is.
+    let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
+}

tests/ui/iter_overeager_cloned.rs

@@ -0,0 +1,47 @@
+// run-rustfix
+#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)]
+
+fn main() {
+    let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()];
+
+    let _: Option<String> = vec.iter().cloned().last();
+
+    let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next();
+
+    let _: usize = vec.iter().filter(|x| x == &"2").cloned().count();
+
+    let _: Vec<_> = vec.iter().cloned().take(2).collect();
+
+    let _: Vec<_> = vec.iter().cloned().skip(2).collect();
+
+    let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2);
+
+    let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
+        .iter()
+        .cloned()
+        .flatten();
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().map(|x| x.len());
+
+    // This would fail if changed.
+    let _ = vec.iter().cloned().map(|x| x + "2");
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().find(|x| x == "2");
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().all(|x| x.len() == 1);
+
+    // Not implemented yet
+    let _ = vec.iter().cloned().any(|x| x.len() == 1);
+
+    // Should probably stay as it is.
+    let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
+}

tests/ui/iter_overeager_cloned.stderr

@@ -0,0 +1,58 @@
+error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `last().cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:7:29
+   |
+LL |     let _: Option<String> = vec.iter().cloned().last();
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().last().cloned()`
+   |
+   = note: `-D clippy::iter-overeager-cloned` implied by `-D warnings`
+
+error: called `cloned().next()` on an `Iterator`. It may be more efficient to call `next().cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:9:29
+   |
+LL |     let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next();
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().chain(vec.iter()).next().cloned()`
+
+error: called `cloned().count()` on an `Iterator`. It may be more efficient to call `count()` instead
+  --> $DIR/iter_overeager_cloned.rs:11:20
+   |
+LL |     let _: usize = vec.iter().filter(|x| x == &"2").cloned().count();
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").count()`
+   |
+   = note: `-D clippy::redundant-clone` implied by `-D warnings`
+
+error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call `take(...).cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:13:21
+   |
+LL |     let _: Vec<_> = vec.iter().cloned().take(2).collect();
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().take(2).cloned()`
+
+error: called `cloned().skip(...)` on an `Iterator`. It may be more efficient to call `skip(...).cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:15:21
+   |
+LL |     let _: Vec<_> = vec.iter().cloned().skip(2).collect();
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().skip(2).cloned()`
+
+error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call `nth(...).cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:17:13
+   |
+LL |     let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()`
+
+error: called `cloned().flatten()` on an `Iterator`. It may be more efficient to call `flatten().cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:19:13
+   |
+LL |       let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
+   |  _____________^
+LL | |         .iter()
+LL | |         .cloned()
+LL | |         .flatten();
+   | |__________________^
+   |
+help: try this
+   |
+LL ~     let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
+LL ~         .iter().flatten().cloned();
+   |
+
+error: aborting due to 7 previous errors
+