diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs index 03f777600..6e336efbb 100644 --- a/clippy_lints/src/assigning_clones.rs +++ b/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() diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed index 60f6a385f..b376d55a4 100644 --- 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; @@ -182,6 +183,31 @@ impl Clone for AvoidRecursiveCloneFrom { } } +// Deref handling +fn clone_into_deref_method(mut a: DerefWrapper, b: HasCloneFrom) { + (*a).clone_from(&b); +} + +fn clone_into_deref_with_clone_method(mut a: DerefWrapperWithClone, b: HasCloneFrom) { + (*a).clone_from(&b); +} + +fn clone_into_box_method(mut a: Box, b: HasCloneFrom) { + (*a).clone_from(&b); +} + +fn clone_into_self_deref_method(a: &mut DerefWrapperWithClone, b: DerefWrapperWithClone) { + a.clone_from(&b); +} + +fn clone_into_deref_function(mut a: DerefWrapper, b: HasCloneFrom) { + Clone::clone_from(&mut *a, &b); +} + +fn clone_into_box_function(mut a: Box, 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); + +impl Deref for DerefWrapper { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for DerefWrapper { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +struct DerefWrapperWithClone(T); + +impl Deref for DerefWrapperWithClone { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for DerefWrapperWithClone { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl Clone for DerefWrapperWithClone { + 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 index 6eb85be51..11a5d4459 100644 --- 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, b: HasCloneFrom) { + *a = b.clone(); +} + +fn clone_into_deref_with_clone_method(mut a: DerefWrapperWithClone, b: HasCloneFrom) { + *a = b.clone(); +} + +fn clone_into_box_method(mut a: Box, b: HasCloneFrom) { + *a = b.clone(); +} + +fn clone_into_self_deref_method(a: &mut DerefWrapperWithClone, b: DerefWrapperWithClone) { + *a = b.clone(); +} + +fn clone_into_deref_function(mut a: DerefWrapper, b: HasCloneFrom) { + *a = Clone::clone(&b); +} + +fn clone_into_box_function(mut a: Box, 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); + +impl Deref for DerefWrapper { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for DerefWrapper { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +struct DerefWrapperWithClone(T); + +impl Deref for DerefWrapperWithClone { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for DerefWrapperWithClone { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl Clone for DerefWrapperWithClone { + 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.stderr b/tests/ui/assigning_clones.stderr index a68516376..19724a6d4 100644 --- a/tests/ui/assigning_clones.stderr +++ b/tests/ui/assigning_clones.stderr @@ -1,5 +1,5 @@ error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:24:5 + --> tests/ui/assigning_clones.rs:25:5 | LL | *mut_thing = value_thing.clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `mut_thing.clone_from(&value_thing)` @@ -8,142 +8,178 @@ LL | *mut_thing = value_thing.clone(); = help: to override `-D warnings` add `#[allow(clippy::assigning_clones)]` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:28:5 + --> tests/ui/assigning_clones.rs:29:5 | LL | *mut_thing = ref_thing.clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `mut_thing.clone_from(ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:32:5 + --> tests/ui/assigning_clones.rs:33:5 | LL | mut_thing = ref_thing.clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `mut_thing.clone_from(ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:36:5 + --> tests/ui/assigning_clones.rs:37:5 | LL | *mut_thing = Clone::clone(ref_thing); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:40:5 + --> tests/ui/assigning_clones.rs:41:5 | LL | mut_thing = Clone::clone(ref_thing); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(&mut mut_thing, ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:44:5 + --> tests/ui/assigning_clones.rs:45:5 | LL | *mut_thing = Clone::clone(ref_thing); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:48:5 + --> tests/ui/assigning_clones.rs:49:5 | LL | *mut_thing = HasCloneFrom::clone(ref_thing); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:52:5 + --> tests/ui/assigning_clones.rs:53:5 | LL | *mut_thing = ::clone(ref_thing); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:57:5 + --> tests/ui/assigning_clones.rs:58:5 | LL | *(mut_thing + &mut HasCloneFrom) = ref_thing.clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `(mut_thing + &mut HasCloneFrom).clone_from(ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:62:5 + --> tests/ui/assigning_clones.rs:63:5 | LL | *mut_thing = (ref_thing + ref_thing).clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `mut_thing.clone_from(ref_thing + ref_thing)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:67:5 + --> tests/ui/assigning_clones.rs:68:5 | LL | s = format!("{} {}", "hello", "world").clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `s.clone_from(&format!("{} {}", "hello", "world"))` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:72:5 + --> tests/ui/assigning_clones.rs:73:5 | LL | s = Clone::clone(&format!("{} {}", "hello", "world")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(&mut s, &format!("{} {}", "hello", "world"))` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:78:9 + --> tests/ui/assigning_clones.rs:79:9 | LL | a = b.clone(); | ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:149:5 + --> tests/ui/assigning_clones.rs:150:5 | LL | a = b.clone(); | ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)` error: assigning the result of `Clone::clone()` may be inefficient - --> tests/ui/assigning_clones.rs:156:5 + --> tests/ui/assigning_clones.rs:157:5 | LL | a = b.clone(); | ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:157:5 + --> tests/ui/assigning_clones.rs:158:5 | LL | a = c.to_owned(); | ^^^^^^^^^^^^^^^^ help: use `clone_into()`: `c.clone_into(&mut a)` +error: assigning the result of `Clone::clone()` may be inefficient + --> tests/ui/assigning_clones.rs:188:5 + | +LL | *a = b.clone(); + | ^^^^^^^^^^^^^^ help: use `clone_from()`: `(*a).clone_from(&b)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> tests/ui/assigning_clones.rs:192:5 + | +LL | *a = b.clone(); + | ^^^^^^^^^^^^^^ help: use `clone_from()`: `(*a).clone_from(&b)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> tests/ui/assigning_clones.rs:196:5 + | +LL | *a = b.clone(); + | ^^^^^^^^^^^^^^ help: use `clone_from()`: `(*a).clone_from(&b)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> tests/ui/assigning_clones.rs:200:5 + | +LL | *a = b.clone(); + | ^^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> tests/ui/assigning_clones.rs:204:5 + | +LL | *a = Clone::clone(&b); + | ^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(&mut *a, &b)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> tests/ui/assigning_clones.rs:208:5 + | +LL | *a = Clone::clone(&b); + | ^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(&mut *a, &b)` + error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:187:5 + --> tests/ui/assigning_clones.rs:213:5 | LL | *mut_string = ref_str.to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(mut_string)` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:191:5 + --> tests/ui/assigning_clones.rs:217:5 | LL | mut_string = ref_str.to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(&mut mut_string)` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:212:5 + --> tests/ui/assigning_clones.rs:238:5 | LL | **mut_box_string = ref_str.to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(&mut (*mut_box_string))` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:216:5 + --> tests/ui/assigning_clones.rs:242:5 | LL | **mut_box_string = ref_str.to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(&mut (*mut_box_string))` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:220:5 + --> tests/ui/assigning_clones.rs:246:5 | LL | *mut_thing = ToOwned::to_owned(ref_str); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, mut_thing)` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:224:5 + --> tests/ui/assigning_clones.rs:250:5 | LL | mut_thing = ToOwned::to_owned(ref_str); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, &mut mut_thing)` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:229:5 + --> tests/ui/assigning_clones.rs:255:5 | LL | s = format!("{} {}", "hello", "world").to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `format!("{} {}", "hello", "world").clone_into(&mut s)` error: assigning the result of `ToOwned::to_owned()` may be inefficient - --> tests/ui/assigning_clones.rs:234:5 + --> tests/ui/assigning_clones.rs:260:5 | LL | s = ToOwned::to_owned(&format!("{} {}", "hello", "world")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(&format!("{} {}", "hello", "world"), &mut s)` -error: aborting due to 24 previous errors +error: aborting due to 30 previous errors