Auto merge of rust-lang#12756 - y21:assigning_clones_lifetimes, r=Alexendoo

Avoid emitting `assigning_clones` when cloned data borrows from the place to clone into

Fixes rust-lang#12444
Fixes rust-lang#12460
Fixes rust-lang#12749
Fixes rust-lang#12757
Fixes rust-lang#12929

I think the documentation for the function should describe what- and how this is fixing the issues well.
It avoids emitting a warning when the data being cloned borrows from the place to clone into, which is information that we can get from `PossibleBorrowerMap`. Unfortunately, it is a tiny bit tedious to match on the MIR like that and I'm not sure if this is possibly relying a bit too much on the exact MIR lowering for assignments.

Things left to do:
- [x] Handle place projections (or verify that they work as expected)
- [x] Handle non-`Drop` types

changelog: [`assigning_clones`]: avoid warning when the suggestion would lead to a borrow-check error

Author: bors

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,85 @@ 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 {
+    /// If this basic block only exists to drop a local as part of an assignment, returns its
+    /// successor. Otherwise returns the basic block that was passed in.
+    fn skip_drop_block(mir: &mir::Body<'_>, bb: mir::BasicBlock) -> mir::BasicBlock {
+        if let mir::TerminatorKind::Drop { target, .. } = mir.basic_blocks[bb].terminator().kind {
+            target
+        } else {
+            bb
+        }
+    }
+
+    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 *if* the type has `Drop`
+    // code. For types that don't, the second basic block is simply skipped.
+    // 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 {
+            continue;
+        }
+
+        if let mir::TerminatorKind::Call { ref args, target: Some(assign_bb), .. } = terminator.kind
+            && let [source] = &**args
+            && let mir::Operand::Move(source) = &source.node
+            && let assign_bb = skip_drop_block(mir, assign_bb)
+            // Skip any storage statements as they are just noise
+            && let Some(assignment) = mir.basic_blocks[assign_bb].statements
+                .iter()
+                .find(|stmt| {
+                    !matches!(stmt.kind, mir::StatementKind::StorageDead(_) | mir::StatementKind::StorageLive(_))
+                })
+            && let mir::StatementKind::Assign(box (borrowed, _)) = &assignment.kind
+            && let Some(borrowers) = borrow_map.get(&borrowed.local)
+            && borrowers.contains(source.local)
+        {
+            return true;
+        }
+
+        return false;
+    }
+    false
+}
+
 fn suggest<'tcx>(
     cx: &LateContext<'tcx>,
     ctxt: SyntaxContext,
@@ -255,6 +330,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

clippy_utils/src/mir/possible_borrower.rs

@@ -69,7 +69,7 @@ impl<'a, 'b, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'b,
     fn visit_assign(&mut self, place: &mir::Place<'tcx>, rvalue: &mir::Rvalue<'_>, _location: mir::Location) {
         let lhs = place.local;
         match rvalue {
-            mir::Rvalue::Ref(_, _, borrowed) => {
+            mir::Rvalue::Ref(_, _, borrowed) | mir::Rvalue::CopyForDeref(borrowed) => {
                 self.possible_borrower.add(borrowed.local, lhs);
             },
             other => {

tests/ui/assigning_clones.fixed

@@ -272,3 +272,59 @@ 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 issue12444_nodrop_projections() {
+        struct NoDrop;
+
+        impl Clone for NoDrop {
+            fn clone(&self) -> Self {
+                todo!()
+            }
+            fn clone_from(&mut self, other: &Self) {
+                todo!()
+            }
+        }
+
+        let mut s = NoDrop;
+        let s2 = &s;
+        s = s2.clone();
+
+        let mut s = (NoDrop, NoDrop);
+        let s2 = &s.0;
+        s.0 = s2.clone();
+
+        // This *could* emit a warning, but PossibleBorrowerMap only works with locals so it
+        // considers `s` fully borrowed
+        let mut s = (NoDrop, NoDrop);
+        let s2 = &s.1;
+        s.0 = s2.clone();
+    }
+
+    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,59 @@ 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 issue12444_nodrop_projections() {
+        struct NoDrop;
+
+        impl Clone for NoDrop {
+            fn clone(&self) -> Self {
+                todo!()
+            }
+            fn clone_from(&mut self, other: &Self) {
+                todo!()
+            }
+        }
+
+        let mut s = NoDrop;
+        let s2 = &s;
+        s = s2.clone();
+
+        let mut s = (NoDrop, NoDrop);
+        let s2 = &s.0;
+        s.0 = s2.clone();
+
+        // This *could* emit a warning, but PossibleBorrowerMap only works with locals so it
+        // considers `s` fully borrowed
+        let mut s = (NoDrop, NoDrop);
+        let s2 = &s.1;
+        s.0 = s2.clone();
+    }
+
+    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();
+    }
+}