[assigning_clones]: bail out when the source borrows from target

Author: y21

clippy_lints/src/assigning_clones.rs

@@ -1,16 +1,18 @@
 use clippy_config::msrvs::{self, Msrv};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::macros::HirNode;
+use clippy_utils::mir::{enclosing_mir, PossibleBorrowerMap};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::{is_trait_method, local_is_initialized, path_to_local};
 use rustc_errors::Applicability;
 use rustc_hir::{self as hir, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::mir;
 use rustc_middle::ty::{self, Instance, Mutability};
 use rustc_session::impl_lint_pass;
 use rustc_span::def_id::DefId;
 use rustc_span::symbol::sym;
-use rustc_span::{ExpnKind, SyntaxContext};
+use rustc_span::{ExpnKind, Span, SyntaxContext};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -144,6 +146,7 @@ fn extract_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<
     };
 
     Some(CallCandidate {
+        span: expr.span,
         target,
         kind,
         method_def_id: resolved_method.def_id(),
@@ -215,13 +218,72 @@ fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallC
         return false;
     };
 
+    if clone_source_borrows_from_dest(cx, lhs, call.span) {
+        return false;
+    }
+
     // Now take a look if the impl block defines an implementation for the method that we're interested
     // in. If not, then we're using a default implementation, which is not interesting, so we will
     // not suggest the lint.
     let implemented_fns = cx.tcx.impl_item_implementor_ids(impl_block);
     implemented_fns.contains_key(&provided_fn.def_id)
 }
 
+/// Checks if the data being cloned borrows from the place that is being assigned to:
+///
+/// ```
+/// let mut s = String::new();
+/// let s2 = &s;
+/// s = s2.to_owned();
+/// ```
+///
+/// This cannot be written `s2.clone_into(&mut s)` because it has conflicting borrows.
+fn clone_source_borrows_from_dest(cx: &LateContext<'_>, lhs: &Expr<'_>, call_span: Span) -> bool {
+    let Some(mir) = enclosing_mir(cx.tcx, lhs.hir_id) else {
+        return false;
+    };
+    let PossibleBorrowerMap { map: borrow_map, .. } = PossibleBorrowerMap::new(cx, mir);
+
+    // The operation `dest = src.to_owned()` in MIR is split up across 3 blocks.
+    // For the doc example above that would be roughly:
+    //
+    // bb0:
+    //  s2 = &s
+    //  s_temp = ToOwned::to_owned(move s2) -> bb1
+    //
+    // bb1:
+    //  drop(s) -> bb2  // drop the old string
+    //
+    // bb2:
+    //  s = s_temp
+    for bb in mir.basic_blocks.iter() {
+        let terminator = bb.terminator();
+
+        // Look for the to_owned/clone call.
+        if terminator.source_info.span == call_span
+            && let mir::TerminatorKind::Call {
+                args,
+                target: Some(drop_bb),
+                ..
+            } = &terminator.kind
+            && let [source] = &**args
+            && let mir::Operand::Move(source) = &source.node
+            // Block 2 only has the `drop()` terminator from to the assignment
+            && let drop_bb = &mir.basic_blocks[*drop_bb]
+            && let mir::TerminatorKind::Drop { target: assign_bb, .. } = drop_bb.terminator().kind
+            // Block 3 has the final assignment to the original local
+            && let assign_bb = &mir.basic_blocks[assign_bb]
+            && let [assignment, ..] = &*assign_bb.statements
+            && let mir::StatementKind::Assign(box (borrowed, _)) = &assignment.kind
+            && let Some(borrowers) = borrow_map.get(&borrowed.local)
+            && borrowers.contains(source.local)
+        {
+            return true;
+        }
+    }
+    false
+}
+
 fn suggest<'tcx>(
     cx: &LateContext<'tcx>,
     ctxt: SyntaxContext,
@@ -255,6 +317,7 @@ enum TargetTrait {
 
 #[derive(Debug)]
 struct CallCandidate<'tcx> {
+    span: Span,
     target: TargetTrait,
     kind: CallKind<'tcx>,
     // DefId of the called method from an impl block that implements the target trait

tests/ui/assigning_clones.fixed

@@ -272,3 +272,32 @@ impl<'a> Add for &'a mut HasCloneFrom {
         self
     }
 }
+
+mod borrowck_conflicts {
+    //! Cases where clone_into and friends cannot be used because the src/dest have conflicting
+    //! borrows.
+    use std::path::PathBuf;
+
+    fn issue12444(mut name: String) {
+        let parts = name.split(", ").collect::<Vec<_>>();
+        let first = *parts.first().unwrap();
+        name = first.to_owned();
+    }
+
+    fn issue12444_simple() {
+        let mut s = String::new();
+        let s2 = &s;
+        s = s2.to_owned();
+    }
+
+    fn issue12460(mut name: String) {
+        if let Some(stripped_name) = name.strip_prefix("baz-") {
+            name = stripped_name.to_owned();
+        }
+    }
+
+    fn issue12749() {
+        let mut path = PathBuf::from("/a/b/c");
+        path = path.components().as_path().to_owned();
+    }
+}

tests/ui/assigning_clones.rs

@@ -272,3 +272,32 @@ impl<'a> Add for &'a mut HasCloneFrom {
         self
     }
 }
+
+mod borrowck_conflicts {
+    //! Cases where clone_into and friends cannot be used because the src/dest have conflicting
+    //! borrows.
+    use std::path::PathBuf;
+
+    fn issue12444(mut name: String) {
+        let parts = name.split(", ").collect::<Vec<_>>();
+        let first = *parts.first().unwrap();
+        name = first.to_owned();
+    }
+
+    fn issue12444_simple() {
+        let mut s = String::new();
+        let s2 = &s;
+        s = s2.to_owned();
+    }
+
+    fn issue12460(mut name: String) {
+        if let Some(stripped_name) = name.strip_prefix("baz-") {
+            name = stripped_name.to_owned();
+        }
+    }
+
+    fn issue12749() {
+        let mut path = PathBuf::from("/a/b/c");
+        path = path.components().as_path().to_owned();
+    }
+}