Expand mutable capture check for is_iter_with_side_effects()

Author: profetia

clippy_utils/src/ty/mod.rs

@@ -1377,33 +1377,40 @@ 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;
-    };
-
-    is_iter_with_side_effects_impl(cx, iter_ty, iter_trait)
+    cx.tcx
+        .lang_items()
+        .iterator_trait()
+        .is_some_and(|iter_trait| has_non_owning_mutable_access(cx, iter_ty, iter_trait))
 }
 
-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)
-            }
-        });
+/// Check if `ty` contains mutable references or equivalent, which includes:
+/// - A mutable reference/pointer.
+/// - A reference/pointer to a non-`Freeze` type.
+/// - A `PhantomData` type containing any of the previous.
+fn has_non_owning_mutable_access<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, iter_trait: DefId) -> bool {
+    match ty.kind() {
+        ty::Adt(adt_def, args) if adt_def.is_phantom_data() => args
+            .types()
+            .any(|arg_ty| has_non_owning_mutable_access(cx, arg_ty, iter_trait)),
+        ty::Adt(adt_def, args) => adt_def
+            .all_fields()
+            .any(|field| has_non_owning_mutable_access(cx, field.ty(cx.tcx, args), iter_trait)),
+        ty::Array(elem_ty, _) | ty::Slice(elem_ty) => has_non_owning_mutable_access(cx, *elem_ty, iter_trait),
+        ty::Ref(_, pointee_ty, Mutability::Mut) if implements_trait(cx, *pointee_ty, iter_trait, &[]) => {
+            has_non_owning_mutable_access(cx, *pointee_ty, iter_trait)
+        },
+        ty::RawPtr(pointee_ty, mutability) | ty::Ref(_, pointee_ty, mutability) => {
+            mutability.is_mut() || !pointee_ty.is_freeze(cx.tcx, cx.typing_env())
+        },
+        ty::Closure(_, closure_args) => {
+            matches!(closure_args.types().next_back(), Some(captures) if has_non_owning_mutable_access(cx, captures, iter_trait))
+        },
+        ty::Tuple(tuple_args) => tuple_args
+            .iter()
+            .any(|arg_ty| has_non_owning_mutable_access(cx, arg_ty, iter_trait)),
+        _ => false,
     }
-
-    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> {