Auto merge of #11301 - y21:issue11300, r=dswij

[`useless_conversion`]: don't lint if type parameter has unsatisfiable bounds for `.into_iter()` receiver

Fixes #11300.

Before this PR, clippy assumed that if it sees a `f(x.into_iter())` call and the type at that argument position is generic over any `IntoIterator`, then the `.into_iter()` call must be useless because `x` already implements `IntoIterator`, *however* this assumption is not right if the generic parameter has more than just the `IntoIterator` bound (because other traits can be implemented for the IntoIterator target type but not the IntoIterator implementor, as can be seen in the linked issue: `<[i32; 3] as IntoIterator>::IntoIter` satisfies `ExactSizeIterator`, but `[i32; 3]` does not).

So, this PR makes it check that the type parameter only has a single `IntoIterator` bound. It *might* be possible to check if the type of `x` in `f(x.into_iter())` satisfies all the bounds on the generic type parameter as defined on the function (which would allow removing the `.into_iter()` call even with multiple bounds), however I'm not sure how to do that, and the current fix should always work.

**Edit:** This PR has been changed to check if any of the bounds don't hold for the type of the `.into_iter()` receiver, so we can still lint in some cases.

changelog: [`useless_conversion`]: don't lint `.into_iter()` if type parameter has multiple bounds

4 files changed, 331 insertions, 31 deletions

diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs
index 5ac4f0aa46c..f32e7edad6c 100644
--- a/clippy_lints/src/useless_conversion.rs
+++ b/clippy_lints/src/useless_conversion.rs
@@ -8,10 +8,14 @@ use rustc_errors::Applicability;
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::DefId;
 use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind};
+use rustc_infer::infer::TyCtxtInferExt;
+use rustc_infer::traits::Obligation;
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty;
+use rustc_middle::traits::ObligationCause;
+use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{sym, Span};
+use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -61,22 +65,69 @@ impl MethodOrFunction {
     }
 }
 
-/// 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, 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::ClauseKind::Trait(tr) = pred.kind().skip_binder()
-                && tr.def_id() == into_iter_did
-                && tr.self_ty().is_param(param_index)
-            {
-                Some(span)
-            } else {
-                None
+/// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did`,
+/// iff all of the bounds also hold for the type of the `.into_iter()` receiver.
+/// ```ignore
+/// pub fn foo<I>(i: I)
+/// where I: IntoIterator<Item=i32> + ExactSizeIterator
+///                                   ^^^^^^^^^^^^^^^^^ this extra bound stops us from suggesting to remove `.into_iter()` ...
+/// {
+///     assert_eq!(i.len(), 3);
+/// }
+///
+/// pub fn bar() {
+///     foo([1, 2, 3].into_iter());
+///                  ^^^^^^^^^^^^ ... here, because `[i32; 3]` is not `ExactSizeIterator`
+/// }
+/// ```
+fn into_iter_bound<'tcx>(
+    cx: &LateContext<'tcx>,
+    fn_did: DefId,
+    into_iter_did: DefId,
+    into_iter_receiver: Ty<'tcx>,
+    param_index: u32,
+    node_args: GenericArgsRef<'tcx>,
+) -> Option<Span> {
+    let param_env = cx.tcx.param_env(fn_did);
+    let mut into_iter_span = None;
+
+    for (pred, span) in cx.tcx.explicit_predicates_of(fn_did).predicates {
+        if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() {
+            if tr.self_ty().is_param(param_index) {
+                if tr.def_id() == into_iter_did {
+                    into_iter_span = Some(*span);
+                } else {
+                    let tr = cx.tcx.erase_regions(tr);
+                    if tr.has_escaping_bound_vars() {
+                        return None;
+                    }
+
+                    // Substitute generics in the predicate and replace the IntoIterator type parameter with the
+                    // `.into_iter()` receiver to see if the bound also holds for that type.
+                    let args = cx.tcx.mk_args_from_iter(node_args.iter().enumerate().map(|(i, arg)| {
+                        if i == param_index as usize {
+                            GenericArg::from(into_iter_receiver)
+                        } else {
+                            arg
+                        }
+                    }));
+
+                    let predicate = EarlyBinder::bind(tr).instantiate(cx.tcx, args);
+                    let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), param_env, predicate);
+                    if !cx
+                        .tcx
+                        .infer_ctxt()
+                        .build()
+                        .predicate_must_hold_modulo_regions(&obligation)
+                    {
+                        return None;
+                    }
+                }
             }
-        })
+        }
+    }
+
+    into_iter_span
 }
 
 /// Extracts the receiver of a `.into_iter()` method call.
