Expand mutable capture check for is_iter_with_side_effects()

Author: profetia

clippy_utils/src/ty/mod.rs

@@ -1377,8 +1377,8 @@ pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'t
     }
 }
 
-/// Check if `ty` is an `Iterator` and has side effects when iterated over. Currently, this only
-/// checks if the `ty` contains mutable captures, and thus may be imcomplete.
+/// Check if a Ty<'_> of `Iterator` has side effects when iterated over by checking if it
+/// captures any mutable references or equivalents.
 pub fn is_iter_with_side_effects<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool {
     let Some(iter_trait) = cx.tcx.lang_items().iterator_trait() else {
         return false;
@@ -1388,22 +1388,50 @@ pub fn is_iter_with_side_effects<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>
 }
 
 fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>, iter_trait: DefId) -> bool {
-    if implements_trait(cx, iter_ty, iter_trait, &[])
-        && let ty::Adt(_, args) = iter_ty.kind()
-    {
-        return args.types().any(|arg_ty| {
-            if let ty::Closure(_, closure_args) = arg_ty.kind()
-                && let Some(captures) = closure_args.types().next_back()
-            {
-                captures
-                    .tuple_fields()
-                    .iter()
-                    .any(|capture_ty| matches!(capture_ty.ref_mutability(), Some(Mutability::Mut)))
-            } else {
-                is_iter_with_side_effects_impl(cx, arg_ty, iter_trait)
-            }
-        });
+    if let ty::Adt(adt_def, args) = iter_ty.kind() {
+        return adt_def
+            .all_fields()
+            .any(|field| is_ty_with_side_effects(cx, field.ty(cx.tcx, args), iter_trait));
     }
 
     false
 }
+
+fn is_ty_with_side_effects<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, iter_trait: DefId) -> bool {
+    match ty.kind() {
+        ty::RawPtr(..) | ty::Ref(..) | ty::Adt(..) => is_mutable_reference_or_equivalent_and(cx, ty, |inner_ty| {
+            !implements_trait(cx, inner_ty, iter_trait, &[]) || is_iter_with_side_effects_impl(cx, inner_ty, iter_trait)
+        }),
+        ty::Closure(_, closure_args) => {
+            matches!(closure_args.types().next_back(), Some(captures) if captures.tuple_fields().iter().any(|capture_ty| is_ty_with_side_effects(cx, capture_ty, iter_trait)))
+        },
+        ty::Array(elem_ty, _) | ty::Slice(elem_ty) => is_ty_with_side_effects(cx, *elem_ty, iter_trait),
+        ty::Tuple(field_tys) => field_tys
+            .iter()
+            .any(|field_ty| is_ty_with_side_effects(cx, field_ty, iter_trait)),
+        _ => false,
+    }
+}
+
+/// Check if `ty` is a mutable reference or equivalent. This includes:
+/// - A mutable reference/pointer.
+/// - A reference/pointer to a non-`Freeze` type.
+/// - A `PhantomData` type containing any of the previous.
+pub fn is_mutable_reference_or_equivalent<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    is_mutable_reference_or_equivalent_and(cx, ty, |_| true)
+}
+
+fn is_mutable_reference_or_equivalent_and<'tcx, F>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, pred: F) -> bool
+where
+    F: Fn(Ty<'tcx>) -> bool,
+{
+    match ty.kind() {
+        ty::RawPtr(ty, mutability) | ty::Ref(_, ty, mutability) => {
+            (mutability.is_mut() || !ty.is_freeze(cx.tcx, cx.typing_env())) && pred(*ty)
+        },
+        ty::Adt(adt_def, args) => adt_def.all_fields().any(|field| {
+            matches!(field.ty(cx.tcx, args).kind(), ty::Adt(adt_def, args) if adt_def.is_phantom_data() && args.types().any(|arg_ty| is_mutable_reference_or_equivalent(cx, arg_ty)))
+        }),
+        _ => false,
+    }
+}

tests/ui/needless_collect.fixed

@@ -128,13 +128,45 @@ fn bar<I: IntoIterator<Item = usize>>(_: Vec<usize>, _: I) {}
 fn baz<I: IntoIterator<Item = usize>>(_: I, _: (), _: impl IntoIterator<Item = char>) {}
 
 mod issue9191 {
+    use std::cell::Cell;
     use std::collections::HashSet;
+    use std::hash::Hash;
+    use std::marker::PhantomData;
+    use std::ops::Deref;
 
-    fn foo(xs: Vec<i32>, mut ys: HashSet<i32>) {
+    fn captures_ref_mut(xs: Vec<i32>, mut ys: HashSet<i32>) {
         if xs.iter().map(|x| ys.remove(x)).collect::<Vec<_>>().contains(&true) {
             todo!()
         }
     }
+
+    #[derive(Debug, Clone)]
+    struct MyRef<'a>(PhantomData<&'a mut Cell<HashSet<i32>>>, *mut Cell<HashSet<i32>>);
+
+    impl MyRef<'_> {
+        fn new(target: &mut Cell<HashSet<i32>>) -> Self {
+            MyRef(PhantomData, target)
+        }
+
+        fn get(&mut self) -> &mut Cell<HashSet<i32>> {
+            unsafe { &mut *self.1 }
+        }
+    }
+
+    fn captures_phantom(xs: Vec<i32>, mut ys: Cell<HashSet<i32>>) {
+        let mut ys_ref = MyRef::new(&mut ys);
+        if xs
+            .iter()
+            .map({
+                let mut ys_ref = ys_ref.clone();
+                move |x| ys_ref.get().get_mut().remove(x)
+            })
+            .collect::<Vec<_>>()
+            .contains(&true)
+        {
+            todo!()
+        }
+    }
 }
 
 pub fn issue8055(v: impl IntoIterator<Item = i32>) -> Result<impl Iterator<Item = i32>, usize> {

tests/ui/needless_collect.rs

@@ -128,13 +128,45 @@ fn bar<I: IntoIterator<Item = usize>>(_: Vec<usize>, _: I) {}
 fn baz<I: IntoIterator<Item = usize>>(_: I, _: (), _: impl IntoIterator<Item = char>) {}
 
 mod issue9191 {
+    use std::cell::Cell;
     use std::collections::HashSet;
+    use std::hash::Hash;
+    use std::marker::PhantomData;
+    use std::ops::Deref;
 
-    fn foo(xs: Vec<i32>, mut ys: HashSet<i32>) {
+    fn captures_ref_mut(xs: Vec<i32>, mut ys: HashSet<i32>) {
         if xs.iter().map(|x| ys.remove(x)).collect::<Vec<_>>().contains(&true) {
             todo!()
         }
     }
+
+    #[derive(Debug, Clone)]
+    struct MyRef<'a>(PhantomData<&'a mut Cell<HashSet<i32>>>, *mut Cell<HashSet<i32>>);
+
+    impl MyRef<'_> {
+        fn new(target: &mut Cell<HashSet<i32>>) -> Self {
+            MyRef(PhantomData, target)
+        }
+
+        fn get(&mut self) -> &mut Cell<HashSet<i32>> {
+            unsafe { &mut *self.1 }
+        }
+    }
+
+    fn captures_phantom(xs: Vec<i32>, mut ys: Cell<HashSet<i32>>) {
+        let mut ys_ref = MyRef::new(&mut ys);
+        if xs
+            .iter()
+            .map({
+                let mut ys_ref = ys_ref.clone();
+                move |x| ys_ref.get().get_mut().remove(x)
+            })
+            .collect::<Vec<_>>()
+            .contains(&true)
+        {
+            todo!()
+        }
+    }
 }
 
 pub fn issue8055(v: impl IntoIterator<Item = i32>) -> Result<impl Iterator<Item = i32>, usize> {