From 0656d28f6b7d04463d0d5a1d8f87ba2489648dd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 1 Jan 2024 18:51:45 +0100 Subject: [PATCH] Add `assigning_clones` lint --- clippy_lints/src/assigning_clones.rs | 312 +++++++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + tests/ui/assigning_clones.fixed | 211 ++++++++++++++++++ tests/ui/assigning_clones.rs | 211 ++++++++++++++++++ tests/ui/assigning_clones.stderr | 107 +++++++++ 6 files changed, 844 insertions(+) create mode 100644 clippy_lints/src/assigning_clones.rs create mode 100644 tests/ui/assigning_clones.fixed create mode 100644 tests/ui/assigning_clones.rs create mode 100644 tests/ui/assigning_clones.stderr diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs new file mode 100644 index 000000000..c1dd068cc --- /dev/null +++ b/clippy_lints/src/assigning_clones.rs @@ -0,0 +1,312 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::sugg::Sugg; +use clippy_utils::{is_trait_method, path_to_local}; +use rustc_errors::Applicability; +use rustc_hir::{self as hir, Expr, ExprKind, Node}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_middle::ty::{Instance, Mutability}; +use rustc_session::declare_lint_pass; +use rustc_span::def_id::DefId; +use rustc_span::symbol::sym; + +declare_clippy_lint! { + /// ### What it does + /// Checks for code like `foo = bar.clone();` + /// + /// ### Why is this bad? + /// Custom `Clone::clone_from()` or `ToOwned::clone_into` implementations allow the objects + /// to share resources and therefore avoid allocations. + /// + /// ### Example + /// ```rust + /// struct Thing; + /// + /// impl Clone for Thing { + /// fn clone(&self) -> Self { todo!() } + /// fn clone_from(&mut self, other: &Self) -> Self { todo!() } + /// } + /// + /// pub fn assign_to_ref(a: &mut Thing, b: Thing) { + /// *a = b.clone(); + /// } + /// ``` + /// Use instead: + /// ```rust + /// struct Thing; + /// impl Clone for Thing { + /// fn clone(&self) -> Self { todo!() } + /// fn clone_from(&mut self, other: &Self) -> Self { todo!() } + /// } + /// + /// pub fn assign_to_ref(a: &mut Thing, b: Thing) { + /// a.clone_from(&b); + /// } + /// ``` + #[clippy::version = "1.77.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(lhs, rhs, _span) = assign_expr.kind else { + return; + }; + + let Some(call) = extract_call(cx, rhs) else { + return; + }; + + if is_ok_to_suggest(cx, lhs, &call) { + suggest(cx, assign_expr, lhs, call); + } + } +} + +// Try to resolve the call to `Clone::clone` or `ToOwned::to_owned`. +fn extract_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + let Some(fn_def_id) = clippy_utils::fn_def_id(cx, expr) else { + return None; + }; + + // Fast paths to only check method calls without arguments or function calls with a single argument + let (target, kind, resolved_method) = match expr.kind { + ExprKind::MethodCall(path, receiver, [], _span) => { + let args = cx.typeck_results().node_args(expr.hir_id); + let resolved_method = Instance::resolve(cx.tcx, cx.param_env, fn_def_id, args); + if is_trait_method(cx, expr, sym::Clone) && path.ident.name == sym::clone { + (TargetTrait::Clone, CallKind::MethodCall { receiver }, resolved_method) + } else if is_trait_method(cx, expr, sym::ToOwned) && path.ident.name == sym!(to_owned) { + (TargetTrait::ToOwned, CallKind::MethodCall { receiver }, resolved_method) + } else { + return None; + } + }, + ExprKind::Call(function, args) if args.len() == 1 => { + let kind = cx.typeck_results().node_type(function.hir_id).kind(); + let resolved_method = match kind { + ty::FnDef(_, args) => Instance::resolve(cx.tcx, cx.param_env, fn_def_id, args), + _ => return None, + }; + if cx.tcx.is_diagnostic_item(sym::to_owned_method, fn_def_id) { + ( + TargetTrait::ToOwned, + CallKind::FunctionCall { self_arg: &args[0] }, + resolved_method, + ) + } else if let Some(trait_did) = cx.tcx.trait_of_item(fn_def_id) + && cx.tcx.is_diagnostic_item(sym::Clone, trait_did) + { + ( + TargetTrait::Clone, + CallKind::FunctionCall { self_arg: &args[0] }, + resolved_method, + ) + } else { + return None; + } + }, + _ => return None, + }; + let Ok(Some(resolved_method)) = resolved_method else { + // If we could not resolve the method, don't apply the lint + return None; + }; + + Some(CallCandidate { + target, + kind, + method_def_id: resolved_method.def_id(), + }) +} + +// 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 { + // 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) { + // TODO: This check currently bails if the local variable has no initializer. + // That is overly conservative - the lint should fire even if there was no initializer, + // but the variable has been initialized before `lhs` was evaluated. + if let Some(Node::Local(local)) = cx.tcx.opt_hir_node(cx.tcx.hir().parent_id(local)) + && local.init.is_none() + { + return false; + } + } + + let Some(impl_block) = cx.tcx.impl_of_method(call.method_def_id) else { + return false; + }; + + // If the method implementation comes from #[derive(Clone)], then don't suggest the lint. + // Automatically generated Clone impls do not override `clone_from`. + // See e.g. https://github.com/rust-lang/rust/pull/98445#issuecomment-1190681305 for more details. + if cx.tcx.is_builtin_derived(impl_block) { + return false; + } + + // Find the function for which we want to check that it is implemented. + let provided_fn = match call.target { + TargetTrait::Clone => cx.tcx.get_diagnostic_item(sym::Clone).and_then(|clone| { + cx.tcx + .provided_trait_methods(clone) + .find(|item| item.name == sym::clone_from) + }), + TargetTrait::ToOwned => cx.tcx.get_diagnostic_item(sym::ToOwned).and_then(|to_owned| { + cx.tcx + .provided_trait_methods(to_owned) + .find(|item| item.name == sym!(clone_into)) + }), + }; + let Some(provided_fn) = provided_fn else { + return false; + }; + + // Now take a look if the impl block defines an implementation for the method that we're interested + // in. If not, then we're using a default implementation, which is not interesting, so we will + // not suggest the lint. + let implemented_fns = cx.tcx.impl_item_implementor_ids(impl_block); + implemented_fns.contains_key(&provided_fn.def_id) +} + +fn suggest<'tcx>( + cx: &LateContext<'tcx>, + assign_expr: &hir::Expr<'tcx>, + lhs: &hir::Expr<'tcx>, + call: CallCandidate<'tcx>, +) { + span_lint_and_then(cx, ASSIGNING_CLONES, assign_expr.span, call.message(), |diag| { + let mut applicability = Applicability::MachineApplicable; + + diag.span_suggestion( + assign_expr.span, + call.suggestion_msg(), + call.suggested_replacement(cx, lhs, &mut applicability), + applicability, + ); + }); +} + +#[derive(Copy, Clone, Debug)] +enum CallKind<'tcx> { + MethodCall { receiver: &'tcx Expr<'tcx> }, + FunctionCall { self_arg: &'tcx Expr<'tcx> }, +} + +#[derive(Copy, Clone, Debug)] +enum TargetTrait { + Clone, + ToOwned, +} + +#[derive(Debug)] +struct CallCandidate<'tcx> { + target: TargetTrait, + kind: CallKind<'tcx>, + // DefId of the called method from an impl block that implements the target trait + method_def_id: DefId, +} + +impl<'tcx> CallCandidate<'tcx> { + fn message(&self) -> &'static str { + match self.target { + TargetTrait::Clone => "assigning the result of `Clone::clone()` may be inefficient", + TargetTrait::ToOwned => "assigning the result of `ToOwned::to_owned()` may be inefficient", + } + } + + fn suggestion_msg(&self) -> &'static str { + match self.target { + TargetTrait::Clone => "use `clone_from()`", + TargetTrait::ToOwned => "use `clone_into()`", + } + } + + fn suggested_replacement( + &self, + cx: &LateContext<'tcx>, + lhs: &hir::Expr<'tcx>, + applicability: &mut Applicability, + ) -> String { + match self.target { + TargetTrait::Clone => { + match self.kind { + CallKind::MethodCall { receiver } => { + let receiver_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind { + // `*lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)` + Sugg::hir_with_applicability(cx, ref_expr, "_", applicability) + } else { + // `lhs = self_expr.clone();` -> `lhs.clone_from(self_expr)` + Sugg::hir_with_applicability(cx, lhs, "_", applicability) + } + .maybe_par(); + + // Determine whether we need to reference the argument to clone_from(). + let clone_receiver_type = cx.typeck_results().expr_ty(receiver); + let clone_receiver_adj_type = cx.typeck_results().expr_ty_adjusted(receiver); + let mut arg_sugg = Sugg::hir_with_applicability(cx, 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. + arg_sugg = arg_sugg.addr(); + }; + + format!("{receiver_sugg}.clone_from({arg_sugg})") + }, + CallKind::FunctionCall { self_arg, .. } => { + let self_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind { + // `*lhs = Clone::clone(self_expr);` -> `Clone::clone_from(lhs, self_expr)` + Sugg::hir_with_applicability(cx, ref_expr, "_", applicability) + } else { + // `lhs = Clone::clone(self_expr);` -> `Clone::clone_from(&mut lhs, self_expr)` + Sugg::hir_with_applicability(cx, lhs, "_", applicability).mut_addr() + }; + // The RHS had to be exactly correct before the call, there is no auto-deref for function calls. + let rhs_sugg = Sugg::hir_with_applicability(cx, self_arg, "_", applicability); + + // TODO: how to get rid of the full path? Modify the function call function's (q)path? Or + // auto-import it the trait? + format!("::std::clone::Clone::clone_from({self_sugg}, {rhs_sugg})") + }, + } + }, + TargetTrait::ToOwned => { + let rhs_sugg = if let ExprKind::Unary(hir::UnOp::Deref, ref_expr) = lhs.kind { + // `*lhs = rhs.to_owned()` -> `rhs.clone_into(lhs)` + // `*lhs = ToOwned::to_owned(rhs)` -> `ToOwned::clone_into(rhs, lhs)` + let sugg = Sugg::hir_with_applicability(cx, ref_expr, "_", applicability).maybe_par(); + let inner_type = cx.typeck_results().expr_ty(ref_expr); + // If after unwrapping the dereference, the type is not a mutable reference, we add &mut to make it + // deref to a mutable reference. + if matches!(inner_type.kind(), ty::Ref(_, _, Mutability::Mut)) { + sugg + } else { + sugg.mut_addr() + } + } else { + // `lhs = rhs.to_owned()` -> `rhs.clone_into(&mut lhs)` + // `lhs = ToOwned::to_owned(rhs)` -> `ToOwned::clone_into(rhs, &mut lhs)` + Sugg::hir_with_applicability(cx, lhs, "_", applicability) + .maybe_par() + .mut_addr() + }; + + match self.kind { + CallKind::MethodCall { receiver } => { + let receiver_sugg = Sugg::hir_with_applicability(cx, receiver, "_", applicability); + format!("{receiver_sugg}.clone_into({rhs_sugg})") + }, + CallKind::FunctionCall { self_arg, .. } => { + let self_sugg = Sugg::hir_with_applicability(cx, self_arg, "_", applicability); + format!("::std::borrow::ToOwned::clone_into({self_sugg}, {rhs_sugg})") + }, + } + }, + } + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a45338615..2b324f5f9 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -47,6 +47,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 76e759683..b930175c4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -80,6 +80,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; @@ -1118,6 +1119,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(incompatible_msrv::IncompatibleMsrv::new(msrv()))); store.register_late_pass(|_| Box::new(to_string_trait_impl::ToStringTraitImpl)); store.register_early_pass(|| Box::new(multiple_bound_locations::MultipleBoundLocations)); + 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 000000000..d065c6044 --- /dev/null +++ b/tests/ui/assigning_clones.fixed @@ -0,0 +1,211 @@ +// run-rustfix +#![allow(unused)] +#![allow(clippy::redundant_clone)] +#![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612 +#![allow(clippy::needless_late_init)] +#![allow(clippy::box_collection)] +#![warn(clippy::assigning_clones)] + +use std::borrow::ToOwned; +use std::ops::{Add, Deref, DerefMut}; + +// Clone +pub struct HasCloneFrom; + +impl Clone for HasCloneFrom { + fn clone(&self) -> Self { + Self + } + fn clone_from(&mut self, source: &Self) { + *self = HasCloneFrom; + } +} + +fn clone_method_rhs_val(mut_thing: &mut HasCloneFrom, value_thing: HasCloneFrom) { + mut_thing.clone_from(&value_thing); +} + +fn clone_method_rhs_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + mut_thing.clone_from(ref_thing); +} + +fn clone_method_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) { + mut_thing.clone_from(ref_thing); +} + +fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + ::std::clone::Clone::clone_from(mut_thing, ref_thing); +} + +fn clone_function_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) { + ::std::clone::Clone::clone_from(&mut mut_thing, ref_thing); +} + +fn clone_function_through_trait(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + ::std::clone::Clone::clone_from(mut_thing, ref_thing); +} + +fn clone_function_through_type(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + ::std::clone::Clone::clone_from(mut_thing, ref_thing); +} + +fn clone_function_fully_qualified(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + ::std::clone::Clone::clone_from(mut_thing, ref_thing); +} + +fn clone_method_lhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + // These parens should be kept as necessary for a receiver + (mut_thing + &mut HasCloneFrom).clone_from(ref_thing); +} + +fn clone_method_rhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + // These parens should be removed since they are not needed in a function argument + mut_thing.clone_from(ref_thing + ref_thing); +} + +fn assign_to_init_mut_var(b: HasCloneFrom) -> HasCloneFrom { + let mut a = HasCloneFrom; + for _ in 1..10 { + a.clone_from(&b); + } + a +} + +fn assign_to_late_init_mut_var(b: HasCloneFrom) { + let mut a; + a = HasCloneFrom; + a = b.clone(); +} + +fn assign_to_uninit_var(b: HasCloneFrom) { + let a; + a = b.clone(); +} + +fn assign_to_uninit_mut_var(b: HasCloneFrom) { + let mut a; + a = b.clone(); +} + +#[derive(Clone)] +pub struct HasDeriveClone; + +fn ignore_derive_clone(a: &mut HasDeriveClone, b: &HasDeriveClone) { + // Should not be linted, since the Clone impl is derived + *a = b.clone(); +} + +pub struct HasCloneImpl; + +impl Clone for HasCloneImpl { + fn clone(&self) -> Self { + Self + } +} + +fn ignore_missing_clone_from(a: &mut HasCloneImpl, b: &HasCloneImpl) { + // Should not be linted, since the Clone impl doesn't override clone_from + *a = b.clone(); +} + +struct FakeClone; + +impl FakeClone { + /// This looks just like `Clone::clone` + fn clone(&self) -> Self { + FakeClone + } +} + +fn ignore_fake_clone() { + let mut a = FakeClone; + let b = FakeClone; + // Should not be linted, since the Clone impl doesn't come from std + a = b.clone(); +} + +fn ignore_generic_clone(a: &mut T, b: &T) { + // Should not be linted, since we don't know the actual clone impl + *a = b.clone(); +} + +// ToOwned +fn owned_method_mut_ref(mut_string: &mut String, ref_str: &str) { + ref_str.clone_into(mut_string); +} + +fn owned_method_val(mut mut_string: String, ref_str: &str) { + ref_str.clone_into(&mut mut_string); +} + +struct HasDeref { + a: String +} + +impl Deref for HasDeref { + type Target = String; + fn deref(&self) -> &Self::Target { + &self.a + } +} + +impl DerefMut for HasDeref { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.a + } +} + +fn owned_method_box(mut_box_string: &mut Box, ref_str: &str) { + ref_str.clone_into(&mut (*mut_box_string)); +} + +fn owned_method_deref(mut_box_string: &mut HasDeref, ref_str: &str) { + ref_str.clone_into(&mut (*mut_box_string)); +} + +fn owned_function_mut_ref(mut_thing: &mut String, ref_str: &str) { + ::std::borrow::ToOwned::clone_into(ref_str, mut_thing); +} + +fn owned_function_val(mut mut_thing: String, ref_str: &str) { + ::std::borrow::ToOwned::clone_into(ref_str, &mut mut_thing); +} + +struct FakeToOwned; +impl FakeToOwned { + /// This looks just like `ToOwned::to_owned` + fn to_owned(&self) -> Self { + FakeToOwned + } +} + +fn fake_to_owned() { + let mut a = FakeToOwned; + let b = FakeToOwned; + // Should not be linted, since the ToOwned impl doesn't come from std + a = b.to_owned(); +} + +fn main() {} + +/// Trait implementation to allow producing a `Thing` with a low-precedence expression. +impl Add for HasCloneFrom { + type Output = Self; + fn add(self, _: HasCloneFrom) -> Self { + self + } +} +/// Trait implementation to allow producing a `&Thing` with a low-precedence expression. +impl<'a> Add for &'a HasCloneFrom { + type Output = Self; + fn add(self, _: &'a HasCloneFrom) -> Self { + self + } +} +/// Trait implementation to allow producing a `&mut Thing` with a low-precedence expression. +impl<'a> Add for &'a mut HasCloneFrom { + type Output = Self; + fn add(self, _: &'a mut HasCloneFrom) -> Self { + self + } +} diff --git a/tests/ui/assigning_clones.rs b/tests/ui/assigning_clones.rs new file mode 100644 index 000000000..f1ee202c5 --- /dev/null +++ b/tests/ui/assigning_clones.rs @@ -0,0 +1,211 @@ +// run-rustfix +#![allow(unused)] +#![allow(clippy::redundant_clone)] +#![allow(clippy::ptr_arg)] // https://github.com/rust-lang/rust-clippy/issues/10612 +#![allow(clippy::needless_late_init)] +#![allow(clippy::box_collection)] +#![warn(clippy::assigning_clones)] + +use std::borrow::ToOwned; +use std::ops::{Add, Deref, DerefMut}; + +// Clone +pub struct HasCloneFrom; + +impl Clone for HasCloneFrom { + fn clone(&self) -> Self { + Self + } + fn clone_from(&mut self, source: &Self) { + *self = HasCloneFrom; + } +} + +fn clone_method_rhs_val(mut_thing: &mut HasCloneFrom, value_thing: HasCloneFrom) { + *mut_thing = value_thing.clone(); +} + +fn clone_method_rhs_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + *mut_thing = ref_thing.clone(); +} + +fn clone_method_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) { + mut_thing = ref_thing.clone(); +} + +fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + *mut_thing = Clone::clone(ref_thing); +} + +fn clone_function_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) { + mut_thing = Clone::clone(ref_thing); +} + +fn clone_function_through_trait(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + *mut_thing = Clone::clone(ref_thing); +} + +fn clone_function_through_type(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + *mut_thing = HasCloneFrom::clone(ref_thing); +} + +fn clone_function_fully_qualified(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + *mut_thing = ::clone(ref_thing); +} + +fn clone_method_lhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + // These parens should be kept as necessary for a receiver + *(mut_thing + &mut HasCloneFrom) = ref_thing.clone(); +} + +fn clone_method_rhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) { + // These parens should be removed since they are not needed in a function argument + *mut_thing = (ref_thing + ref_thing).clone(); +} + +fn assign_to_init_mut_var(b: HasCloneFrom) -> HasCloneFrom { + let mut a = HasCloneFrom; + for _ in 1..10 { + a = b.clone(); + } + a +} + +fn assign_to_late_init_mut_var(b: HasCloneFrom) { + let mut a; + a = HasCloneFrom; + a = b.clone(); +} + +fn assign_to_uninit_var(b: HasCloneFrom) { + let a; + a = b.clone(); +} + +fn assign_to_uninit_mut_var(b: HasCloneFrom) { + let mut a; + a = b.clone(); +} + +#[derive(Clone)] +pub struct HasDeriveClone; + +fn ignore_derive_clone(a: &mut HasDeriveClone, b: &HasDeriveClone) { + // Should not be linted, since the Clone impl is derived + *a = b.clone(); +} + +pub struct HasCloneImpl; + +impl Clone for HasCloneImpl { + fn clone(&self) -> Self { + Self + } +} + +fn ignore_missing_clone_from(a: &mut HasCloneImpl, b: &HasCloneImpl) { + // Should not be linted, since the Clone impl doesn't override clone_from + *a = b.clone(); +} + +struct FakeClone; + +impl FakeClone { + /// This looks just like `Clone::clone` + fn clone(&self) -> Self { + FakeClone + } +} + +fn ignore_fake_clone() { + let mut a = FakeClone; + let b = FakeClone; + // Should not be linted, since the Clone impl doesn't come from std + a = b.clone(); +} + +fn ignore_generic_clone(a: &mut T, b: &T) { + // Should not be linted, since we don't know the actual clone impl + *a = b.clone(); +} + +// ToOwned +fn owned_method_mut_ref(mut_string: &mut String, ref_str: &str) { + *mut_string = ref_str.to_owned(); +} + +fn owned_method_val(mut mut_string: String, ref_str: &str) { + mut_string = ref_str.to_owned(); +} + +struct HasDeref { + a: String, +} + +impl Deref for HasDeref { + type Target = String; + fn deref(&self) -> &Self::Target { + &self.a + } +} + +impl DerefMut for HasDeref { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.a + } +} + +fn owned_method_box(mut_box_string: &mut Box, ref_str: &str) { + **mut_box_string = ref_str.to_owned(); +} + +fn owned_method_deref(mut_box_string: &mut HasDeref, ref_str: &str) { + **mut_box_string = ref_str.to_owned(); +} + +fn owned_function_mut_ref(mut_thing: &mut String, ref_str: &str) { + *mut_thing = ToOwned::to_owned(ref_str); +} + +fn owned_function_val(mut mut_thing: String, ref_str: &str) { + mut_thing = ToOwned::to_owned(ref_str); +} + +struct FakeToOwned; +impl FakeToOwned { + /// This looks just like `ToOwned::to_owned` + fn to_owned(&self) -> Self { + FakeToOwned + } +} + +fn fake_to_owned() { + let mut a = FakeToOwned; + let b = FakeToOwned; + // Should not be linted, since the ToOwned impl doesn't come from std + a = b.to_owned(); +} + +fn main() {} + +/// Trait implementation to allow producing a `Thing` with a low-precedence expression. +impl Add for HasCloneFrom { + type Output = Self; + fn add(self, _: HasCloneFrom) -> Self { + self + } +} +/// Trait implementation to allow producing a `&Thing` with a low-precedence expression. +impl<'a> Add for &'a HasCloneFrom { + type Output = Self; + fn add(self, _: &'a HasCloneFrom) -> Self { + self + } +} +/// Trait implementation to allow producing a `&mut Thing` with a low-precedence expression. +impl<'a> Add for &'a mut HasCloneFrom { + type Output = Self; + fn add(self, _: &'a mut HasCloneFrom) -> Self { + self + } +} diff --git a/tests/ui/assigning_clones.stderr b/tests/ui/assigning_clones.stderr new file mode 100644 index 000000000..3b84406a7 --- /dev/null +++ b/tests/ui/assigning_clones.stderr @@ -0,0 +1,107 @@ +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:25: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` + = help: to override `-D warnings` add `#[allow(clippy::assigning_clones)]` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/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 + --> $DIR/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 + --> $DIR/assigning_clones.rs:37:5 + | +LL | *mut_thing = Clone::clone(ref_thing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:41:5 + | +LL | mut_thing = Clone::clone(ref_thing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(&mut mut_thing, ref_thing)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:45:5 + | +LL | *mut_thing = Clone::clone(ref_thing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:49:5 + | +LL | *mut_thing = HasCloneFrom::clone(ref_thing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/assigning_clones.rs:53:5 + | +LL | *mut_thing = ::clone(ref_thing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)` + +error: assigning the result of `Clone::clone()` may be inefficient + --> $DIR/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 + --> $DIR/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 + --> $DIR/assigning_clones.rs:69:9 + | +LL | a = b.clone(); + | ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)` + +error: assigning the result of `ToOwned::to_owned()` may be inefficient + --> $DIR/assigning_clones.rs:134: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 + --> $DIR/assigning_clones.rs:138: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 + --> $DIR/assigning_clones.rs:159: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 + --> $DIR/assigning_clones.rs:163: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 + --> $DIR/assigning_clones.rs:167:5 + | +LL | *mut_thing = ToOwned::to_owned(ref_str); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `::std::borrow::ToOwned::clone_into(ref_str, mut_thing)` + +error: assigning the result of `ToOwned::to_owned()` may be inefficient + --> $DIR/assigning_clones.rs:171:5 + | +LL | mut_thing = ToOwned::to_owned(ref_str); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `::std::borrow::ToOwned::clone_into(ref_str, &mut mut_thing)` + +error: aborting due to 17 previous errors +