@@ -160,22 +211,41 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
                                     // `fn_sig` does not ICE. (see #11065)
                                     && cx.tcx.opt_def_kind(did).is_some_and(DefKind::is_fn_like) =>
                             {
-                                    Some((did, args, MethodOrFunction::Function))
+                                Some((
+                                    did,
+                                    args,
+                                    cx.typeck_results().node_args(recv.hir_id),
+                                    MethodOrFunction::Function
+                                ))
                             }
                             ExprKind::MethodCall(.., args, _) => {
                                 cx.typeck_results().type_dependent_def_id(parent.hir_id)
-                                    .map(|did| (did, args, MethodOrFunction::Method))
+                                    .map(|did| {
+                                        return (
+                                            did,
+                                            args,
+                                            cx.typeck_results().node_args(parent.hir_id),
+                                            MethodOrFunction::Method
+                                        );
+                                    })
                             }
                             _ => None,
                         };
 
-                        if let Some((parent_fn_did, args, kind)) = parent_fn
+                        if let Some((parent_fn_did, args, node_args, kind)) = parent_fn
                             && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
                             && let sig = cx.tcx.fn_sig(parent_fn_did).skip_binder().skip_binder()
                             && let Some(arg_pos) = args.iter().position(|x| x.hir_id == e.hir_id)
                             && 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, into_iter_did, param.index)
+                            && let Some(span) = into_iter_bound(
+                                cx,
+                                parent_fn_did,
+                                into_iter_did,
+                                cx.typeck_results().expr_ty(into_iter_recv),
+                                param.index,
+                                node_args
+                            )
                             && self.expn_depth == 0
                         {
                             // Get the "innermost" `.into_iter()` call, e.g. given this expression:
diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed
index 52591959905..ed8387b7eb2 100644
--- a/tests/ui/useless_conversion.fixed
+++ b/tests/ui/useless_conversion.fixed
@@ -151,6 +151,8 @@ fn main() {
     let _ = s3;
     let s4: Foo<'a'> = Foo;
     let _ = vec![s4, s4, s4].into_iter();
+
+    issue11300::bar();
 }
 
 #[allow(dead_code)]
@@ -196,6 +198,95 @@ fn explicit_into_iter_fn_arg() {
     b(macro_generated!());
 }
 
+mod issue11300 {
+    pub fn foo<I>(i: I)
+    where
+        I: IntoIterator<Item = i32> + ExactSizeIterator,
+    {
+        assert_eq!(i.len(), 3);
+    }
+
+    trait Helper<T: ?Sized> {}
+    impl Helper<i32> for [i32; 3] {}
+    impl Helper<i32> for std::array::IntoIter<i32, 3> {}
+    impl Helper<()> for std::array::IntoIter<i32, 3> {}
+
+    fn foo2<X: ?Sized, I>(_: I)
+    where
+        I: IntoIterator<Item = i32> + Helper<X>,
+    {
+    }
+
+    trait Helper2<T> {}
+    impl Helper2<std::array::IntoIter<i32, 3>> for i32 {}
+    impl Helper2<[i32; 3]> for i32 {}
+    fn foo3<I>(_: I)
+    where
+        I: IntoIterator<Item = i32>,
+        i32: Helper2<I>,
+    {
+    }
+
+    pub fn bar() {
+        // This should not trigger the lint:
+        // `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be
+        // removed and is not useless.
+        foo([1, 2, 3].into_iter());
+
+        // This should trigger the lint, receiver type [i32; 3] also implements `Helper`
+        foo2::<i32, _>([1, 2, 3]);
+
+        // This again should *not* lint, since X = () and I = std::array::IntoIter<i32, 3>,
+        // and `[i32; 3]: Helper<()>` is not true (only `std::array::IntoIter<i32, 3>: Helper<()>` is).
+        foo2::<(), _>([1, 2, 3].into_iter());
+
+        // This should lint. Removing the `.into_iter()` means that `I` gets substituted with `[i32; 3]`,
+        // and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary.
+        foo3([1, 2, 3]);
+    }
+
+    fn ice() {
+        struct S1;
+        impl S1 {
+            pub fn foo<I: IntoIterator>(&self, _: I) {}
+        }
+
+        S1.foo([1, 2]);
+
+        // ICE that occured in itertools
+        trait Itertools {
+            fn interleave_shortest<J>(self, other: J)
+            where
+                J: IntoIterator,
+                Self: Sized;
+        }
+        impl<I: Iterator> Itertools for I {
+            fn interleave_shortest<J>(self, other: J)
+            where
+                J: IntoIterator,
+                Self: Sized,
+            {
+            }
+        }
+        let v0: Vec<i32> = vec![0, 2, 4];
+        let v1: Vec<i32> = vec![1, 3, 5, 7];
+        v0.into_iter().interleave_shortest(v1);
+
+        trait TraitWithLifetime<'a> {}
+        impl<'a> TraitWithLifetime<'a> for std::array::IntoIter<&'a i32, 2> {}
+
+        struct Helper;
+        impl<'a> Helper {
+            fn with_lt<I>(&self, _: I)
+            where
+                I: IntoIterator + TraitWithLifetime<'a>,
+            {
+            }
+        }
+        Helper.with_lt([&1, &2].into_iter());
+    }
+}
+
 #[derive(Copy, Clone)]
 struct Foo<const C: char>;
 
diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs
index befb2f9a5c3..991a7762fc6 100644
--- a/tests/ui/useless_conversion.rs
+++ b/tests/ui/useless_conversion.rs
@@ -151,6 +151,8 @@ fn main() {
     let _ = Foo::<'a'>::from(s3);
     let s4: Foo<'a'> = Foo;
     let _ = vec![s4, s4, s4].into_iter().into_iter();
+
+    issue11300::bar();
 }
 
 #[allow(dead_code)]
@@ -196,6 +198,95 @@ fn explicit_into_iter_fn_arg() {
     b(macro_generated!());
 }
 
+mod issue11300 {
+    pub fn foo<I>(i: I)
+    where
+        I: IntoIterator<Item = i32> + ExactSizeIterator,
+    {
+        assert_eq!(i.len(), 3);
+    }
+
+    trait Helper<T: ?Sized> {}
+    impl Helper<i32> for [i32; 3] {}
+    impl Helper<i32> for std::array::IntoIter<i32, 3> {}
+    impl Helper<()> for std::array::IntoIter<i32, 3> {}
+
+    fn foo2<X: ?Sized, I>(_: I)
+    where
+        I: IntoIterator<Item = i32> + Helper<X>,
+    {
+    }
+
+    trait Helper2<T> {}
+    impl Helper2<std::array::IntoIter<i32, 3>> for i32 {}
+    impl Helper2<[i32; 3]> for i32 {}
+    fn foo3<I>(_: I)
+    where
+        I: IntoIterator<Item = i32>,
+        i32: Helper2<I>,
+    {
+    }
+
+    pub fn bar() {
+        // This should not trigger the lint:
+        // `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be
+        // removed and is not useless.
+        foo([1, 2, 3].into_iter());
+
+        // This should trigger the lint, receiver type [i32; 3] also implements `Helper`
+        foo2::<i32, _>([1, 2, 3].into_iter());
+
+        // This again should *not* lint, since X = () and I = std::array::IntoIter<i32, 3>,
+        // and `[i32; 3]: Helper<()>` is not true (only `std::array::IntoIter<i32, 3>: Helper<()>` is).
+        foo2::<(), _>([1, 2, 3].into_iter());
+
+        // This should lint. Removing the `.into_iter()` means that `I` gets substituted with `[i32; 3]`,
+        // and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary.
+        foo3([1, 2, 3].into_iter());
+    }
+
+    fn ice() {
+        struct S1;
+        impl S1 {
+            pub fn foo<I: IntoIterator>(&self, _: I) {}
+        }
+
+        S1.foo([1, 2].into_iter());
+
+        // ICE that occured in itertools
+        trait Itertools {
+            fn interleave_shortest<J>(self, other: J)
+            where
+                J: IntoIterator,
+                Self: Sized;
+        }
+        impl<I: Iterator> Itertools for I {
+            fn interleave_shortest<J>(self, other: J)
+            where
+                J: IntoIterator,
+                Self: Sized,
+            {
+            }
+        }
+        let v0: Vec<i32> = vec![0, 2, 4];
+        let v1: Vec<i32> = vec![1, 3, 5, 7];
+        v0.into_iter().interleave_shortest(v1.into_iter());
+
+        trait TraitWithLifetime<'a> {}
+        impl<'a> TraitWithLifetime<'a> for std::array::IntoIter<&'a i32, 2> {}
+
+        struct Helper;
+        impl<'a> Helper {
+            fn with_lt<I>(&self, _: I)
+            where
+                I: IntoIterator + TraitWithLifetime<'a>,
+            {
+            }
+        }
+        Helper.with_lt([&1, &2].into_iter());
+    }
+}
+
 #[derive(Copy, Clone)]
 struct Foo<const C: char>;
 
