diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs index e94a6f3e3fc..05ea74b0d53 100644 --- a/clippy_lints/src/assigning_clones.rs +++ b/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,6 +218,10 @@ 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. @@ -222,6 +229,74 @@ fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallC 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 diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs index 7b4fd8a210e..07e6705cd3d 100644 --- a/clippy_utils/src/mir/possible_borrower.rs +++ b/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 => { diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed index 70ab43b49b3..60f6a385fc5 100644 --- a/tests/ui/assigning_clones.fixed +++ b/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::>(); + 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(); + } +} diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs index 9699fed100c..6eb85be511a 100644 --- a/tests/ui/assigning_clones.rs +++ b/tests/ui/assigning_clones.rs @@ -272,3 +272,59 @@ fn add(self, _: &'a mut HasCloneFrom) -> Self { 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::>(); + 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(); + } +}