Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-10 07:04:18 +00:00
Code Issues Projects Releases Packages Wiki Activity

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

Browse source 
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](https://github.com/rust-lang/rust-clippy/issues/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: https://github.com/rust-lang/rust-clippy/issues/12437

r? `@blyxyas`

changelog: [`assigning_clones`]: change applicability to `Unspecified` and fix a problem with `Deref`.
This commit is contained in:
bors 2024-07-25 20:31:29 +00:00
commit 533c377950
4 changed files with 222 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
clippy_lints/src/assigning_clones.rs
View file

@ -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 {
// 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 {
// 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()

68
tests/ui/assigning_clones.fixed
View file

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

68
tests/ui/assigning_clones.rs
View file

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

86
tests/ui/assigning_clones.stderr
View file

@ -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 = <HasCloneFrom as 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: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