diff --git a/CHANGELOG.md b/CHANGELOG.md index 559b560dde4b..866a57e31284 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4396,6 +4396,7 @@ Released 2018-09-13 [`assertions_on_result_states`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_result_states [`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern [`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops +[`assigning_clones`]: https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones [`async_yields_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#async_yields_async [`await_holding_invalid_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_invalid_type [`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs new file mode 100644 index 000000000000..b884ef5285fd --- /dev/null +++ b/clippy_lints/src/assigning_clones.rs @@ -0,0 +1,204 @@ +use clippy_utils::{diagnostics::span_lint_and_then, sugg::Sugg, ty::implements_trait}; +use rustc_errors::Applicability; +use rustc_hir::{self as hir, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::sym; + +declare_clippy_lint! { + /// ### What it does + /// Checks for code like `foo = bar.clone();` + /// + /// ### Why is this bad? + /// If cloning `bar` allocates memory (or other resources), then this code will allocate + /// new memory for the clone of `bar`, then drop `foo`, then overwrite `foo` with the clone. + /// Instead, `Clone::clone_from()`, or `ToOwned::clone_into()`, may be able to update + /// `foo` in-place, reusing existing memory. + /// + /// Note that this only provides any actual improvement if the type has explicitly implemented + /// the `clone_from()` trait method, since the trait-provided implementation will just call + /// `clone()`. + /// + /// ### Example + /// ```rust + /// #[derive(Clone)] + /// struct Thing; + /// + /// pub fn assign_to_ref(a: &mut Thing, b: Thing) { + /// *a = b.clone(); + /// } + /// ``` + /// Use instead: + /// ```rust + /// #[derive(Clone)] + /// struct Thing; + /// + /// pub fn assign_to_ref(a: &mut Thing, b: Thing) { + /// a.clone_from(&b); + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub ASSIGNING_CLONES, + perf, + "assigning the result of cloning may be inefficient" +} +declare_lint_pass!(AssigningClones => [ASSIGNING_CLONES]); + +impl<'tcx> LateLintPass<'tcx> for AssigningClones { + fn check_expr(&mut self, cx: &LateContext<'tcx>, assign_expr: &'tcx hir::Expr<'_>) { + let ExprKind::Assign(assign_target, clone_call, _span) = assign_expr.kind else { return }; + // TODO: Also look for `Clone::clone` function calls, not just method calls + let ExprKind::MethodCall(method_name, clone_receiver, args, _span) = clone_call.kind else { return }; + + // Fast syntactic check: if it has args it can't be the call we are looking for, + // so we don't even need to consult the types. + if !args.is_empty() { + return; + } + + let op = if method_name.ident.name == sym::clone { + Op::Clone + } else if method_name.ident.name == sym!(to_owned) { + Op::ToOwned + } else { + return; + }; + + if ok_to_suggest(cx, op, assign_target, clone_call).is_some() { + suggest(cx, op, assign_expr, assign_target, clone_receiver); + } + } +} + +#[derive(Copy, Clone, Debug)] +enum Op { + Clone, + ToOwned, +} + +// Return `Some(())` iff we have confirmed that the call is in fact one we want to replace. +fn ok_to_suggest<'tcx>( + cx: &LateContext<'tcx>, + op: Op, + assign_target: &hir::Expr<'tcx>, + clone_call: &hir::Expr<'tcx>, +) -> Option<()> { + // Check that the call is actually a call to the trait. + // TODO: Actually we are currently just checking that the result of the call is + // a type that implements the trait, which is a bad proxy for it. + let clone_result_type = cx.typeck_results().expr_ty_adjusted(clone_call); + if !(implements_trait(cx, clone_result_type, op.expected_trait(cx)?, &[])) { + return None; + } + + // If we're assigning to a dereferenced reference, then we know the place is already valid. + // On the other hand, if the place is a variable or a Box, it might be uninitialized, + // in which case the suggestion might be wrong. + // TODO: Actually ask whether the place is uninitialized at this point, instead of + // guessing based on the syntax and type. + let ExprKind::Unary(hir::UnOp::Deref, derefed_target_expr) = assign_target.kind + else { return None }; + if !cx.typeck_results().expr_ty(derefed_target_expr).is_ref() { + return None; + } + + Some(()) +} + +fn suggest<'tcx>( + cx: &LateContext<'tcx>, + op: Op, + assign_expr: &hir::Expr<'tcx>, + assign_target: &hir::Expr<'tcx>, + clone_receiver: &hir::Expr<'tcx>, +) { + span_lint_and_then(cx, ASSIGNING_CLONES, assign_expr.span, op.message(), |diag| { + // TODO: Make this MachineApplicable once we are more certain that the method being called + // is what we think it is. + let mut applicability = Applicability::MaybeIncorrect; + + diag.span_suggestion( + assign_expr.span, + op.suggestion_msg(), + op.suggested_replacement(cx, assign_target, clone_receiver, &mut applicability), + applicability, + ); + }); +} + +impl Op { + fn expected_trait(self, cx: &LateContext<'_>) -> Option { + match self { + Op::Clone => cx.tcx.lang_items().clone_trait(), + Op::ToOwned => cx.tcx.get_diagnostic_item(sym::ToOwned), + } + } + + fn message(self) -> &'static str { + // TODO: Use the receiver type to say "is" instead of "may be" for types which + // are known to have optimizations (e.g. `String`). + match self { + Op::Clone => "assigning the result of `Clone::clone()` may be inefficient", + Op::ToOwned => "assigning the result of `ToOwned::to_owned()` may be inefficient", + } + } + + fn suggestion_msg(self) -> &'static str { + match self { + Op::Clone => "use `clone_from()`", + Op::ToOwned => "use `clone_into()`", + } + } + + fn suggested_replacement<'tcx>( + self, + cx: &LateContext<'tcx>, + assign_target: &hir::Expr<'tcx>, + clone_receiver: &hir::Expr<'tcx>, + applicability: &mut Applicability, + ) -> String { + match self { + Op::Clone => { + // The assignment LHS, which will become the receiver of the `.clone_from()` call, + // should lose one level of dereference operator since autoref takes care of that. + let target_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = assign_target.kind { + Sugg::hir_with_applicability(cx, ref_expr, "_", applicability) + } else { + Sugg::hir_with_applicability(cx, assign_target, "_", applicability) + } + .maybe_par(); + + // Determine whether we need to reference the argument to clone_from(). + let clone_receiver_type = cx.typeck_results().expr_ty(clone_receiver); + let clone_receiver_adj_type = cx.typeck_results().expr_ty_adjusted(clone_receiver); + let mut clone_source_sugg = Sugg::hir_with_applicability(cx, clone_receiver, "_", applicability); + if clone_receiver_type != clone_receiver_adj_type { + // The receiver may have been a value type, so we need to add an `&` to + // be sure the argument to clone_from will be a reference. + clone_source_sugg = clone_source_sugg.addr(); + }; + + format!("{target_sugg}.clone_from({clone_source_sugg})") + }, + Op::ToOwned => { + // If the assignment dereferences, we want the `&mut` that's getting dereferenced. + // If it doesn't, then we need to *take* a `&mut`. + // TODO: This doesn't yet handle `DerefMut` types (but it can't meet them) + let target_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = assign_target.kind { + Sugg::hir_with_applicability(cx, ref_expr, "_", applicability) + } else { + // TODO: there is not yet a test for this branch, and there cannot be + // until we remove the assigning-to-a-variable restriction. + Sugg::hir_with_applicability(cx, assign_target, "_", applicability).mut_addr() + } + .maybe_par(); + + // We are replacing `foo.to_owned()` with `foo.clone_into(...)`, so the receiver + // can stay unchanged. + let receiver_sugg = Sugg::hir_with_applicability(cx, clone_receiver, "_", applicability); + + format!("{receiver_sugg}.clone_into({target_sugg})") + }, + } + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index f24dab627809..96a025e98fb3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -43,6 +43,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX_INFO, crate::assertions_on_constants::ASSERTIONS_ON_CONSTANTS_INFO, crate::assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES_INFO, + crate::assigning_clones::ASSIGNING_CLONES_INFO, crate::async_yields_async::ASYNC_YIELDS_ASYNC_INFO, crate::attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON_INFO, crate::attrs::BLANKET_CLIPPY_RESTRICTION_LINTS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bac82eca8174..4a7caeeea295 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -73,6 +73,7 @@ mod as_conversions; mod asm_syntax; mod assertions_on_constants; mod assertions_on_result_states; +mod assigning_clones; mod async_yields_async; mod attrs; mod await_holding_invalid; @@ -960,6 +961,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule)); store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation)); store.register_early_pass(|| Box::new(suspicious_doc_comments::SuspiciousDocComments)); + store.register_late_pass(|_| Box::new(assigning_clones::AssigningClones)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/assigning_clones.fixed b/tests/ui/assigning_clones.fixed new file mode 100644 index 000000000000..fa3dd56433ef --- /dev/null +++ b/tests/ui/assigning_clones.fixed @@ -0,0 +1,96 @@ +// run-rustfix +#![allow(unused)] +#![allow(clippy::redundant_clone)] +#![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612 +#![warn(clippy::assigning_clones)] + +use std::borrow::ToOwned; +use std::ops::Add; + +#[derive(Clone)] +pub struct Thing; + +pub fn val_to_mut_ref(mut_thing: &mut Thing, value_thing: Thing) { + mut_thing.clone_from(&value_thing); +} + +pub fn ref_to_mut_ref(mut_thing: &mut Thing, ref_thing: &Thing) { + mut_thing.clone_from(ref_thing); +} + +pub fn to_op(mut_thing: &mut Thing, ref_thing: &Thing) { + // These parens should be kept as necessary for a receiver + (mut_thing + &mut Thing).clone_from(ref_thing); +} + +pub fn from_op(mut_thing: &mut Thing, ref_thing: &Thing) { + // These parens should be removed since they are not needed in a function argument + mut_thing.clone_from(ref_thing + ref_thing); +} + +pub fn assign_to_var(b: Thing) -> Thing { + let mut a = Thing; + for _ in 1..10 { + // TODO: We should rewrite this, but aren't checking whether `a` is initialized + // to be certain it's okay. + a = b.clone(); + } + a +} + +pub fn assign_to_uninit_var(b: Thing) -> Thing { + // This case CANNOT be rewritten as clone_from() and should not warn. + let mut a; + a = b.clone(); + a +} + +/// Test `ToOwned` as opposed to `Clone`. +pub fn toowned_to_mut_ref(mut_string: &mut String, ref_str: &str) { + ref_str.clone_into(mut_string); +} + +/// Don't remove the dereference operator here (as we would for clone_from); we need it. +pub fn toowned_to_derefed(mut_box_string: &mut Box, ref_str: &str) { + // TODO: This is not actually linted, but it should be. + **mut_box_string = ref_str.to_owned(); +} + +struct FakeClone; +impl FakeClone { + /// This looks just like Clone::clone but has no clone_from() + fn clone(&self) -> Self { + FakeClone + } +} + +pub fn test_fake_clone() { + let mut a = FakeClone; + let b = FakeClone; + // This should not be changed because it is not Clone::clone + a = b.clone(); +} + +fn main() {} + +/// Trait implementation to allow producing a `Thing` with a low-precedence expression. +impl Add for Thing { + type Output = Self; + fn add(self, _: Thing) -> Self { + self + } +} +/// Trait implementation to allow producing a `&Thing` with a low-precedence expression. +impl<'a> Add for &'a Thing { + type Output = Self; + fn add(self, _: &'a Thing) -> Self { + self + } +} +/// Trait implementation to allow producing a `&mut Thing` with a low-precedence expression. +impl<'a> Add for &'a mut Thing { + type Output = Self; + fn add(self, _: &'a mut Thing) -> Self { + self + } +} diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs new file mode 100644 index 000000000000..93df6ac9d0c3 --- /dev/null +++ b/tests/ui/assigning_clones.rs @@ -0,0 +1,96 @@ +// run-rustfix +#![allow(unused)] +#![allow(clippy::redundant_clone)] +#![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612 +#![warn(clippy::assigning_clones)] + +use std::borrow::ToOwned; +use std::ops::Add; + +#[derive(Clone)] +pub struct Thing; + +pub fn val_to_mut_ref(mut_thing: &mut Thing, value_thing: Thing) { + *mut_thing = value_thing.clone(); +} + +pub fn ref_to_mut_ref(mut_thing: &mut Thing, ref_thing: &Thing) { + *mut_thing = ref_thing.clone(); +} + +pub fn to_op(mut_thing: &mut Thing, ref_thing: &Thing) { + // These parens should be kept as necessary for a receiver + *(mut_thing + &mut Thing) = ref_thing.clone(); +} + +pub fn from_op(mut_thing: &mut Thing, ref_thing: &Thing) { + // These parens should be removed since they are not needed in a function argument + *mut_thing = (ref_thing + ref_thing).clone(); +} + +pub fn assign_to_var(b: Thing) -> Thing { + let mut a = Thing; + for _ in 1..10 { + // TODO: We should rewrite this, but aren't checking whether `a` is initialized + // to be certain it's okay. + a = b.clone(); + } + a +} + +pub fn assign_to_uninit_var(b: Thing) -> Thing { + // This case CANNOT be rewritten as clone_from() and should not warn. + let mut a; + a = b.clone(); + a +} + +/// Test `ToOwned` as opposed to `Clone`. +pub fn toowned_to_mut_ref(mut_string: &mut String, ref_str: &str) { + *mut_string = ref_str.to_owned(); +} + +/// Don't remove the dereference operator here (as we would for clone_from); we need it. +pub fn toowned_to_derefed(mut_box_string: &mut Box, ref_str: &str) { + // TODO: This is not actually linted, but it should be. + **mut_box_string = ref_str.to_owned(); +} + +struct FakeClone; +impl FakeClone { + /// This looks just like Clone::clone but has no clone_from() + fn clone(&self) -> Self { + FakeClone + } +} + +pub fn test_fake_clone() { + let mut a = FakeClone; + let b = FakeClone; + // This should not be changed because it is not Clone::clone + a = b.clone(); +} + +fn main() {} + +/// Trait implementation to allow producing a `Thing` with a low-precedence expression. +impl Add for Thing { + type Output = Self; + fn add(self, _: Thing) -> Self { + self + } +} +/// Trait implementation to allow producing a `&Thing` with a low-precedence expression. +impl<'a> Add for &'a Thing { + type Output = Self; + fn add(self, _: &'a Thing) -> Self { + self + } +} +/// Trait implementation to allow producing a `&mut Thing` with a low-precedence expression. +impl<'a> Add for &'a mut Thing { + type Output = Self; + fn add(self, _: &'a mut Thing) -> Self { + self + } +} diff --git a/tests/ui/assigning_clones.stderr b/tests/ui/assigning_clones.stderr new file mode 100644 index 000000000000..597b7897f824 --- /dev/null +++ b/tests/ui/assigning_clones.stderr @@ -0,0 +1,34 @@ +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:14:5 + | +LL | *mut_thing = value_thing.clone(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `mut_thing.clone_from(&value_thing)` + | + = note: `-D clippy::assigning-clones` implied by `-D warnings` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:18: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 + --> $DIR/assigning_clones.rs:23:5 + | +LL | *(mut_thing + &mut Thing) = ref_thing.clone(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `(mut_thing + &mut Thing).clone_from(ref_thing)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:28: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 `ToOwned::to_owned()` may be inefficient + --> $DIR/assigning_clones.rs:50:5 + | +LL | *mut_string = ref_str.to_owned(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(mut_string)` + +error: aborting due to 5 previous errors +