Handle Deref and Clone better when turning .clone into `.clone_…

…from`
rust-lang · Jun 21, 2024 · 419277a · 419277a
1 parent 3e84ca8
commit 419277a
Show file tree

Hide file tree

Showing 4 changed files with 241 additions and 37 deletions.
diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs
@@ -3,6 +3,7 @@ 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::ty::implements_trait;
 use clippy_utils::{is_trait_method, local_is_initialized, path_to_local};
 use rustc_errors::Applicability;
 use rustc_hir::{self as hir, Expr, ExprKind};
@@ -366,8 +367,20 @@ impl<'tcx> CallCandidate<'tcx> {
  match self.kind {
  CallKind::MethodCall { receiver } => {
  let receiver_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind {
- // `*lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
- Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
+ // If `ref_expr` implements both Deref(Mut) and Clone, we cannot remove the dereference.
+ // Because in that case we would be calling `clone_from` on the type of `lhs` directly, but
+ // we would also be passing it a value of type `self_expr`, which has type of `*lhs`.
+ let ty = cx.typeck_results().expr_ty(ref_expr);
+ let impls_clone = cx.tcx.lang_items().clone_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));
+ let impls_deref = cx.tcx.lang_items().deref_mut_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));
+
+ if impls_clone && impls_deref {
+ // `*lhs = self_expr.clone();` -> `(*lhs).clone_from(self_expr)`
+ Sugg::hir_with_applicability(cx, lhs, "_", applicability)
+ } else {
+ // `*lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
+ Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
+ }
  } else {
  // `lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
  Sugg::hir_with_applicability(cx, lhs, "_", applicability)
@@ -388,8 +401,27 @@ impl<'tcx> CallCandidate<'tcx> {
  },
  CallKind::FunctionCall { self_arg, .. } => {
  let self_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind {
- // `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(lhs, self_expr)`
- Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
+ // If `ref_expr` implements `Deref(Mut)`, we have to keep the dereference, and add `&mut`,
+ // otherwise the first argument to `clone_from` would apply to the wrapper type, rather
+ // than the target type.
+ let ty = cx.typeck_results().expr_ty(ref_expr);
+ // let ty2 = cx.typeck_results().expr_ty(lhs);
+ // cx.typeck_results().expr_adjustments(lhs);
+ let impls_deref = cx.tcx.lang_items().deref_mut_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));
+ // let adjusted = cx.typeck_results().expr_ty_adjusted(ref_expr);
+ // let adjusted2 = cx.typeck_results().expr_ty_adjusted(lhs);
+ // eprintln!("{ty:?}, {ty2:?}, {}, {adjusted:?}, {adjusted2:?} impls_deref: {impls_deref}", ty == ty2);
+ // eprintln!("{:?}", cx.typeck_results().expr_adjustments(lhs));
+ // eprintln!("{:?}", cx.typeck_results().expr_adjustments(ref_expr));
+
+ if impls_deref {
+ // `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(&mut *lhs, self_expr)`
+ // mut_addr_deref is used to avoid unnecessary parentheses around `*lhs`
+ Sugg::hir_with_applicability(cx, ref_expr, "_", applicability).mut_addr_deref()
+ } else {
+ // `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(lhs, self_expr)`
+ Sugg::hir_with_applicability(cx, ref_expr, "_", applicability)
+ }
  } else {
  // `lhs = Clone::clone(self_expr);` -> `Clone::clone_from(&mut lhs, self_expr)`
  Sugg::hir_with_applicability(cx, lhs, "_", applicability).mut_addr()

diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed
@@ -3,6 +3,7 @@
 #![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612
 #![allow(clippy::needless_late_init)]
 #![allow(clippy::box_collection)]
+#![allow(clippy::boxed_local)]
 #![warn(clippy::assigning_clones)]
 
 use std::borrow::ToOwned;
@@ -33,23 +34,23 @@ fn clone_method_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) {
 }
 
 fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
- Clone::clone_from(mut_thing, ref_thing);
+ Clone::clone_from(&mut *mut_thing, ref_thing);
 }
 
 fn clone_function_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) {
  Clone::clone_from(&mut mut_thing, ref_thing);
 }
 
 fn clone_function_through_trait(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
- Clone::clone_from(mut_thing, ref_thing);
+ Clone::clone_from(&mut *mut_thing, ref_thing);
 }
 
 fn clone_function_through_type(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
- Clone::clone_from(mut_thing, ref_thing);
+ Clone::clone_from(&mut *mut_thing, ref_thing);
 }
 
 fn clone_function_fully_qualified(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
- Clone::clone_from(mut_thing, ref_thing);
+ Clone::clone_from(&mut *mut_thing, ref_thing);
 }
 
 fn clone_method_lhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
