From 5f7b3c5eea02b697a0a45ba969b7b42b10aed528 Mon Sep 17 00:00:00 2001 From: humannum14916 Date: Wed, 20 Mar 2024 14:04:12 -0400 Subject: [PATCH 1/2] Only enforce MSRV check on .clone_into() suggestions --- clippy_lints/src/assigning_clones.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs index ea5578572ea7..88d9f762a87b 100644 --- a/clippy_lints/src/assigning_clones.rs +++ b/clippy_lints/src/assigning_clones.rs @@ -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 { @@ -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); } } @@ -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) { From db7c9feaa00dcc5adcbf6d345ddcfa54bcf3debf Mon Sep 17 00:00:00 2001 From: humannum14916 Date: Wed, 20 Mar 2024 14:07:53 -0400 Subject: [PATCH 2/2] Add UI test for new MSRV check --- tests/ui/assigning_clones.fixed | 10 ++++++---- tests/ui/assigning_clones.rs | 10 ++++++---- tests/ui/assigning_clones.stderr | 32 ++++++++++++++++++++++---------- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed index 771c7f6f56a1..160f3b946631 100644 --- a/tests/ui/assigning_clones.fixed +++ b/tests/ui/assigning_clones.fixed @@ -129,14 +129,16 @@ fn ignore_generic_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 { diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs index 1320481322f6..14ba1d4db9a8 100644 --- a/tests/ui/assigning_clones.rs +++ b/tests/ui/assigning_clones.rs @@ -129,14 +129,16 @@ fn ignore_generic_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 { diff --git a/tests/ui/assigning_clones.stderr b/tests/ui/assigning_clones.stderr index b8e20cee1602..ba59f067431a 100644 --- a/tests/ui/assigning_clones.stderr +++ b/tests/ui/assigning_clones.stderr @@ -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