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

Author: bors

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};
 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,61 @@ 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)
+/// 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.def_id() == into_iter_did && tr.self_ty().is_param(param_index) {
+                into_iter_span = Some(*span);
             } else {
-                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 +203,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(recv.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:

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,54 @@ 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]);
+    }
+}
+
 #[derive(Copy, Clone)]
 struct Foo<const C: char>;
 

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,54 @@ 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());
+    }
+}
+
 #[derive(Copy, Clone)]
 struct Foo<const C: char>;
 

tests/ui/useless_conversion.stderr

@@ -119,64 +119,88 @@ 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: aborting due to 26 previous errors