Skip to content

Commit 419277a

Browse files
KobzolKobzol
committed
Handle Deref and Clone better when turning .clone into .clone_from 
1 parent 3e84ca8 commit 419277a

File tree

4 files changed

+241
-37
lines changed

4 files changed

+241
-37
lines changed

‎clippy_lints/src/assigning_clones.rs

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use clippy_utils::diagnostics::span_lint_and_then;
33
use clippy_utils::macros::HirNode;
44
use clippy_utils::mir::{enclosing_mir, PossibleBorrowerMap};
55
use clippy_utils::sugg::Sugg;
6+
use clippy_utils::ty::implements_trait;
67
use clippy_utils::{is_trait_method, local_is_initialized, path_to_local};
78
use rustc_errors::Applicability;
89
use rustc_hir::{self as hir, Expr, ExprKind};
@@ -366,8 +367,20 @@ impl<'tcx> CallCandidate<'tcx> {
366367
match self.kind {
367368
CallKind::MethodCall { receiver } => {
368369
let receiver_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind {
369-
// `*lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
370-
Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
370+
// If `ref_expr` implements both Deref(Mut) and Clone, we cannot remove the dereference.
371+
// Because in that case we would be calling `clone_from` on the type of `lhs` directly, but
372+
// we would also be passing it a value of type `self_expr`, which has type of `*lhs`.
373+
let ty = cx.typeck_results().expr_ty(ref_expr);
374+
let impls_clone = cx.tcx.lang_items().clone_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));
375+
let impls_deref = cx.tcx.lang_items().deref_mut_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));
376+
377+
if impls_clone && impls_deref {
378+
// `*lhs = self_expr.clone();` -> `(*lhs).clone_from(self_expr)`
379+
Sugg::hir_with_applicability(cx, lhs, "_", applicability)
380+
} else {
381+
// `*lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
382+
Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
383+
}
371384
} else {
372385
// `lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
373386
Sugg::hir_with_applicability(cx, lhs, "_", applicability)
@@ -388,8 +401,27 @@ impl<'tcx> CallCandidate<'tcx> {
388401
},
389402
CallKind::FunctionCall { self_arg, .. } => {
390403
let self_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind {
391-
// `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(lhs, self_expr)`
392-
Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
404+
// If `ref_expr` implements `Deref(Mut)`, we have to keep the dereference, and add `&mut`,
405+
// otherwise the first argument to `clone_from` would apply to the wrapper type, rather
406+
// than the target type.
407+
let ty = cx.typeck_results().expr_ty(ref_expr);
408+
// let ty2 = cx.typeck_results().expr_ty(lhs);
409+
// cx.typeck_results().expr_adjustments(lhs);
410+
let impls_deref = cx.tcx.lang_items().deref_mut_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));
411+
// let adjusted = cx.typeck_results().expr_ty_adjusted(ref_expr);
412+
// let adjusted2 = cx.typeck_results().expr_ty_adjusted(lhs);
413+
// eprintln!("{ty:?}, {ty2:?}, {}, {adjusted:?}, {adjusted2:?} impls_deref: {impls_deref}", ty == ty2);
414+
// eprintln!("{:?}", cx.typeck_results().expr_adjustments(lhs));
415+
// eprintln!("{:?}", cx.typeck_results().expr_adjustments(ref_expr));
416+
417+
if impls_deref {
418+
// `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(&mut *lhs, self_expr)`
419+
// mut_addr_deref is used to avoid unnecessary parentheses around `*lhs`
420+
Sugg::hir_with_applicability(cx, ref_expr, "_", applicability).mut_addr_deref()
421+
} else {
422+
// `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(lhs, self_expr)`
423+
Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
424+
}
393425
} else {
394426
// `lhs = Clone::clone(self_expr);` -> `Clone::clone_from(&mut lhs, self_expr)`
395427
Sugg::hir_with_applicability(cx, lhs, "_", applicability).mut_addr()

‎tests/ui/assigning_clones.fixed

Lines changed: 72 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612
44
#![allow(clippy::needless_late_init)]
55
#![allow(clippy::box_collection)]
6+
#![allow(clippy::boxed_local)]
67
#![warn(clippy::assigning_clones)]
78

89
use std::borrow::ToOwned;
@@ -33,23 +34,23 @@ fn clone_method_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) {
3334
}
3435

3536
fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
36-
Clone::clone_from(mut_thing, ref_thing);
37+
Clone::clone_from(&mut *mut_thing, ref_thing);
3738
}
3839

3940
fn clone_function_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) {
4041
Clone::clone_from(&mut mut_thing, ref_thing);
4142
}
4243

4344
fn clone_function_through_trait(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
44-
Clone::clone_from(mut_thing, ref_thing);
45+
Clone::clone_from(&mut *mut_thing, ref_thing);
4546
}
4647

4748
fn clone_function_through_type(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
48-
Clone::clone_from(mut_thing, ref_thing);
49+
Clone::clone_from(&mut *mut_thing, ref_thing);
4950
}
5051

5152
fn clone_function_fully_qualified(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
52-
Clone::clone_from(mut_thing, ref_thing);
53+
Clone::clone_from(&mut *mut_thing, ref_thing);
5354
}
5455

5556
fn clone_method_lhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
@@ -182,6 +183,31 @@ impl Clone for AvoidRecursiveCloneFrom {
182183
}
183184
}
184185

