diff --git a/CHANGELOG.md b/CHANGELOG.md index 218bca40cb0..c0925988ca1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5005,6 +5005,7 @@ Released 2018-09-13 [`needless_bool_assign`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_bool_assign [`needless_borrow`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow [`needless_borrowed_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference +[`needless_clone_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_clone_impl [`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect [`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 9509e6d016a..1fd70fce14f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -463,6 +463,7 @@ crate::needless_else::NEEDLESS_ELSE_INFO, crate::needless_for_each::NEEDLESS_FOR_EACH_INFO, crate::needless_if::NEEDLESS_IF_INFO, + crate::needless_impls::NEEDLESS_CLONE_IMPL_INFO, crate::needless_late_init::NEEDLESS_LATE_INIT_INFO, crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO, crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cf6499b9616..e07959849a3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -226,6 +226,7 @@ mod needless_else; mod needless_for_each; mod needless_if; +mod needless_impls; mod needless_late_init; mod needless_parens_on_range_literals; mod needless_pass_by_value; @@ -1047,6 +1048,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let stack_size_threshold = conf.stack_size_threshold; store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(stack_size_threshold))); store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit)); + store.register_late_pass(|_| Box::new(needless_impls::NeedlessImpls)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/needless_impls.rs b/clippy_lints/src/needless_impls.rs new file mode 100644 index 00000000000..ccfd55886ec --- /dev/null +++ b/clippy_lints/src/needless_impls.rs @@ -0,0 +1,109 @@ +use clippy_utils::{diagnostics::span_lint_and_sugg, get_parent_node, last_path_segment, ty::implements_trait}; +use rustc_errors::Applicability; +use rustc_hir::{ExprKind, ImplItem, ImplItemKind, ItemKind, Node, UnOp}; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::EarlyBinder; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{sym, symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for manual implementations of `Clone` when `Copy` is already implemented. + /// + /// ### Why is this bad? + /// If both `Clone` and `Copy` are implemented, they must agree. This is done by dereferencing + /// `self` in `Clone`'s implementation. Anything else is incorrect. + /// + /// ### Example + /// ```rust,ignore + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Clone for A { + /// fn clone(&self) -> Self { + /// Self(self.0) + /// } + /// } + /// + /// impl Copy for A {} + /// ``` + /// Use instead: + /// ```rust,ignore + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Clone for A { + /// fn clone(&self) -> Self { + /// *self + /// } + /// } + /// + /// impl Copy for A {} + /// ``` + #[clippy::version = "1.72.0"] + pub NEEDLESS_CLONE_IMPL, + correctness, + "manual implementation of `Clone` on a `Copy` type" +} +declare_lint_pass!(NeedlessImpls => [NEEDLESS_CLONE_IMPL]); + +impl LateLintPass<'_> for NeedlessImpls { + #[expect(clippy::needless_return)] + fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { + let node = get_parent_node(cx.tcx, impl_item.hir_id()); + let Some(Node::Item(item)) = node else { + return; + }; + let ItemKind::Impl(imp) = item.kind else { + return; + }; + let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else { + return; + }; + let trait_impl_def_id = trait_impl.def_id; + if cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) { + return; + } + let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir().impl_item(impl_item.impl_item_id()).kind else { + return; + }; + let body = cx.tcx.hir().body(impl_item_id); + let ExprKind::Block(block, ..) = body.value.kind else { + return; + }; + // Above is duplicated from the `duplicate_manual_partial_ord_impl` branch. + // Remove it while solving conflicts once that PR is merged. + + // Actual implementation; remove this comment once aforementioned PR is merged + if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl_def_id) + && impl_item.ident.name == sym::clone + && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) + && implements_trait( + cx, + hir_ty_to_ty(cx.tcx, imp.self_ty), + copy_def_id, + trait_impl.substs, + ) + { + if block.stmts.is_empty() + && let Some(expr) = block.expr + && let ExprKind::Unary(UnOp::Deref, inner) = expr.kind + && let ExprKind::Path(qpath) = inner.kind + && last_path_segment(&qpath).ident.name == symbol::kw::SelfLower + {} else { + span_lint_and_sugg( + cx, + NEEDLESS_CLONE_IMPL, + block.span, + "manual implementation of `Clone` on a `Copy` type", + "change this to", + "{ *self }".to_owned(), + Applicability::Unspecified, + ); + + return; + } + } + } +} diff --git a/tests/ui/clone_on_copy_impl.rs b/tests/ui/clone_on_copy_impl.rs index 8f9f2a0db8c..06d8ad48446 100644 --- a/tests/ui/clone_on_copy_impl.rs +++ b/tests/ui/clone_on_copy_impl.rs @@ -1,3 +1,5 @@ +#![allow(clippy::needless_clone_impl)] + use std::fmt; use std::marker::PhantomData; diff --git a/tests/ui/derive.rs b/tests/ui/derive.rs index 843e1df8bc6..857cd379fe2 100644 --- a/tests/ui/derive.rs +++ b/tests/ui/derive.rs @@ -1,4 +1,4 @@ -#![allow(dead_code)] +#![allow(clippy::needless_clone_impl, dead_code)] #![warn(clippy::expl_impl_clone_on_copy)] #[derive(Copy)] diff --git a/tests/ui/needless_clone_impl.fixed b/tests/ui/needless_clone_impl.fixed new file mode 100644 index 00000000000..05caf485db6 --- /dev/null +++ b/tests/ui/needless_clone_impl.fixed @@ -0,0 +1,69 @@ +//@run-rustfix +#![allow(unused)] +#![no_main] + +// lint + +struct A(u32); + +impl Clone for A { + fn clone(&self) -> Self { *self } +} + +impl Copy for A {} + +// do not lint + +struct B(u32); + +impl Clone for B { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for B {} + +// do not lint derived (clone's implementation is `*self` here anyway) + +#[derive(Clone, Copy)] +struct C(u32); + +// do not lint derived (fr this time) + +struct D(u32); + +#[automatically_derived] +impl Clone for D { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for D {} + +// do not lint if clone is not manually implemented + +struct E(u32); + +#[automatically_derived] +impl Clone for E { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for E {} + +// do not lint since copy has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Clone for Uwu { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for Uwu {} diff --git a/tests/ui/needless_clone_impl.rs b/tests/ui/needless_clone_impl.rs new file mode 100644 index 00000000000..e30469e09e8 --- /dev/null +++ b/tests/ui/needless_clone_impl.rs @@ -0,0 +1,71 @@ +//@run-rustfix +#![allow(unused)] +#![no_main] + +// lint + +struct A(u32); + +impl Clone for A { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for A {} + +// do not lint + +struct B(u32); + +impl Clone for B { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for B {} + +// do not lint derived (clone's implementation is `*self` here anyway) + +#[derive(Clone, Copy)] +struct C(u32); + +// do not lint derived (fr this time) + +struct D(u32); + +#[automatically_derived] +impl Clone for D { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for D {} + +// do not lint if clone is not manually implemented + +struct E(u32); + +#[automatically_derived] +impl Clone for E { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for E {} + +// do not lint since copy has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Clone for Uwu { + fn clone(&self) -> Self { + Self(self.0) + } +} + +impl Copy for Uwu {} diff --git a/tests/ui/needless_clone_impl.stderr b/tests/ui/needless_clone_impl.stderr new file mode 100644 index 00000000000..51e2ef4f741 --- /dev/null +++ b/tests/ui/needless_clone_impl.stderr @@ -0,0 +1,13 @@ +error: manual implementation of `Clone` on a `Copy` type + --> $DIR/needless_clone_impl.rs:10:29 + | +LL | fn clone(&self) -> Self { + | _____________________________^ +LL | | Self(self.0) +LL | | } + | |_____^ help: change this to: `{ *self }` + | + = note: `#[deny(clippy::needless_clone_impl)]` on by default + +error: aborting due to previous error + diff --git a/tests/ui/unnecessary_struct_initialization.fixed b/tests/ui/unnecessary_struct_initialization.fixed index bdf746cf2c4..aec9725c4b9 100644 --- a/tests/ui/unnecessary_struct_initialization.fixed +++ b/tests/ui/unnecessary_struct_initialization.fixed @@ -1,6 +1,6 @@ //@run-rustfix -#![allow(unused)] +#![allow(clippy::needless_clone_impl, unused)] #![warn(clippy::unnecessary_struct_initialization)] struct S { diff --git a/tests/ui/unnecessary_struct_initialization.rs b/tests/ui/unnecessary_struct_initialization.rs index 7271e2f957a..a3b9b88dbc2 100644 --- a/tests/ui/unnecessary_struct_initialization.rs +++ b/tests/ui/unnecessary_struct_initialization.rs @@ -1,6 +1,6 @@ //@run-rustfix -#![allow(unused)] +#![allow(clippy::needless_clone_impl, unused)] #![warn(clippy::unnecessary_struct_initialization)] struct S {