Expand mutable capture check for is_iter_with_side_effects()

Author: profetia

clippy_lints/src/methods/double_ended_iterator_last.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::ty::{implements_trait, is_iter_with_side_effects};
+use clippy_utils::ty::{has_non_owning_mutable_access, implements_trait};
 use clippy_utils::{is_mutable, is_trait_method, path_to_local};
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, Node, PatKind};
@@ -28,11 +28,14 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp
         // if the resolved method is the same as the provided definition
         && fn_def.def_id() == last_def.def_id
         && let self_ty = cx.typeck_results().expr_ty(self_expr)
-        && !is_iter_with_side_effects(cx, self_ty)
+        && !has_non_owning_mutable_access(cx, self_ty)
     {
         let mut sugg = vec![(call_span, String::from("next_back()"))];
         let mut dont_apply = false;
+
         // if `self_expr` is a reference, it is mutable because it is used for `.last()`
+        // TODO: Change this to lint only when the referred iterator is not used later. If it is used later,
+        // changing to `next_back()` may change its behavior.
         if !(is_mutable(cx, self_expr) || self_type.is_ref()) {
             if let Some(hir_id) = path_to_local(self_expr)
                 && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)

clippy_lints/src/methods/needless_collect.rs

@@ -5,7 +5,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
 use clippy_utils::source::{snippet, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{
-    get_type_diagnostic_name, is_iter_with_side_effects, make_normalized_projection, make_projection,
+    get_type_diagnostic_name, has_non_owning_mutable_access, make_normalized_projection, make_projection,
 };
 use clippy_utils::{
     CaptureKind, can_move_expr_to_closure, fn_def_id, get_enclosing_block, higher, is_trait_method, path_to_local,
@@ -34,7 +34,7 @@ pub(super) fn check<'tcx>(
     call_span: Span,
 ) {
     let iter_ty = cx.typeck_results().expr_ty(iter_expr);
-    if is_iter_with_side_effects(cx, iter_ty) {
+    if has_non_owning_mutable_access(cx, iter_ty) {
         return; // don't lint if the iterator has side effects
     }
 

clippy_utils/src/ty/mod.rs

@@ -1377,33 +1377,48 @@ 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.
-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)
-}
+/// Check if a Ty<'_> of `Iterator` contains any mutable access to non-owning types by checking if
+/// it contains fields of mutable references or pointers, or references/pointers to non-`Freeze`
+/// types, or `PhantomData` types containing any of the previous. This can be used to check whether
+/// skipping iterating over an iterator will change its behavior.
+pub fn has_non_owning_mutable_access<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool {
+    fn normalize_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
+        cx.tcx.try_normalize_erasing_regions(cx.typing_env(), ty).unwrap_or(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_inner<'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_inner(cx, phantoms, arg_ty))
+            },
+            ty::Adt(adt_def, args) => adt_def.all_fields().any(|field| {
+                has_non_owning_mutable_access_inner(cx, phantoms, normalize_ty(cx, field.ty(cx.tcx, args)))
+            }),
+            ty::Array(elem_ty, _) | ty::Slice(elem_ty) => has_non_owning_mutable_access_inner(cx, phantoms, *elem_ty),
+            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_inner(cx, phantoms, captures))
+            },
+            ty::Tuple(tuple_args) => tuple_args
+                .iter()
+                .any(|arg_ty| has_non_owning_mutable_access_inner(cx, phantoms, arg_ty)),
+            _ => false,
+        }
     }
 
-    false
+    let mut phantoms = FxHashSet::default();
+    has_non_owning_mutable_access_inner(cx, &mut phantoms, iter_ty)
 }

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() {