Expand mutable capture check for is_iter_with_side_effects()

Author: profetia

clippy_utils/src/ty/mod.rs

@@ -1377,33 +1377,43 @@ 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)
+    let mut phantoms = FxHashSet::default();
+    has_non_owning_mutable_access(cx, &mut phantoms, iter_ty)
 }
 
-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>,
+    phantoms: &mut FxHashSet<Ty<'tcx>>,
+    ty: Ty<'tcx>,
+) -> bool {
+    match ty.kind() {
+        ty::Adt(adt_def, args) if adt_def.is_phantom_data() => {
+            phantoms.insert(ty)
+                && args
+                    .types()
+                    .any(|arg_ty| has_non_owning_mutable_access(cx, phantoms, arg_ty))
+        },
+        ty::Adt(adt_def, args) => adt_def
+            .all_fields()
+            .any(|field| has_non_owning_mutable_access(cx, phantoms, field.ty(cx.tcx, args))),
+        ty::Array(elem_ty, _) | ty::Slice(elem_ty) => has_non_owning_mutable_access(cx, phantoms, *elem_ty),
 
-    false
+        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, phantoms, captures))
+        },
+        ty::Tuple(tuple_args) => tuple_args
+            .iter()
+            .any(|arg_ty| has_non_owning_mutable_access(cx, phantoms, arg_ty)),
+        _ => false,
+    }
 }

tests/ui/crashes/ice-11230.fixed

@@ -12,7 +12,7 @@ fn main() {
 // needless_collect
 trait Helper<'a>: Iterator<Item = fn()> {}
 
+// Should not be linted because we have no idea whether the iterator has side effects
 fn x(w: &mut dyn for<'a> Helper<'a>) {
-    w.next().is_none();
-    //~^ needless_collect
+    w.collect::<Vec<_>>().is_empty();
 }

tests/ui/crashes/ice-11230.rs

@@ -12,7 +12,7 @@ fn main() {
 // needless_collect
 trait Helper<'a>: Iterator<Item = fn()> {}
 
+// Should not be linted because we have no idea whether the iterator has side effects
 fn x(w: &mut dyn for<'a> Helper<'a>) {
     w.collect::<Vec<_>>().is_empty();
-    //~^ needless_collect
 }

tests/ui/crashes/ice-11230.stderr

@@ -7,14 +7,5 @@ LL |     for v in A.iter() {}
    = note: `-D clippy::explicit-iter-loop` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::explicit_iter_loop)]`
 
-error: avoid using `collect()` when not needed
-  --> tests/ui/crashes/ice-11230.rs:16:7
-   |
-LL |     w.collect::<Vec<_>>().is_empty();
-   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()`
-   |
-   = note: `-D clippy::needless-collect` implied by `-D warnings`
-   = help: to override `-D warnings` add `#[allow(clippy::needless_collect)]`
-
-error: aborting due to 2 previous errors
+error: aborting due to 1 previous error
 

tests/ui/double_ended_iterator_last.fixed

@@ -52,28 +52,35 @@ fn main() {
     let _ = CustomLast.last();
 }
 
