Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[incorrect_clone_impl_on_copy_type]: Do not lint if only has MaybeUninit fields #11089

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 39 additions & 3 deletions clippy_lints/src/incorrect_impls.rs
Original file line number Diff line number Diff line change
@@ -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};

Expand All @@ -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)]
Expand Down Expand Up @@ -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! {
Expand Down Expand Up @@ -140,7 +149,34 @@ 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<T>`
ty::Adt(_, _) if is_type_lang_item(cx, ty, LangItem::MaybeUninit) => true,
// `[MaybeUninit<T>; N]`
ty::Array(inner_ty, _) | ty::Slice(inner_ty) => {
is_type_lang_item(cx, *inner_ty, LangItem::MaybeUninit)
},
// Other cases are likely pretty rare.
_ => false,
}
})
{
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
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/incorrect_clone_impl_on_copy_type.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,27 @@ impl<A: Copy> Clone for Uwu<A> {
}

impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}

// do not lint if type has only `MaybeUninit` fields, #11072

struct G([std::mem::MaybeUninit<u32>; 100]);

impl Clone for G {
fn clone(&self) -> Self {
todo!()
}
}

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 {}
24 changes: 24 additions & 0 deletions tests/ui/incorrect_clone_impl_on_copy_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,27 @@ impl<A: Copy> Clone for Uwu<A> {
}

impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}

// do not lint if type has only `MaybeUninit` fields, #11072

struct G([std::mem::MaybeUninit<u32>; 100]);

impl Clone for G {
fn clone(&self) -> Self {
todo!()
}
}

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 {}
2 changes: 1 addition & 1 deletion tests/ui/incorrect_clone_impl_on_copy_type.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down