Changes to iter_overeager_cloned

Jarcho · Jarcho · commit bf3ab592f04f · 2022-06-07T00:40:32.000-04:00
* Don't lint on `.cloned().flatten()` when `T::Item` doesn't implement `IntoIterator`
* Reduce verbosity of lint message
* Narrow down the scope of the replacement range
diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs
@@ -1,70 +1,59 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::snippet;
-use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy};
-use itertools::Itertools;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::source::snippet_opt;
+use clippy_utils::ty::{get_associated_type, implements_trait, is_copy};
 use rustc_errors::Applicability;
-use rustc_hir as hir;
+use rustc_hir::Expr;
 use rustc_lint::LateContext;
 use rustc_middle::ty;
 use rustc_span::sym;
-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<'_>],
+    expr: &'tcx Expr<'_>,
+    cloned_call: &'tcx Expr<'_>,
+    cloned_recv: &'tcx Expr<'_>,
+    is_count: bool,
+    needs_into_iter: bool,
 ) {
-    // Check if it's iterator and get type associated with `Item`.
-    let inner_ty = if_chain! {
-        if let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
-        let recv_ty = cx.typeck_results().expr_ty(recv);
-        if implements_trait(cx, recv_ty, iterator_trait_id, &[]);
-        if let Some(inner_ty) = get_iterator_item_ty(cx, cx.typeck_results().expr_ty_adjusted(recv));
-        then {
-            inner_ty
-        } else {
+    let typeck = cx.typeck_results();
+    if let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
+        && let Some(method_id) = typeck.type_dependent_def_id(expr.hir_id)
+        && cx.tcx.trait_of_item(method_id) == Some(iter_id)
+        && let Some(method_id) = typeck.type_dependent_def_id(cloned_call.hir_id)
+        && cx.tcx.trait_of_item(method_id) == Some(iter_id)
+        && let cloned_recv_ty = typeck.expr_ty_adjusted(cloned_recv)
+        && let Some(iter_assoc_ty) = get_associated_type(cx, cloned_recv_ty, iter_id, "Item")
+        && matches!(*iter_assoc_ty.kind(), ty::Ref(_, ty, _) if !is_copy(cx, ty))
+    {
+        if needs_into_iter
+            && let Some(into_iter_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
+            && !implements_trait(cx, iter_assoc_ty, into_iter_id, &[])
+        {
             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(),
-    );
+        let (lint, msg, trailing_clone) = if is_count {
+            (REDUNDANT_CLONE, "unneeded cloning of iterator items", "")
+        } else {
+            (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()")
+        };
 
-    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,
-    );
+        span_lint_and_then(
+            cx,
+            lint,
+            expr.span,
+            msg,
+            |diag| {
+                let method_span = expr.span.with_lo(cloned_call.span.hi());
+                if let Some(mut snip) = snippet_opt(cx, method_span) {
+                    snip.push_str(trailing_clone);
+                    let replace_span = expr.span.with_lo(cloned_recv.span.hi());
+                    diag.span_suggestion(replace_span, "try this", snip, Applicability::MachineApplicable);
+                }
+            }
+        );
+    }
 }
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -2583,8 +2583,8 @@ impl Methods {
                     },
                     _ => {},
                 },
-                (name @ "count", args @ []) => match method_call(recv) {
-                    Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
+                ("count", []) => match method_call(recv) {
+                    Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false),
                     Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => {
                         iter_count::check(cx, expr, recv2, name2);
                     },
@@ -2614,9 +2614,9 @@ impl Methods {
                     flat_map_identity::check(cx, expr, arg, span);
                     flat_map_option::check(cx, expr, arg, span);
                 },
-                (name @ "flatten", args @ []) => match method_call(recv) {
+                ("flatten", []) => match method_call(recv) {
                     Some(("map", [recv, map_arg], map_span)) => map_flatten::check(cx, expr, recv, map_arg, map_span),
-                    Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
+                    Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true),
                     _ => {},
                 },
                 ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span),
@@ -2636,10 +2636,10 @@ impl Methods {
                         unnecessary_join::check(cx, expr, recv, join_arg, span);
                     }
                 },
-                ("last", args @ []) | ("skip", args @ [_]) => {
+                ("last", []) | ("skip", [_]) => {
                     if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
                         if let ("cloned", []) = (name2, args2) {
-                            iter_overeager_cloned::check(cx, expr, recv2, name, args);
+                            iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
                         }
                     }
                 },
@@ -2660,10 +2660,10 @@ impl Methods {
                     map_identity::check(cx, expr, recv, m_arg, name, span);
                 },
                 ("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map),
-                (name @ "next", args @ []) => {
+                ("next", []) => {
                     if let Some((name2, [recv2, args2 @ ..], _)) = method_call(recv) {
                         match (name2, args2) {
-                            ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
+                            ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false),
                             ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg),
                             ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, self.msrv),
                             ("iter", []) => iter_next_slice::check(cx, expr, recv2),
@@ -2673,9 +2673,9 @@ impl Methods {
                         }
                     }
                 },
-                ("nth", args @ [n_arg]) => match method_call(recv) {
+                ("nth", [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(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false),
                     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),
@@ -2698,10 +2698,10 @@ impl Methods {
                     }
                 },
                 ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
-                ("take", args @ [_arg]) => {
+                ("take", [_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);
+                            iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
                         }
                     }
                 },
diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs
@@ -78,9 +78,9 @@ pub fn get_associated_type<'tcx>(
     cx.tcx
         .associated_items(trait_id)
         .find_by_name_and_kind(cx.tcx, Ident::from_str(name), ty::AssocKind::Type, trait_id)
-        .map(|assoc| {
+        .and_then(|assoc| {
             let proj = cx.tcx.mk_projection(assoc.def_id, cx.tcx.mk_substs_trait(ty, &[]));
-            cx.tcx.normalize_erasing_regions(cx.param_env, proj)
+            cx.tcx.try_normalize_erasing_regions(cx.param_env, proj).ok()
         })
 }
 
diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed
@@ -18,7 +18,8 @@ fn main() {
     let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned();
 
     let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
-        .iter().flatten().cloned();
+        .iter()
+        .flatten().cloned();
 
     // Not implemented yet
     let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
@@ -43,6 +44,9 @@ fn main() {
 
     // Should probably stay as it is.
     let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
+
+    // `&Range<_>` doesn't implement `IntoIterator`
+    let _ = [0..1, 2..5].iter().cloned().flatten();
 }
 
 // #8527
diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs
@@ -45,6 +45,9 @@ fn main() {
 
     // Should probably stay as it is.
     let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
+
+    // `&Range<_>` doesn't implement `IntoIterator`
+    let _ = [0..1, 2..5].iter().cloned().flatten();
 }
 
 // #8527
diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr
@@ -1,44 +1,56 @@
-error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `last().cloned()` instead
+error: unnecessarily eager cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:8:29
    |
 LL |     let _: Option<String> = vec.iter().cloned().last();
-   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().last().cloned()`
+   |                             ^^^^^^^^^^----------------
+   |                                       |
+   |                                       help: try this: `.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
+error: unnecessarily eager cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:10:29
    |
 LL |     let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next();
-   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().chain(vec.iter()).next().cloned()`
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------
+   |                                                         |
+   |                                                         help: try this: `.next().cloned()`
 
-error: called `cloned().count()` on an `Iterator`. It may be more efficient to call `count()` instead
+error: unneeded cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:12:20
    |
 LL |     let _: usize = vec.iter().filter(|x| x == &"2").cloned().count();
-   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").count()`
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------
+   |                                                    |
+   |                                                    help: try this: `.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
+error: unnecessarily eager cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:14:21
    |
 LL |     let _: Vec<_> = vec.iter().cloned().take(2).collect();
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().take(2).cloned()`
+   |                     ^^^^^^^^^^-----------------
+   |                               |
+   |                               help: try this: `.take(2).cloned()`
 
-error: called `cloned().skip(...)` on an `Iterator`. It may be more efficient to call `skip(...).cloned()` instead
+error: unnecessarily eager cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:16:21
    |
 LL |     let _: Vec<_> = vec.iter().cloned().skip(2).collect();
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().skip(2).cloned()`
+   |                     ^^^^^^^^^^-----------------
+   |                               |
+   |                               help: try this: `.skip(2).cloned()`
 
-error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call `nth(...).cloned()` instead
+error: unnecessarily eager cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:18:13
    |
 LL |     let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2);
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()`
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------
+   |                                             |
+   |                                             help: try this: `.nth(2).cloned()`
 
-error: called `cloned().flatten()` on an `Iterator`. It may be more efficient to call `flatten().cloned()` instead
+error: unnecessarily eager cloning of iterator items
   --> $DIR/iter_overeager_cloned.rs:20:13
    |
 LL |       let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
@@ -50,8 +62,8 @@ LL | |         .flatten();
    |
 help: try this
    |
-LL ~     let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
-LL ~         .iter().flatten().cloned();
+LL ~         .iter()
+LL ~         .flatten().cloned();
    |
 
 error: aborting due to 7 previous errors

Original file line number	Diff line number	Diff line change
`@@ -45,6 +45,9 @@ fn main() {`
`45`	`45`
`46`	`46`	`// Should probably stay as it is.`
`47`	`47`	`let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);`
	`48`	`+`
	`49`	+ // `&Range<_>` doesn't implement `IntoIterator`
	`50`	`+ let _ = [0..1, 2..5].iter().cloned().flatten();`
`48`	`51`	`}`
`49`	`52`
`50`	`53`	`// #8527`