+// Should not be linted because applying the lint would move the original iterator. This can only be
+// linted if the iterator is used thereafter.
 fn issue_14139() {
     let mut index = [true, true, false, false, false, true].iter();
-    let mut subindex = index.by_ref().take(3);
-    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let subindex = index.by_ref().take(3);
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let mut subindex = index.by_ref().take(3);
-    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let mut subindex = index.by_ref().take(3);
     let subindex = &mut subindex;
-    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let mut subindex = index.by_ref().take(3);
     let subindex = &mut subindex;
-    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
-    let (mut subindex, _) = (index.by_ref().take(3), 42);
-    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let (subindex, _) = (index.by_ref().take(3), 42);
+    let _ = subindex.last();
+    let _ = index.next();
 }
 
 fn drop_order() {

tests/ui/double_ended_iterator_last.rs

@@ -52,28 +52,35 @@ fn main() {
     let _ = CustomLast.last();
 }
 
+// Should not be linted because applying the lint would move the original iterator. This can only be
+// linted if the iterator is used thereafter.
 fn issue_14139() {
     let mut index = [true, true, false, false, false, true].iter();
     let subindex = index.by_ref().take(3);
-    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let mut subindex = index.by_ref().take(3);
-    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let mut subindex = index.by_ref().take(3);
     let subindex = &mut subindex;
-    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let mut subindex = index.by_ref().take(3);
     let subindex = &mut subindex;
-    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 
     let mut index = [true, true, false, false, false, true].iter();
     let (subindex, _) = (index.by_ref().take(3), 42);
-    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.last();
+    let _ = index.next();
 }
 
 fn drop_order() {

tests/ui/double_ended_iterator_last.stderr

@@ -18,55 +18,7 @@ LL |     let _ = DeIterator.last();
    |                        help: try: `next_back()`
 
 error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:58:13
-   |
-LL |     let _ = subindex.last();
-   |             ^^^^^^^^^^^^^^^
-   |
-help: try
-   |
-LL ~     let mut subindex = index.by_ref().take(3);
-LL ~     let _ = subindex.next_back();
-   |
-
-error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:62:13
-   |
-LL |     let _ = subindex.last();
-   |             ^^^^^^^^^------
-   |                      |
-   |                      help: try: `next_back()`
-
-error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:67:13
-   |
-LL |     let _ = subindex.last();
-   |             ^^^^^^^^^------
-   |                      |
-   |                      help: try: `next_back()`
-
-error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:72:13
-   |
-LL |     let _ = subindex.last();
-   |             ^^^^^^^^^------
-   |                      |
-   |                      help: try: `next_back()`
-
-error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:76:13
-   |
-LL |     let _ = subindex.last();
-   |             ^^^^^^^^^^^^^^^
-   |
-help: try
-   |
-LL ~     let (mut subindex, _) = (index.by_ref().take(3), 42);
-LL ~     let _ = subindex.next_back();
-   |
-
-error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:89:36
+  --> tests/ui/double_ended_iterator_last.rs:96:36
    |
 LL |     println!("Last element is {}", v.last().unwrap().0);
    |                                    ^^^^^^^^
@@ -78,5 +30,5 @@ LL ~     let mut v = v.into_iter();
 LL ~     println!("Last element is {}", v.next_back().unwrap().0);
    |
 
-error: aborting due to 8 previous errors
+error: aborting due to 3 previous errors
 

tests/ui/double_ended_iterator_last_unfixable.rs

@@ -1,10 +1,13 @@
 //@no-rustfix
 #![warn(clippy::double_ended_iterator_last)]
 
+// Should not be linted because applying the lint would move the original iterator. This can only be
+// linted if the iterator is used thereafter.
 fn main() {
     let mut index = [true, true, false, false, false, true].iter();
     let subindex = (index.by_ref().take(3), 42);
-    let _ = subindex.0.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    let _ = subindex.0.last();
+    let _ = index.next();
 }
 
 fn drop_order() {

tests/ui/double_ended_iterator_last_unfixable.stderr

@@ -1,21 +1,5 @@
 error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last_unfixable.rs:7:13
-   |
-LL |     let _ = subindex.0.last();
-   |             ^^^^^^^^^^^------
-   |                        |
-   |                        help: try: `next_back()`
-   |
-note: this must be made mutable to use `.next_back()`
-  --> tests/ui/double_ended_iterator_last_unfixable.rs:7:13
-   |
-LL |     let _ = subindex.0.last();
-   |             ^^^^^^^^^^
-   = note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
-   = help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
-
-error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last_unfixable.rs:20:36
+  --> tests/ui/double_ended_iterator_last_unfixable.rs:23:36
    |
 LL |     println!("Last element is {}", v.0.last().unwrap().0);
    |                                    ^^^^------
@@ -24,10 +8,12 @@ LL |     println!("Last element is {}", v.0.last().unwrap().0);
    |
    = note: this change will alter drop order which may be undesirable
 note: this must be made mutable to use `.next_back()`
-  --> tests/ui/double_ended_iterator_last_unfixable.rs:20:36
+  --> tests/ui/double_ended_iterator_last_unfixable.rs:23:36
    |
 LL |     println!("Last element is {}", v.0.last().unwrap().0);
    |                                    ^^^
+   = note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
 
-error: aborting due to 2 previous errors
+error: aborting due to 1 previous error
 

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> {
@@ -155,3 +187,26 @@ pub fn issue8055(v: impl IntoIterator<Item = i32>) -> Result<impl Iterator<Item
     }
     Ok(res.into_iter())
 }
+
+mod issue8055_regression {
+    struct Foo<T> {
+        inner: T,
+        marker: core::marker::PhantomData<Self>,
+    }
+
+    impl<T: Iterator> Iterator for Foo<T> {
+        type Item = T::Item;
+        fn next(&mut self) -> Option<Self::Item> {
+            self.inner.next()
+        }
+    }
+
+    fn foo() {
+        Foo {
+            inner: [].iter(),
+            marker: core::marker::PhantomData,
+        }
+        .collect::<Vec<&i32>>()
+        .len();
+    }
+}