diff --git a/tests/ui/useless_conversion.stderr b/tests/ui/useless_conversion.stderr
index 28e7bb61098..c1f8b6b4aa9 100644
--- a/tests/ui/useless_conversion.stderr
+++ b/tests/ui/useless_conversion.stderr
@@ -119,64 +119,112 @@ LL |     let _ = vec![s4, s4, s4].into_iter().into_iter();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![s4, s4, s4].into_iter()`
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:183:7
+  --> $DIR/useless_conversion.rs:185:7
    |
 LL |     b(vec![1, 2].into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]`
    |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:173:13
+  --> $DIR/useless_conversion.rs:175:13
    |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:184:7
+  --> $DIR/useless_conversion.rs:186:7
    |
 LL |     c(vec![1, 2].into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]`
    |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:174:18
+  --> $DIR/useless_conversion.rs:176:18
    |
 LL |     fn c(_: impl IntoIterator<Item = i32>) {}
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:185:7
+  --> $DIR/useless_conversion.rs:187:7
    |
 LL |     d(vec![1, 2].into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]`
    |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:177:12
+  --> $DIR/useless_conversion.rs:179:12
    |
 LL |         T: IntoIterator<Item = i32>,
    |            ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:188:7
+  --> $DIR/useless_conversion.rs:190:7
    |
 LL |     b(vec![1, 2].into_iter().into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`s: `vec![1, 2]`
    |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:173:13
+  --> $DIR/useless_conversion.rs:175:13
    |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:189:7
+  --> $DIR/useless_conversion.rs:191:7
    |
 LL |     b(vec![1, 2].into_iter().into_iter().into_iter());
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`s: `vec![1, 2]`
    |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:173:13
+  --> $DIR/useless_conversion.rs:175:13
    |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 24 previous errors
+error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
+  --> $DIR/useless_conversion.rs:237:24
+   |
+LL |         foo2::<i32, _>([1, 2, 3].into_iter());
+   |                        ^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `[1, 2, 3]`
+   |
+note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
+  --> $DIR/useless_conversion.rs:216:12
+   |
+LL |         I: IntoIterator<Item = i32> + Helper<X>,
+   |            ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
+  --> $DIR/useless_conversion.rs:245:14
+   |
+LL |         foo3([1, 2, 3].into_iter());
+   |              ^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `[1, 2, 3]`
+   |
+note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
+  --> $DIR/useless_conversion.rs:225:12
+   |
+LL |         I: IntoIterator<Item = i32>,
+   |            ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
+  --> $DIR/useless_conversion.rs:254:16
+   |
+LL |         S1.foo([1, 2].into_iter());
+   |                ^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `[1, 2]`
+   |
+note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
+  --> $DIR/useless_conversion.rs:251:27
+   |
+LL |             pub fn foo<I: IntoIterator>(&self, _: I) {}
+   |                           ^^^^^^^^^^^^
+
+error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
+  --> $DIR/useless_conversion.rs:273:44
+   |
+LL |         v0.into_iter().interleave_shortest(v1.into_iter());
+   |                                            ^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `v1`
+   |
+note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
+  --> $DIR/useless_conversion.rs:260:20
+   |
+LL |                 J: IntoIterator,
+   |                    ^^^^^^^^^^^^
+
+error: aborting due to 28 previous errors