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 `ty` is an `Iterator` and 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;
@@ -1398,7 +1398,7 @@ fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx
                 captures
                     .tuple_fields()
                     .iter()
-                    .any(|capture_ty| matches!(capture_ty.ref_mutability(), Some(Mutability::Mut)))
+                    .any(|capture_ty| is_mutable_reference_or_equivalent(cx, capture_ty))
             } else {
                 is_iter_with_side_effects_impl(cx, arg_ty, iter_trait)
             }
@@ -1407,3 +1407,19 @@ fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx
 
     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 {
+    match ty.kind() {
+        ty::RawPtr(ty, mutability) | ty::Ref(_, ty, mutability) => {
+            mutability.is_mut() || !ty.is_freeze(cx.tcx, cx.typing_env())
+        },
+        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> {