Auto merge of #12650 - cocodery:issue/12098, r=xFrednet

fix false positive in Issue/12098 because lack of consideration of mutable caller

fixes [Issue#12098](https://github.com/rust-lang/rust-clippy/issues/12098)

In issue#12098, the former code doesn't consider the caller for clone is mutable, and suggests to delete clone function.

In this change, we first get the inner caller requests for clone,
and if it's immutable, the following code will suggest deleting clone.

If it's mutable, the loop will check whether a borrow check violation exists,
if exists, the lint should not execute, and the function will directly return;
otherwise, the following code will handle this.

changelog: [`clippy::unnecessary_to_owned`]: fix false positive

4 files changed, 117 insertions, 4 deletions

diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs
index 520dcb2d52d..7431dc1cf0b 100644
--- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs
+++ b/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::{BindingMode, 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(BindingMode::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)
diff --git a/tests/ui/unnecessary_iter_cloned.fixed b/tests/ui/unnecessary_iter_cloned.fixed
index ad0e5fab029..2c582c90ba8 100644
--- a/tests/ui/unnecessary_iter_cloned.fixed
+++ b/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;
+        }
+    }
+}
diff --git a/tests/ui/unnecessary_iter_cloned.rs b/tests/ui/unnecessary_iter_cloned.rs
index d3d59c4c70f..a28ccd1efef 100644
--- a/tests/ui/unnecessary_iter_cloned.rs
+++ b/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;
+        }
+    }
+}
diff --git a/tests/ui/unnecessary_iter_cloned.stderr b/tests/ui/unnecessary_iter_cloned.stderr
index 9d3591e0dbf..fb98cfddc26 100644
--- a/tests/ui/unnecessary_iter_cloned.stderr
+++ b/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() {
    |                      ^^^^^^^^^^^^^^^^^^^^^