From 53d9f64f664a8d5a42d8298abc68baa5aac30578 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Mon, 3 Jul 2023 09:14:13 -0500 Subject: [PATCH 1/2] Do not lint if only has `MaybeUninit` fields --- clippy_lints/src/incorrect_impls.rs | 39 +++++++++++++++++-- .../incorrect_clone_impl_on_copy_type.fixed | 11 ++++++ tests/ui/incorrect_clone_impl_on_copy_type.rs | 11 ++++++ .../incorrect_clone_impl_on_copy_type.stderr | 2 +- 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs index 25e1cb36b74a..245420746fa8 100644 --- a/clippy_lints/src/incorrect_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -1,13 +1,14 @@ use clippy_utils::{ diagnostics::{span_lint_and_sugg, span_lint_and_then}, get_parent_node, is_res_lang_ctor, last_path_segment, path_res, - ty::implements_trait, + ty::{implements_trait, is_type_lang_item}, }; +use itertools::Itertools; use rustc_errors::Applicability; use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, UnOp}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::EarlyBinder; +use rustc_middle::ty::{self, EarlyBinder}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{sym, symbol::kw}; @@ -19,6 +20,14 @@ declare_clippy_lint! { /// If both `Clone` and `Copy` are implemented, they must agree. This is done by dereferencing /// `self` in `Clone`'s implementation. Anything else is incorrect. /// + /// ### Known issues + /// While anything other than `*self` is *technically* incorrect, it can often be done as an + /// optimization, like in the case of `MaybeUninit` for example. Returning a new `MaybeUninit` + /// is both faster and as correct as `memcpy`ing the original. If this is not the case however, + /// the lint's advice should almost always be applied. + /// + /// Note: This lint ignores `Clone` implementations on types that are just `MaybeUninit`. + /// /// ### Example /// ```rust,ignore /// #[derive(Eq, PartialEq)] @@ -47,7 +56,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.72.0"] pub INCORRECT_CLONE_IMPL_ON_COPY_TYPE, - correctness, + complexity, "manual implementation of `Clone` on a `Copy` type" } declare_clippy_lint! { @@ -140,7 +149,31 @@ impl LateLintPass<'_> for IncorrectImpls { copy_def_id, &[], ) + && let ty::Adt(def, substs) = hir_ty_to_ty(cx.tcx, imp.self_ty).kind() { + let fields = def.all_fields().collect_vec(); + // Necessary as `all` returns true on an empty iterator + if !fields.is_empty() + && fields.iter().all(|field| { + let ty = field.ty(cx.tcx, substs); + + match ty.kind() { + // `MaybeUninit` + ty::Adt(_, _) if is_type_lang_item(cx, ty, LangItem::MaybeUninit) => true, + // `[MaybeUninit; N]` + ty::Array(inner_ty, _) | ty::Slice(inner_ty) + if is_type_lang_item(cx, *inner_ty, LangItem::MaybeUninit) => + { + true + }, + // Other cases are likely pretty rare. + _ => false, + } + }) + { + return; + } + if impl_item.ident.name == sym::clone { if block.stmts.is_empty() && let Some(expr) = block.expr diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.fixed b/tests/ui/incorrect_clone_impl_on_copy_type.fixed index ac482dcda1ee..446873b31872 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.fixed +++ b/tests/ui/incorrect_clone_impl_on_copy_type.fixed @@ -95,3 +95,14 @@ impl Clone for Uwu { } impl Copy for Uwu {} + +// do not lint if type has only `MaybeUninit` fields +struct G([std::mem::MaybeUninit; 100]); + +impl Clone for G { + fn clone(&self) -> Self { + todo!() + } +} + +impl Copy for G {} diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.rs b/tests/ui/incorrect_clone_impl_on_copy_type.rs index 00775874ff58..d8be3569af27 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.rs +++ b/tests/ui/incorrect_clone_impl_on_copy_type.rs @@ -105,3 +105,14 @@ impl Clone for Uwu { } impl Copy for Uwu {} + +// do not lint if type has only `MaybeUninit` fields +struct G([std::mem::MaybeUninit; 100]); + +impl Clone for G { + fn clone(&self) -> Self { + todo!() + } +} + +impl Copy for G {} diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.stderr b/tests/ui/incorrect_clone_impl_on_copy_type.stderr index 7bcba8ba45a2..ec0890f9ed6d 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.stderr +++ b/tests/ui/incorrect_clone_impl_on_copy_type.stderr @@ -7,7 +7,7 @@ LL | | Self(self.0) LL | | } | |_____^ help: change this to: `{ *self }` | - = note: `#[deny(clippy::incorrect_clone_impl_on_copy_type)]` on by default + = note: `-D clippy::incorrect-clone-impl-on-copy-type` implied by `-D warnings` error: incorrect implementation of `clone_from` on a `Copy` type --> $DIR/incorrect_clone_impl_on_copy_type.rs:14:5 From ff923b0437e6c599e1f681bb2c504f19f3f3ef98 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Mon, 3 Jul 2023 09:39:47 -0500 Subject: [PATCH 2/2] Do not lint `never`-like enums --- clippy_lints/src/incorrect_impls.rs | 11 +++++++---- tests/ui/incorrect_clone_impl_on_copy_type.fixed | 15 ++++++++++++++- tests/ui/incorrect_clone_impl_on_copy_type.rs | 15 ++++++++++++++- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs index 245420746fa8..b9fa91a347c9 100644 --- a/clippy_lints/src/incorrect_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -161,10 +161,8 @@ impl LateLintPass<'_> for IncorrectImpls { // `MaybeUninit` ty::Adt(_, _) if is_type_lang_item(cx, ty, LangItem::MaybeUninit) => true, // `[MaybeUninit; N]` - ty::Array(inner_ty, _) | ty::Slice(inner_ty) - if is_type_lang_item(cx, *inner_ty, LangItem::MaybeUninit) => - { - true + ty::Array(inner_ty, _) | ty::Slice(inner_ty) => { + is_type_lang_item(cx, *inner_ty, LangItem::MaybeUninit) }, // Other cases are likely pretty rare. _ => false, @@ -174,6 +172,11 @@ impl LateLintPass<'_> for IncorrectImpls { return; } + // Skip `never`-like enums + if def.is_enum() && def.variants().is_empty() { + return; + } + if impl_item.ident.name == sym::clone { if block.stmts.is_empty() && let Some(expr) = block.expr diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.fixed b/tests/ui/incorrect_clone_impl_on_copy_type.fixed index 446873b31872..1f0836766705 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.fixed +++ b/tests/ui/incorrect_clone_impl_on_copy_type.fixed @@ -96,7 +96,8 @@ impl Clone for Uwu { impl Copy for Uwu {} -// do not lint if type has only `MaybeUninit` fields +// do not lint if type has only `MaybeUninit` fields, #11072 + struct G([std::mem::MaybeUninit; 100]); impl Clone for G { @@ -106,3 +107,15 @@ impl Clone for G { } impl Copy for G {} + +// do not lint `never`-like enums, #11071 + +enum H {} + +impl Clone for H { + fn clone(&self) -> Self { + todo!() + } +} + +impl Copy for H {} diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.rs b/tests/ui/incorrect_clone_impl_on_copy_type.rs index d8be3569af27..845e03aa3d30 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.rs +++ b/tests/ui/incorrect_clone_impl_on_copy_type.rs @@ -106,7 +106,8 @@ impl Clone for Uwu { impl Copy for Uwu {} -// do not lint if type has only `MaybeUninit` fields +// do not lint if type has only `MaybeUninit` fields, #11072 + struct G([std::mem::MaybeUninit; 100]); impl Clone for G { @@ -116,3 +117,15 @@ impl Clone for G { } impl Copy for G {} + +// do not lint `never`-like enums, #11071 + +enum H {} + +impl Clone for H { + fn clone(&self) -> Self { + todo!() + } +} + +impl Copy for H {}