diff options
| -rw-r--r-- | clippy_lints/src/assigning_clones.rs | 65 | ||||
| -rw-r--r-- | tests/ui/assigning_clones.fixed | 29 | ||||
| -rw-r--r-- | tests/ui/assigning_clones.rs | 29 |
3 files changed, 122 insertions, 1 deletions
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::<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(); + } +} 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 @@ 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(); + } +} |