Check inner caller for clone and judge whether they are mutable

if immutbale -> lint delete redudant clone
if mutable   -> lint check whether clone is needed

Author: cocodery

clippy_lints/src/methods/unnecessary_iter_cloned.rs

@@ -3,10 +3,12 @@ use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::ForLoop;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{get_iterator_item_ty, implements_trait};
-use clippy_utils::{fn_def_id, get_parent_expr};
+use clippy_utils::visitors::for_each_expr;
+use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local};
+use core::ops::ControlFlow;
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{Expr, ExprKind};
+use rustc_hir::{BindingAnnotation, Expr, ExprKind, Node, PatKind};
 use rustc_lint::LateContext;
 use rustc_span::{sym, Symbol};
 
@@ -40,6 +42,53 @@ pub fn check_for_loop_iter(
         && !clone_or_copy_needed
         && let Some(receiver_snippet) = snippet_opt(cx, receiver.span)
     {
+        // Issue 12098
+        // https://github.com/rust-lang/rust-clippy/issues/12098
+        // if the assignee have `mut borrow` conflict with the iteratee
+        // the lint should not execute, former didn't consider the mut case
+
+        // check whether `expr` is mutable
+        fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+            if let Some(hir_id) = path_to_local(expr)
+                && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
+            {
+                matches!(pat.kind, PatKind::Binding(BindingAnnotation::MUT, ..))
+            } else {
+                true
+            }
+        }
+
+        fn is_caller_or_fields_change(cx: &LateContext<'_>, body: &Expr<'_>, caller: &Expr<'_>) -> bool {
+            let mut change = false;
+            if let ExprKind::Block(block, ..) = body.kind {
+                for_each_expr(block, |e| {
+                    match e.kind {
+                        ExprKind::Assign(assignee, _, _) | ExprKind::AssignOp(_, assignee, _) => {
+                            change |= !can_mut_borrow_both(cx, caller, assignee);
+                        },
+                        _ => {},
+                    }
+                    // the return value has no effect but the function need one return value
+                    ControlFlow::<()>::Continue(())
+                });
+            }
+            change
+        }
+
+        if let ExprKind::Call(_, [child, ..]) = expr.kind {
+            // filter first layer of iterator
+            let mut child = child;
+            // get inner real caller requests for clone
+            while let ExprKind::MethodCall(_, caller, _, _) = child.kind {
+                child = caller;
+            }
+            if is_mutable(cx, child) && is_caller_or_fields_change(cx, body, child) {
+                // skip lint
+                return true;
+            }
+        };
+
+        // the lint should not be executed if no violation happens
         let snippet = if let ExprKind::MethodCall(maybe_iter_method_name, collection, [], _) = receiver.kind
             && maybe_iter_method_name.ident.name == sym::iter
             && let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator)

tests/ui/unnecessary_iter_cloned.fixed

@@ -21,6 +21,8 @@ fn main() {
     let _ = check_files_ref_mut(&[(FileType::Account, path)]);
     let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
     let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]);
+
+    check_mut_iteratee_and_modify_inner_variable();
 }
 
 // `check_files` and its variants are based on:
@@ -138,3 +140,33 @@ fn check_files_mut_path_buf(files: &[(FileType, std::path::PathBuf)]) -> bool {
 fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
     Ok(std::path::PathBuf::new())
 }
+
+// Issue 12098
+// https://github.com/rust-lang/rust-clippy/issues/12098
+// no message emits
+fn check_mut_iteratee_and_modify_inner_variable() {
+    struct Test {
+        list: Vec<String>,
+        mut_this: bool,
+    }
+
+    impl Test {
+        fn list(&self) -> &[String] {
+            &self.list
+        }
+    }
+
+    let mut test = Test {
+        list: vec![String::from("foo"), String::from("bar")],
+        mut_this: false,
+    };
+
+    for _item in test.list().to_vec() {
+        println!("{}", _item);
+
+        test.mut_this = true;
+        {
+            test.mut_this = true;
+        }
+    }
+}

tests/ui/unnecessary_iter_cloned.rs

@@ -21,6 +21,8 @@ fn main() {
     let _ = check_files_ref_mut(&[(FileType::Account, path)]);
     let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
     let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]);
+
+    check_mut_iteratee_and_modify_inner_variable();
 }
 
 // `check_files` and its variants are based on:
@@ -138,3 +140,33 @@ fn check_files_mut_path_buf(files: &[(FileType, std::path::PathBuf)]) -> bool {
 fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
     Ok(std::path::PathBuf::new())
 }
+
+// Issue 12098
+// https://github.com/rust-lang/rust-clippy/issues/12098
+// no message emits
+fn check_mut_iteratee_and_modify_inner_variable() {
+    struct Test {
+        list: Vec<String>,
+        mut_this: bool,
+    }
+
+    impl Test {
+        fn list(&self) -> &[String] {
+            &self.list
+        }
+    }
+
+    let mut test = Test {
+        list: vec![String::from("foo"), String::from("bar")],
+        mut_this: false,
+    };
+
+    for _item in test.list().to_vec() {
+        println!("{}", _item);
+
+        test.mut_this = true;
+        {
+            test.mut_this = true;
+        }
+    }
+}

tests/ui/unnecessary_iter_cloned.stderr

@@ -1,5 +1,5 @@
 error: unnecessary use of `copied`
-  --> tests/ui/unnecessary_iter_cloned.rs:29:22
+  --> tests/ui/unnecessary_iter_cloned.rs:31:22
    |
 LL |     for (t, path) in files.iter().copied() {
    |                      ^^^^^^^^^^^^^^^^^^^^^
@@ -17,7 +17,7 @@ LL +         let other = match get_file_path(t) {
    |
 
 error: unnecessary use of `copied`
-  --> tests/ui/unnecessary_iter_cloned.rs:44:22
+  --> tests/ui/unnecessary_iter_cloned.rs:46:22
    |
 LL |     for (t, path) in files.iter().copied() {
    |                      ^^^^^^^^^^^^^^^^^^^^^