186+
// Deref handling
187+
fn clone_into_deref_method(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
188+
a.clone_from(&b);
189+
}
190+
191+
fn clone_into_deref_with_clone_method(mut a: DerefWrapperWithClone<HasCloneFrom>, b: HasCloneFrom) {
192+
(*a).clone_from(&b);
193+
}
194+
195+
fn clone_into_box_method(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
196+
(*a).clone_from(&b);
197+
}
198+
199+
fn clone_into_self_deref_method(a: &mut DerefWrapperWithClone<HasCloneFrom>, b: DerefWrapperWithClone<HasCloneFrom>) {
200+
a.clone_from(&b);
201+
}
202+
203+
fn clone_into_deref_function(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
204+
Clone::clone_from(&mut *a, &b);
205+
}
206+
207+
fn clone_into_box_function(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
208+
Clone::clone_from(&mut *a, &b);
209+
}
210+
185211
// ToOwned
186212
fn owned_method_mut_ref(mut_string: &mut String, ref_str: &str) {
187213
ref_str.clone_into(mut_string);
@@ -328,3 +354,45 @@ mod borrowck_conflicts {
328354
path = path.components().as_path().to_owned();
329355
}
330356
}
357+
358+
struct DerefWrapper<T>(T);
359+
360+
impl<T> Deref for DerefWrapper<T> {
361+
type Target = T;
362+
363+
fn deref(&self) -> &Self::Target {
364+
&self.0
365+
}
366+
}
367+
368+
impl<T> DerefMut for DerefWrapper<T> {
369+
fn deref_mut(&mut self) -> &mut Self::Target {
370+
&mut self.0
371+
}
372+
}
373+
374+
struct DerefWrapperWithClone<T>(T);
375+
376+
impl<T> Deref for DerefWrapperWithClone<T> {
377+
type Target = T;
378+
379+
fn deref(&self) -> &Self::Target {
380+
&self.0
381+
}
382+
}
383+
384+
impl<T> DerefMut for DerefWrapperWithClone<T> {
385+
fn deref_mut(&mut self) -> &mut Self::Target {
386+
&mut self.0
387+
}
388+
}
389+
390+
impl<T: Clone> Clone for DerefWrapperWithClone<T> {
391+
fn clone(&self) -> Self {
392+
Self(self.0.clone())
393+
}
394+
395+
fn clone_from(&mut self, source: &Self) {
396+
*self = Self(source.0.clone());
397+
}
398+
}

‎tests/ui/assigning_clones.rs

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612
44
#![allow(clippy::needless_late_init)]
55
#![allow(clippy::box_collection)]
6+
#![allow(clippy::boxed_local)]
67
#![warn(clippy::assigning_clones)]
78

89
use std::borrow::ToOwned;
@@ -182,6 +183,31 @@ impl Clone for AvoidRecursiveCloneFrom {
182183
}
183184
}
184185

186+
// Deref handling
187+
fn clone_into_deref_method(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
188+
*a = b.clone();
189+
}
190+
191+
fn clone_into_deref_with_clone_method(mut a: DerefWrapperWithClone<HasCloneFrom>, b: HasCloneFrom) {
192+
*a = b.clone();
193+
}
194+
195+
fn clone_into_box_method(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
196+
*a = b.clone();
197+
}
198+
199+
fn clone_into_self_deref_method(a: &mut DerefWrapperWithClone<HasCloneFrom>, b: DerefWrapperWithClone<HasCloneFrom>) {
200+
*a = b.clone();
201+
}
202+
203+
fn clone_into_deref_function(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
204+
*a = Clone::clone(&b);
205+
}
206+
207+
fn clone_into_box_function(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
208+
*a = Clone::clone(&b);
209+
}
210+
185211
// ToOwned
186212
fn owned_method_mut_ref(mut_string: &mut String, ref_str: &str) {
187213
*mut_string = ref_str.to_owned();
@@ -328,3 +354,45 @@ mod borrowck_conflicts {
328354
path = path.components().as_path().to_owned();
329355
}
330356
}
357+
358+
struct DerefWrapper<T>(T);
359+
360+
impl<T> Deref for DerefWrapper<T> {
361+
type Target = T;
362+
363+
fn deref(&self) -> &Self::Target {
364+
&self.0
365+
}
366+
}
367+
368+
impl<T> DerefMut for DerefWrapper<T> {
369+
fn deref_mut(&mut self) -> &mut Self::Target {
370+
&mut self.0
371+
}
372+
}
373+
374+
struct DerefWrapperWithClone<T>(T);
375+
376+
impl<T> Deref for DerefWrapperWithClone<T> {
377+
type Target = T;
378+
379+
fn deref(&self) -> &Self::Target {
380+
&self.0
381+
}
382+
}
383+
384+
impl<T> DerefMut for DerefWrapperWithClone<T> {
385+
fn deref_mut(&mut self) -> &mut Self::Target {
386+
&mut self.0
387+
}
388+
}
389+
390+
impl<T: Clone> Clone for DerefWrapperWithClone<T> {
391+
fn clone(&self) -> Self {
392+
Self(self.0.clone())
393+
}
394+
395+
fn clone_from(&mut self, source: &Self) {
396+
*self = Self(source.0.clone());
397+
}
398+
}

0 commit comments

Comments
 (0)