update docs, simplify arg->param map, dont lint on chain

y21 · y21 · commit 6a76101d3a46 · 2023-05-24T15:59:12.000+02:00
diff --git a/clippy_lints/src/explicit_into_iter_fn_arg.rs b/clippy_lints/src/explicit_into_iter_fn_arg.rs
@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::source::snippet;
+use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::{get_parent_expr, is_trait_method};
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
@@ -16,50 +16,26 @@ declare_clippy_lint! {
     /// in a call argument that accepts `IntoIterator`.
     ///
     /// ### Why is this bad?
-    /// If a function has a generic parameter with an `IntoIterator` trait bound, it means that the function
-    /// will *have* to call `.into_iter()` to get an iterator out of it, thereby making the call to `.into_iter()`
-    /// at call site redundant.
-    ///
-    /// Consider this example:
-    /// ```rs,ignore
-    /// fn foo<T: IntoIterator<Item = u32>>(iter: T) {
-    ///   let it = iter.into_iter();
-    ///                ^^^^^^^^^^^^ the function has to call `.into_iter()` to get the iterator
-    /// }
-    ///
-    /// foo(vec![1, 2, 3].into_iter());
-    ///                  ^^^^^^^^^^^^ ... making this `.into_iter()` call redundant.
-    /// ```
-    ///
-    /// The reason for why calling `.into_iter()` twice (once at call site and again inside of the function) works in the first place
-    /// is because there is a blanket implementation of `IntoIterator` for all types that implement `Iterator` in the standard library,
-    /// in which it simply returns itself, effectively making the second call to `.into_iter()` a "no-op":
-    /// ```rust,ignore
-    /// impl<I: Iterator> IntoIterator for I {
-    ///     type Item = I::Item;
-    ///     type IntoIter = I;
-    ///
-    ///     fn into_iter(self) -> I {
-    ///        self
-    ///     }
-    /// }
-    /// ```
+    /// If a generic parameter has an `IntoIterator` bound, there is no need to call `.into_iter()` at call site.
+    /// Calling `IntoIterator::into_iter()` on a value implies that its type already implements `IntoIterator`,
+    /// so you can just pass the value as is.
     ///
     /// ### Example
     /// ```rust
-    /// fn even_sum<I: IntoIterator<Item = u32>>(iter: I) -> u32 {
+    /// fn even_sum<I: IntoIterator<Item = i32>>(iter: I) -> i32 {
     ///     iter.into_iter().filter(|&x| x % 2 == 0).sum()
     /// }
     ///
-    /// let _ = even_sum(vec![1, 2, 3].into_iter());
+    /// let _ = even_sum([1, 2, 3].into_iter());
+    /// //                        ^^^^^^^^^^^^ redundant. `[i32; 3]` implements `IntoIterator`
     /// ```
     /// Use instead:
     /// ```rust
-    /// fn even_sum<I: IntoIterator<Item = u32>>(iter: I) -> u32 {
+    /// fn even_sum<I: IntoIterator<Item = i32>>(iter: I) -> i32 {
     ///     iter.into_iter().filter(|&x| x % 2 == 0).sum()
     /// }
     ///
-    /// let _ = even_sum(vec![1, 2, 3]);
+    /// let _ = even_sum([1, 2, 3]);
     /// ```
     #[clippy::version = "1.71.0"]
     pub EXPLICIT_INTO_ITER_FN_ARG,
@@ -73,23 +49,41 @@ enum MethodOrFunction {
     Function,
 }
 
+impl MethodOrFunction {
+    /// Maps the argument position in `pos` to the parameter position.
+    /// For methods, `self` is skipped.
+    fn param_pos(self, pos: usize) -> usize {
+        match self {
+            MethodOrFunction::Method => pos + 1,
+            MethodOrFunction::Function => pos,
+        }
+    }
+}
+
 /// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did`
-fn into_iter_bound(cx: &LateContext<'_>, fn_did: DefId, param_index: u32) -> Option<Span> {
-    if let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator) {
-        cx.tcx
-            .predicates_of(fn_did)
-            .predicates
-            .iter()
-            .find_map(|&(ref pred, span)| {
-                if let ty::PredicateKind::Clause(ty::Clause::Trait(tr)) = pred.kind().skip_binder()
-                    && tr.def_id() == into_iter_did
-                    && tr.self_ty().is_param(param_index)
-                {
-                    Some(span)
-                } else {
-                    None
-                }
-            })
+fn into_iter_bound(cx: &LateContext<'_>, fn_did: DefId, into_iter_did: DefId, param_index: u32) -> Option<Span> {
+    cx.tcx
+        .predicates_of(fn_did)
+        .predicates
+        .iter()
+        .find_map(|&(ref pred, span)| {
+            if let ty::PredicateKind::Clause(ty::Clause::Trait(tr)) = pred.kind().skip_binder()
+                && tr.def_id() == into_iter_did
+                && tr.self_ty().is_param(param_index)
+            {
+                Some(span)
+            } else {
+                None
+            }
+        })
+}
+
+fn into_iter_call<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>) -> Option<&'hir Expr<'hir>> {
+    if let ExprKind::MethodCall(name, recv, _, _) = expr.kind
+        && is_trait_method(cx, expr, sym::IntoIterator)
+        && name.ident.name == sym::into_iter
+    {
+        Some(recv)
     } else {
         None
     }
@@ -101,40 +95,44 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitIntoIterFnArg {
             return;
         }
 
-        if let ExprKind::MethodCall(name, recv, ..) = expr.kind
-            && is_trait_method(cx, expr, sym::IntoIterator)
-            && name.ident.name == sym::into_iter
-            && let Some(parent_expr) = get_parent_expr(cx, expr)
+        if let Some(recv) = into_iter_call(cx, expr)
+            && let Some(parent) = get_parent_expr(cx, expr)
+            // Make sure that this is not a chained into_iter call (i.e. `x.into_iter().into_iter()`)
+            // That case is already covered by `useless_conversion` and we don't want to lint twice
+            // with two contradicting suggestions.
+            && into_iter_call(cx, parent).is_none()
+            && into_iter_call(cx, recv).is_none()
+            && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
         {
-            let parent_expr = match parent_expr.kind {
+
+            let parent = match parent.kind {
                 ExprKind::Call(recv, args) if let ExprKind::Path(ref qpath) = recv.kind => {
                     cx.qpath_res(qpath, recv.hir_id).opt_def_id()
                         .map(|did| (did, args, MethodOrFunction::Function))
                 }
                 ExprKind::MethodCall(.., args, _) => {
-                    cx.typeck_results().type_dependent_def_id(parent_expr.hir_id)
+                    cx.typeck_results().type_dependent_def_id(parent.hir_id)
                         .map(|did| (did, args, MethodOrFunction::Method))
                 }
                 _ => None,
             };
 
-            if let Some((parent_fn_did, args, kind)) = parent_expr
+            if let Some((parent_fn_did, args, kind)) = parent
                 && let sig = cx.tcx.fn_sig(parent_fn_did).skip_binder().skip_binder()
                 && let Some(arg_pos) = args.iter().position(|x| x.hir_id == expr.hir_id)
-                && let Some(&into_iter_param) = sig.inputs().get(match kind {
-                    MethodOrFunction::Function => arg_pos,
-                    MethodOrFunction::Method => arg_pos + 1, // skip self arg
-                })
+                && let Some(&into_iter_param) = sig.inputs().get(kind.param_pos(arg_pos))
                 && let ty::Param(param) = into_iter_param.kind()
-                && let Some(span) = into_iter_bound(cx, parent_fn_did, param.index)
+                && let Some(span) = into_iter_bound(cx, parent_fn_did, into_iter_did, param.index)
             {
-                let sugg = snippet(cx, recv.span.source_callsite(), "<expr>").into_owned();
+                let mut applicability = Applicability::MachineApplicable;
+                let sugg = snippet_with_applicability(cx, recv.span.source_callsite(), "<expr>", &mut applicability).into_owned();
+
                 span_lint_and_then(cx, EXPLICIT_INTO_ITER_FN_ARG, expr.span, "explicit call to `.into_iter()` in function argument accepting `IntoIterator`", |diag| {
                     diag.span_suggestion(
                         expr.span,
                         "consider removing `.into_iter()`",
                         sugg,
-                        Applicability::MachineApplicable,
+                        applicability,
                     );
                     diag.span_note(span, "this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`");
                 });
diff --git a/tests/ui/explicit_into_iter_fn_arg.fixed b/tests/ui/explicit_into_iter_fn_arg.fixed
@@ -1,6 +1,6 @@
 //@run-rustfix
 
-#![allow(unused)]
+#![allow(unused, clippy::useless_conversion)]
 #![warn(clippy::explicit_into_iter_fn_arg)]
 
 fn a<T>(_: T) {}
@@ -11,14 +11,23 @@ where
     T: IntoIterator<Item = i32>,
 {
 }
-fn e<T: IntoIterator<Item = i32>>(_: T) {}
 fn f(_: std::vec::IntoIter<i32>) {}
 
 fn main() {
     a(vec![1, 2].into_iter());
     b(vec![1, 2]);
     c(vec![1, 2]);
     d(vec![1, 2]);
-    e([&1, &2, &3].into_iter().cloned());
-    f(vec![1, 2].into_iter());
+    b([&1, &2, &3].into_iter().cloned());
+
+    // Don't lint chained `.into_iter().into_iter()` calls. Covered by useless_conversion.
+    b(vec![1, 2].into_iter().into_iter());
+    b(vec![1, 2].into_iter().into_iter().into_iter());
+
+    macro_rules! macro_generated {
+        () => {
+            vec![1, 2].into_iter()
+        };
+    }
+    b(macro_generated!());
 }
diff --git a/tests/ui/explicit_into_iter_fn_arg.rs b/tests/ui/explicit_into_iter_fn_arg.rs
@@ -1,6 +1,6 @@
 //@run-rustfix
 
-#![allow(unused)]
+#![allow(unused, clippy::useless_conversion)]
 #![warn(clippy::explicit_into_iter_fn_arg)]
 
 fn a<T>(_: T) {}
@@ -11,14 +11,23 @@ where
     T: IntoIterator<Item = i32>,
 {
 }
-fn e<T: IntoIterator<Item = i32>>(_: T) {}
 fn f(_: std::vec::IntoIter<i32>) {}
 
 fn main() {
     a(vec![1, 2].into_iter());
     b(vec![1, 2].into_iter());
     c(vec![1, 2].into_iter());
     d(vec![1, 2].into_iter());
-    e([&1, &2, &3].into_iter().cloned());
-    f(vec![1, 2].into_iter());
+    b([&1, &2, &3].into_iter().cloned());
+
+    // Don't lint chained `.into_iter().into_iter()` calls. Covered by useless_conversion.
+    b(vec![1, 2].into_iter().into_iter());
+    b(vec![1, 2].into_iter().into_iter().into_iter());
+
+    macro_rules! macro_generated {
+        () => {
+            vec![1, 2].into_iter()
+        };
+    }
+    b(macro_generated!());
 }
diff --git a/tests/ui/explicit_into_iter_fn_arg.stderr b/tests/ui/explicit_into_iter_fn_arg.stderr
@@ -1,5 +1,5 @@
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/explicit_into_iter_fn_arg.rs:19:7
+  --> $DIR/explicit_into_iter_fn_arg.rs:18:7
    |
 LL |     b(vec![1, 2].into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2]`
@@ -12,7 +12,7 @@ LL | fn b<T: IntoIterator<Item = i32>>(_: T) {}
    = note: `-D clippy::explicit-into-iter-fn-arg` implied by `-D warnings`
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/explicit_into_iter_fn_arg.rs:20:7
+  --> $DIR/explicit_into_iter_fn_arg.rs:19:7
    |
 LL |     c(vec![1, 2].into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2]`
@@ -24,7 +24,7 @@ LL | fn c(_: impl IntoIterator<Item = i32>) {}
    |              ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/explicit_into_iter_fn_arg.rs:21:7
+  --> $DIR/explicit_into_iter_fn_arg.rs:20:7
    |
 LL |     d(vec![1, 2].into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2]`
