Consider side effects when rewriting iterator behaviors (rust-lang#14490)

Closes rust-lang#9191
Closes rust-lang#14444
Closes rust-lang#8055

Adds a new helper to partly check for side effects by recursively
checking if the iterator type contains closures with mutable captures.

changelog: [`double_ended_iterator_last`] fix FP when iter has side
effects
changelog: [`needless_collect`] fix lint not consider side effects

Author: Jarcho

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;
+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};
@@ -27,10 +27,15 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp
         && let Some(last_def) = cx.tcx.provided_trait_methods(item).find(|m| m.name().as_str() == "last")
         // 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)
+        && !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

@@ -4,7 +4,9 @@ use super::NEEDLESS_COLLECT;
 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, make_normalized_projection, make_projection};
+use clippy_utils::ty::{
+    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,
     path_to_local_id,
@@ -23,13 +25,19 @@ use rustc_span::{Span, sym};
 
 const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed";
 
+#[expect(clippy::too_many_lines)]
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     name_span: Span,
     collect_expr: &'tcx Expr<'_>,
     iter_expr: &'tcx Expr<'tcx>,
     call_span: Span,
 ) {
+    let iter_ty = cx.typeck_results().expr_ty(iter_expr);
+    if has_non_owning_mutable_access(cx, iter_ty) {
+        return; // don't lint if the iterator has side effects
+    }
+
     match cx.tcx.parent_hir_node(collect_expr.hir_id) {
         Node::Expr(parent) => {
             check_collect_into_intoiterator(cx, parent, collect_expr, call_span, iter_expr);

clippy_utils/src/ty/mod.rs

@@ -1376,3 +1376,49 @@ pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'t
         _ => None,
     }
 }
+
+/// 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)
+    }
+
+    /// 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,
+        }
+    }
+
+    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() {
@@ -90,3 +97,14 @@ fn drop_order() {
     //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
     println!("Done");
 }
+
+fn issue_14444() {
+    let mut squares = vec![];
+    let last_square = [1, 2, 3]
+        .into_iter()
+        .map(|x| {
+            squares.push(x * x);
+            Some(x * x)
+        })
+        .last();
+}

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() {
@@ -90,3 +97,14 @@ fn drop_order() {
     //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
     println!("Done");
 }
+
+fn issue_14444() {
+    let mut squares = vec![];
+    let last_square = [1, 2, 3]
+        .into_iter()
+        .map(|x| {
+            squares.push(x * x);
+            Some(x * x)
+        })
+        .last();
+}

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