Add lint assigning_clones.

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

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<hir::def_id::DefId> {
+        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})")
+            },
+        }
+    }
+}

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,

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`
 }
 

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<String>, 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
+    }
+}