From dff9164ac5a0118e78285869c16f35447b720354 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 2 May 2024 23:32:53 +0200 Subject: [PATCH 1/3] consider `copy_deref` a possible borrower --- clippy_utils/src/mir/possible_borrower.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs index 06229ac938f..b3210fa155c 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 => { From 20d1bb1867e6b5b5c79d1af7cb56b91e290d0283 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 2 May 2024 23:38:31 +0200 Subject: [PATCH 2/3] [`assigning_clones`]: bail out when the source borrows from target --- clippy_lints/src/assigning_clones.rs | 65 +++++++++++++++++++++++++++- tests/ui/assigning_clones.fixed | 29 +++++++++++++ tests/ui/assigning_clones.rs | 29 +++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs index 9202c46e8b6..4819471746f 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,61 @@ 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 { + 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 diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed index 70ab43b49b3..64a1f02eb05 100644 --- a/tests/ui/assigning_clones.fixed +++ b/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::>(); + 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(); + } +} diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs index 9699fed100c..b92452c3577 100644 --- a/tests/ui/assigning_clones.rs +++ b/tests/ui/assigning_clones.rs @@ -272,3 +272,32 @@ 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 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(); + } +} From 60508f546a07fa51169736766b7e16676724c496 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Fri, 3 May 2024 01:45:49 +0200 Subject: [PATCH 3/3] deal with non-`Drop` types and add a test case for projections --- clippy_lints/src/assigning_clones.rs | 39 ++++++++++++++++++---------- tests/ui/assigning_clones.fixed | 27 +++++++++++++++++++ tests/ui/assigning_clones.rs | 27 +++++++++++++++++++ 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs index 4819471746f..920d1bae0b1 100644 --- a/clippy_lints/src/assigning_clones.rs +++ b/clippy_lints/src/assigning_clones.rs @@ -239,12 +239,23 @@ fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallC /// /// 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. + // 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: @@ -260,26 +271,28 @@ fn clone_source_borrows_from_dest(cx: &LateContext<'_>, lhs: &Expr<'_>, call_spa 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 + 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 - // 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 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 } diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed index 64a1f02eb05..60f6a385fc5 100644 --- a/tests/ui/assigning_clones.fixed +++ b/tests/ui/assigning_clones.fixed @@ -290,6 +290,33 @@ mod borrowck_conflicts { 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(); diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs index b92452c3577..6eb85be511a 100644 --- a/tests/ui/assigning_clones.rs +++ b/tests/ui/assigning_clones.rs @@ -290,6 +290,33 @@ fn issue12444_simple() { 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();