Fix suggestions that cause mutable borrow overlaps

Author: GnomedDev

clippy_lints/src/methods/unnecessary_collection_clone.rs

@@ -1,4 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_path_mutable;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::{deref_chain, get_inherent_method, implements_trait, make_normalized_projection};
 use rustc_errors::Applicability;
@@ -30,6 +31,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>) {
 
     // If the call before `into_iter` is `.clone()`
     if let Some(("clone", collection_expr, [], _, _)) = method_call(recv)
+        // and the binding being cloned is not mutable
+        && !is_path_mutable(cx, collection_expr).unwrap_or(false)
         // and the result of `into_iter` is an Iterator
         && let Some(&iterator_def_id) = diagnostic_items.name_to_id.get(&sym::Iterator)
         && let expr_ty = typeck_results.expr_ty(expr)

clippy_lints/src/methods/unnecessary_iter_cloned.rs

@@ -4,11 +4,11 @@ use clippy_utils::higher::ForLoop;
 use clippy_utils::source::SpanRangeExt;
 use clippy_utils::ty::{get_iterator_item_ty, implements_trait};
 use clippy_utils::visitors::for_each_expr_without_closures;
-use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local};
+use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, is_path_mutable};
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind};
+use rustc_hir::{Expr, ExprKind};
 use rustc_lint::LateContext;
 use rustc_span::{Symbol, sym};
 
@@ -42,22 +42,7 @@ pub fn check_for_loop_iter(
         && !clone_or_copy_needed
         && let Some(receiver_snippet) = receiver.span.get_source_text(cx)
     {
-        // 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 {
@@ -82,7 +67,12 @@ pub fn check_for_loop_iter(
             while let ExprKind::MethodCall(_, caller, _, _) = child.kind {
                 child = caller;
             }
-            if is_mutable(cx, child) && is_caller_or_fields_change(cx, body, child) {
+
+            // 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
+            if is_path_mutable(cx, child).unwrap_or(true) && is_caller_or_fields_change(cx, body, child) {
                 // skip lint
                 return true;
             }

clippy_lints/src/unnecessary_struct_initialization.rs

@@ -1,8 +1,8 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::is_copy;
-use clippy_utils::{get_parent_expr, path_to_local};
-use rustc_hir::{BindingMode, Expr, ExprField, ExprKind, Node, PatKind, Path, QPath, UnOp};
+use clippy_utils::{get_parent_expr, is_path_mutable, path_to_local};
+use rustc_hir::{Expr, ExprField, ExprKind, Path, QPath, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 
@@ -155,16 +155,6 @@ fn same_path_in_all_fields<'tcx>(
     }
 }
 
-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 check_references(cx: &LateContext<'_>, expr_a: &Expr<'_>, expr_b: &Expr<'_>) -> bool {
     if let Some(parent) = get_parent_expr(cx, expr_a)
         && let parent_ty = cx.typeck_results().expr_ty_adjusted(parent)
@@ -176,7 +166,7 @@ fn check_references(cx: &LateContext<'_>, expr_a: &Expr<'_>, expr_b: &Expr<'_>)
             return false;
         }
 
-        if parent_ty.is_mutable_ptr() && !is_mutable(cx, expr_b) {
+        if parent_ty.is_mutable_ptr() && !is_path_mutable(cx, expr_b).unwrap_or(true) {
             // The original can be used in a mutable reference context only if it is mutable.
             return false;
         }

clippy_utils/src/lib.rs

@@ -412,6 +412,17 @@ pub fn is_trait_item(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) -
     }
 }
 
+/// Checks if `expr` is a path to a mutable binding.
+pub fn is_path_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<bool> {
+    if let Some(hir_id) = path_to_local(expr)
+        && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
+    {
+        Some(matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..)))
+    } else {
+        None
+    }
+}
+
 pub fn last_path_segment<'tcx>(path: &QPath<'tcx>) -> &'tcx PathSegment<'tcx> {
     match *path {
         QPath::Resolved(_, path) => path.segments.last().expect("A path must have at least one segment"),

tests/ui/unnecessary_collection_clone.fixed

@@ -19,14 +19,20 @@ fn generic<T: Clone>(val: Vec<T>) -> Vec<T> {
     //~^ error: using clone on collection to own iterated items
 }
 
-fn used_mutably<T: Clone>(mut vals: Vec<T>) {
-    fn use_mutable<T>(_: &mut T) {}
+fn use_mutable<T>(_: &mut T) {}
 
-    for val in vals.iter().cloned() {
+// Should not lint, as the replacement causes the mutable borrow to overlap
+fn used_mutably<T: Clone>(mut vals: Vec<T>) {
+    for val in vals.clone().into_iter() {
         use_mutable(&mut vals)
     }
 }
 
+// Should not lint, as the replacement causes the mutable borrow to overlap
+fn used_mutably_chain<T: Clone>(mut vals: Vec<T>) {
+    vals.clone().into_iter().for_each(|_| use_mutable(&mut vals));
+}
+
 // Should not lint, as `Src` has no `iter` method to use.
 fn too_generic<Src, Dst, T: Clone>(val: Src) -> Dst
 where

tests/ui/unnecessary_collection_clone.rs

@@ -19,14 +19,20 @@ fn generic<T: Clone>(val: Vec<T>) -> Vec<T> {
     //~^ error: using clone on collection to own iterated items
 }
 
-fn used_mutably<T: Clone>(mut vals: Vec<T>) {
-    fn use_mutable<T>(_: &mut T) {}
+fn use_mutable<T>(_: &mut T) {}
 
+// Should not lint, as the replacement causes the mutable borrow to overlap
+fn used_mutably<T: Clone>(mut vals: Vec<T>) {
     for val in vals.clone().into_iter() {
         use_mutable(&mut vals)
     }
 }
 
+// Should not lint, as the replacement causes the mutable borrow to overlap
+fn used_mutably_chain<T: Clone>(mut vals: Vec<T>) {
+    vals.clone().into_iter().for_each(|_| use_mutable(&mut vals));
+}
+
 // Should not lint, as `Src` has no `iter` method to use.
 fn too_generic<Src, Dst, T: Clone>(val: Src) -> Dst
 where

tests/ui/unnecessary_collection_clone.stderr

@@ -19,11 +19,5 @@ error: using clone on collection to own iterated items
 LL |     val.clone().into_iter().collect()
    |     ^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `val.iter().cloned()`
 
-error: using clone on collection to own iterated items
-  --> tests/ui/unnecessary_collection_clone.rs:25:16
-   |
-LL |     for val in vals.clone().into_iter() {
-   |                ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `vals.iter().cloned()`
-
-error: aborting due to 4 previous errors
+error: aborting due to 3 previous errors