From bb07da017e1a79d740f194b7ef2279f3406e9fa0 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Sun, 11 Jun 2023 02:46:28 -0500 Subject: [PATCH] add lint [`needless_clone_impl`] --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/needless_impls.rs | 113 ++++++++++++++++++ tests/ui/clone_on_copy_impl.rs | 2 + tests/ui/derive.rs | 2 +- tests/ui/needless_clone_impl.fixed | 69 +++++++++++ tests/ui/needless_clone_impl.rs | 71 +++++++++++ tests/ui/needless_clone_impl.stderr | 13 ++ .../unnecessary_struct_initialization.fixed | 2 +- tests/ui/unnecessary_struct_initialization.rs | 2 +- 11 files changed, 275 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/needless_impls.rs create mode 100644 tests/ui/needless_clone_impl.fixed create mode 100644 tests/ui/needless_clone_impl.rs create mode 100644 tests/ui/needless_clone_impl.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d62bfd4f99b..8b31af6539a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5004,6 +5004,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 769774b27c4c..efd7a74bdaeb 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -461,6 +461,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ 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 dcf1c6f64a4b..5b4944456ae6 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -225,6 +225,7 @@ mod needless_continue; 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; @@ -1042,6 +1043,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: min_ident_chars_threshold, }) }); + 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 000000000000..795fe6b08e4e --- /dev/null +++ b/clippy_lints/src/needless_impls.rs @@ -0,0 +1,113 @@ +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 + .diagnostic_items(trait_impl.def_id.krate) + .name_to_id + .get(&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 8f9f2a0db8c4..06d8ad484464 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 843e1df8bc6b..857cd379fe23 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 000000000000..05caf485db69 --- /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 000000000000..e30469e09e8d --- /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 000000000000..51e2ef4f7410 --- /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 bdf746cf2c42..aec9725c4b9c 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 7271e2f957a8..a3b9b88dbc2f 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 {