Skip to content

Commit

Permalink
Auto merge of #12516 - humannum14916:assigning_clones_msrv, r=Alexendoo
Browse files Browse the repository at this point in the history
Make `assigning_clones` MSRV check more precise

Continuation of #12511

`clone_into` is the only suggestion subject to the 1.63 MSRV requirement, and the lint should still emit other suggestions regardless of the MSRV.

changelog: [assigning_clones]: only apply MSRV check to `clone_into` suggestions.
  • Loading branch information
bors committed Mar 20, 2024
2 parents 89aba8d + db7c9fe commit 5b7efe8
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 24 deletions.
14 changes: 8 additions & 6 deletions clippy_lints/src/assigning_clones.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ impl_lint_pass!(AssigningClones => [ASSIGNING_CLONES]);

impl<'tcx> LateLintPass<'tcx> for AssigningClones {
fn check_expr(&mut self, cx: &LateContext<'tcx>, assign_expr: &'tcx hir::Expr<'_>) {
if !self.msrv.meets(msrvs::ASSIGNING_CLONES) {
return;
}

// Do not fire the lint in macros
let expn_data = assign_expr.span().ctxt().outer_expn_data();
match expn_data.kind {
Expand All @@ -85,7 +81,7 @@ impl<'tcx> LateLintPass<'tcx> for AssigningClones {
return;
};

if is_ok_to_suggest(cx, lhs, &call) {
if is_ok_to_suggest(cx, lhs, &call, &self.msrv) {
suggest(cx, assign_expr, lhs, &call);
}
}
Expand Down Expand Up @@ -154,7 +150,13 @@ fn extract_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<

// Return true if we find that the called method has a custom implementation and isn't derived or
// provided by default by the corresponding trait.
fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallCandidate<'tcx>) -> bool {
fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallCandidate<'tcx>, msrv: &Msrv) -> bool {
// For calls to .to_owned we suggest using .clone_into(), which was only stablilized in 1.63.
// If the current MSRV is below that, don't suggest the lint.
if !msrv.meets(msrvs::ASSIGNING_CLONES) && matches!(call.target, TargetTrait::ToOwned) {
return false;
}

// If the left-hand side is a local variable, it might be uninitialized at this point.
// In that case we do not want to suggest the lint.
if let Some(local) = path_to_local(lhs) {
Expand Down
10 changes: 6 additions & 4 deletions tests/ui/assigning_clones.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,16 @@ fn ignore_generic_clone<T: Clone>(a: &mut T, b: &T) {
}

#[clippy::msrv = "1.62"]
fn msrv_1_62(mut a: String, b: &str) {
fn msrv_1_62(mut a: String, b: String, c: &str) {
a.clone_from(&b);
// Should not be linted, as clone_into wasn't stabilized until 1.63
a = b.to_owned();
a = c.to_owned();
}

#[clippy::msrv = "1.63"]
fn msrv_1_63(mut a: String, b: &str) {
b.clone_into(&mut a);
fn msrv_1_63(mut a: String, b: String, c: &str) {
a.clone_from(&b);
c.clone_into(&mut a);
}

macro_rules! clone_inside {
Expand Down
10 changes: 6 additions & 4 deletions tests/ui/assigning_clones.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,16 @@ fn ignore_generic_clone<T: Clone>(a: &mut T, b: &T) {
}

#[clippy::msrv = "1.62"]
fn msrv_1_62(mut a: String, b: &str) {
fn msrv_1_62(mut a: String, b: String, c: &str) {
a = b.clone();
// Should not be linted, as clone_into wasn't stabilized until 1.63
a = b.to_owned();
a = c.to_owned();
}

#[clippy::msrv = "1.63"]
fn msrv_1_63(mut a: String, b: &str) {
a = b.to_owned();
fn msrv_1_63(mut a: String, b: String, c: &str) {
a = b.clone();
a = c.to_owned();
}

macro_rules! clone_inside {
Expand Down
32 changes: 22 additions & 10 deletions tests/ui/assigning_clones.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -67,47 +67,59 @@ error: assigning the result of `Clone::clone()` may be inefficient
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:133: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:140: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:139:5
--> tests/ui/assigning_clones.rs:141:5
|
LL | a = b.to_owned();
| ^^^^^^^^^^^^^^^^ help: use `clone_into()`: `b.clone_into(&mut a)`
LL | a = c.to_owned();
| ^^^^^^^^^^^^^^^^ help: use `clone_into()`: `c.clone_into(&mut a)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:156:5
--> tests/ui/assigning_clones.rs:158: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:160:5
--> tests/ui/assigning_clones.rs:162: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:181:5
--> tests/ui/assigning_clones.rs:183: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:185:5
--> tests/ui/assigning_clones.rs:187: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:189:5
--> tests/ui/assigning_clones.rs:191: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:193:5
--> tests/ui/assigning_clones.rs:195:5
|
LL | mut_thing = ToOwned::to_owned(ref_str);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, &mut mut_thing)`

error: aborting due to 18 previous errors
error: aborting due to 20 previous errors

0 comments on commit 5b7efe8

Please sign in to comment.