Auto merge of #12473 - Kobzol:assigning-clones-deref, r=Alexendoo

Fix handling of `Deref` in `assigning_clones`

The `assigning_clones` lint had a special case for producing a bit nicer code for mutable references:
```rust
fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
    *mut_thing = Clone::clone(ref_thing);
}
//v
fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
    Clone::clone_from(mut_thing, ref_thing);
}
```
However, this could [break](#12437) when combined with `Deref`.

This PR removes the special case, so that the generated code should work more generally. Later we can improve the detection of `Deref` and put the special case back in a way that does not break code.

Fixes: #12437

r? `@blyxyas`

changelog: [`assigning_clones`]: change applicability to `Unspecified` and fix a problem with `Deref`.

Author: bors

clippy_lints/src/assigning_clones.rs

@@ -227,9 +227,22 @@ fn build_sugg<'tcx>(
             match call_kind {
                 CallKind::Method => {
                     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, "_", app)
+                        // If `ref_expr` is a reference, we can remove the dereference operator (`*`) to make
+                        // the generated code a bit simpler. In other cases, we don't do this special case, to avoid
+                        // having to deal with Deref (https://github.com/rust-lang/rust-clippy/issues/12437).
+
+                        let ty = cx.typeck_results().expr_ty(ref_expr);
+                        if ty.is_ref() {
+                            // Apply special case, remove `*`
+                            // `*lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
+                            Sugg::hir_with_applicability(cx, ref_expr, "_", app)
+                        } else {
+                            // Keep the original lhs
+                            // `*lhs = self_expr.clone();` -> `(*lhs).clone_from(self_expr)`
+                            Sugg::hir_with_applicability(cx, lhs, "_", app)
+                        }
                     } else {
+                        // Keep the original lhs
                         // `lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)`
                         Sugg::hir_with_applicability(cx, lhs, "_", app)
                     }
@@ -249,8 +262,16 @@ fn build_sugg<'tcx>(
                 },
                 CallKind::Ufcs => {
                     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, "_", app)
+                        // See special case of removing `*` in method handling above
+                        let ty = cx.typeck_results().expr_ty(ref_expr);
+                        if ty.is_ref() {
+                            // `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(lhs, self_expr)`
+                            Sugg::hir_with_applicability(cx, ref_expr, "_", app)
+                        } else {
+                            // `*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, "_", app).mut_addr_deref()
+                        }
                     } else {
                         // `lhs = Clone::clone(self_expr);` -> `Clone::clone_from(&mut lhs, self_expr)`
                         Sugg::hir_with_applicability(cx, lhs, "_", app).mut_addr()

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;
@@ -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());
+    }
+}

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());
+    }
+}