@@ -182,6 +183,31 @@ impl Clone for AvoidRecursiveCloneFrom {
  }
 }
 
+// Deref handling
+fn clone_into_deref_method(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
+ a.clone_from(&b);
+}
+
+fn clone_into_deref_with_clone_method(mut a: DerefWrapperWithClone<HasCloneFrom>, b: HasCloneFrom) {
+ (*a).clone_from(&b);
+}
+
+fn clone_into_box_method(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
+ (*a).clone_from(&b);
+}
+
+fn clone_into_self_deref_method(a: &mut DerefWrapperWithClone<HasCloneFrom>, b: DerefWrapperWithClone<HasCloneFrom>) {
+ a.clone_from(&b);
+}
+
+fn clone_into_deref_function(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
+ Clone::clone_from(&mut *a, &b);
+}
+
+fn clone_into_box_function(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
+ Clone::clone_from(&mut *a, &b);
+}
+
 // ToOwned
 fn owned_method_mut_ref(mut_string: &mut String, ref_str: &str) {
  ref_str.clone_into(mut_string);
@@ -328,3 +354,45 @@ mod borrowck_conflicts {
  path = path.components().as_path().to_owned();
  }
 }
+
+struct DerefWrapper<T>(T);
+
+impl<T> Deref for DerefWrapper<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+}
+
+impl<T> DerefMut for DerefWrapper<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.0
+ }
+}
+
+struct DerefWrapperWithClone<T>(T);
+
+impl<T> Deref for DerefWrapperWithClone<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+}
+
+impl<T> DerefMut for DerefWrapperWithClone<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.0
+ }
+}
+
+impl<T: Clone> Clone for DerefWrapperWithClone<T> {
+ fn clone(&self) -> Self {
+ Self(self.0.clone())
+ }
+
+ fn clone_from(&mut self, source: &Self) {
+ *self = Self(source.0.clone());
+ }
+}
diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs
@@ -3,6 +3,7 @@
 #![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612
 #![allow(clippy::needless_late_init)]
 #![allow(clippy::box_collection)]
+#![allow(clippy::boxed_local)]
 #![warn(clippy::assigning_clones)]
 
 use std::borrow::ToOwned;
@@ -182,6 +183,31 @@ impl Clone for AvoidRecursiveCloneFrom {
  }
 }
 
+// Deref handling
+fn clone_into_deref_method(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
+ *a = b.clone();
+}
+
+fn clone_into_deref_with_clone_method(mut a: DerefWrapperWithClone<HasCloneFrom>, b: HasCloneFrom) {
+ *a = b.clone();
+}
+
+fn clone_into_box_method(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
+ *a = b.clone();
+}
+
+fn clone_into_self_deref_method(a: &mut DerefWrapperWithClone<HasCloneFrom>, b: DerefWrapperWithClone<HasCloneFrom>) {
+ *a = b.clone();
+}
+
+fn clone_into_deref_function(mut a: DerefWrapper<HasCloneFrom>, b: HasCloneFrom) {
+ *a = Clone::clone(&b);
+}
+
+fn clone_into_box_function(mut a: Box<HasCloneFrom>, b: HasCloneFrom) {
+ *a = Clone::clone(&b);
+}
+
 // ToOwned
 fn owned_method_mut_ref(mut_string: &mut String, ref_str: &str) {
  *mut_string = ref_str.to_owned();
@@ -328,3 +354,45 @@ mod borrowck_conflicts {
  path = path.components().as_path().to_owned();
  }
 }
+
+struct DerefWrapper<T>(T);
+
+impl<T> Deref for DerefWrapper<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+}
+
+impl<T> DerefMut for DerefWrapper<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.0
+ }
+}
+
+struct DerefWrapperWithClone<T>(T);
+
+impl<T> Deref for DerefWrapperWithClone<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+}
+
+impl<T> DerefMut for DerefWrapperWithClone<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.0
+ }
+}
+
+impl<T: Clone> Clone for DerefWrapperWithClone<T> {
+ fn clone(&self) -> Self {
+ Self(self.0.clone())
+ }
+
+ fn clone_from(&mut self, source: &Self) {
+ *self = Self(source.0.clone());
+ }
+}