From 2a3354ff772362b2608f29c7d952c32af381fb50 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Fri, 30 Jun 2023 15:16:56 -0500 Subject: [PATCH] refactor --- clippy_lints/src/incorrect_impls.rs | 68 ++++++----- .../incorrect_clone_impl_on_copy_type.stderr | 4 +- ...correct_partial_ord_impl_on_ord_type.fixed | 114 ++++++++++++++++++ .../incorrect_partial_ord_impl_on_ord_type.rs | 5 +- ...orrect_partial_ord_impl_on_ord_type.stderr | 23 ++-- 5 files changed, 168 insertions(+), 46 deletions(-) create mode 100644 tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs index 197f4a935b00..9a7c9ba933a8 100644 --- a/clippy_lints/src/incorrect_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -4,12 +4,12 @@ use clippy_utils::{ ty::implements_trait, }; use rustc_errors::Applicability; -use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, PatKind, UnOp}; +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_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{sym, symbol}; +use rustc_span::{sym, symbol::kw}; use std::borrow::Cow; declare_clippy_lint! { @@ -61,31 +61,39 @@ declare_clippy_lint! { /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently /// introduce an error upon refactoring. /// + /// ### Limitations + /// Will not lint if `Self` and `Rhs` do not have the same type. + /// /// ### Example - /// ```rust,ignore + /// ```rust + /// # use std::cmp::Ordering; /// #[derive(Eq, PartialEq)] /// struct A(u32); /// /// impl Ord for A { /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); + /// // ... + /// # todo!(); /// } /// } /// /// impl PartialOrd for A { /// fn partial_cmp(&self, other: &Self) -> Option { - /// todo!(); + /// // ... + /// # todo!(); /// } /// } /// ``` /// Use instead: - /// ```rust,ignore + /// ```rust + /// # use std::cmp::Ordering; /// #[derive(Eq, PartialEq)] /// struct A(u32); /// /// impl Ord for A { /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); + /// // ... + /// # todo!(); /// } /// } /// @@ -105,8 +113,7 @@ declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE, INCORRE impl LateLintPass<'_> for IncorrectImpls { #[expect(clippy::too_many_lines)] 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 { + let Some(Node::Item(item)) = get_parent_node(cx.tcx, impl_item.hir_id()) else { return; }; let ItemKind::Impl(imp) = item.kind else { @@ -115,7 +122,6 @@ impl LateLintPass<'_> for IncorrectImpls { 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; } @@ -127,7 +133,7 @@ impl LateLintPass<'_> for IncorrectImpls { return; }; - if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl_def_id) + if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl.def_id) && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) && implements_trait( cx, @@ -139,9 +145,9 @@ impl LateLintPass<'_> for IncorrectImpls { if impl_item.ident.name == sym::clone { 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 + && let ExprKind::Unary(UnOp::Deref, deref) = expr.kind + && let ExprKind::Path(qpath) = deref.kind + && last_path_segment(&qpath).ident.name == kw::SelfLower {} else { span_lint_and_sugg( cx, @@ -163,7 +169,7 @@ impl LateLintPass<'_> for IncorrectImpls { INCORRECT_CLONE_IMPL_ON_COPY_TYPE, impl_item.span, "incorrect implementation of `clone_from` on a `Copy` type", - "remove this", + "remove it", String::new(), Applicability::MaybeIncorrect, ); @@ -172,7 +178,7 @@ impl LateLintPass<'_> for IncorrectImpls { } } - if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl_def_id) + if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl.def_id) && impl_item.ident.name == sym::partial_cmp && let Some(ord_def_id) = cx .tcx @@ -203,10 +209,7 @@ impl LateLintPass<'_> for IncorrectImpls { {} else { // If lhs and rhs are not the same type, bail. This makes creating a valid // suggestion tons more complex. - if let Some(lhs) = trait_impl.substs.get(0) - && let Some(rhs) = trait_impl.substs.get(1) - && lhs != rhs - { + if let [lhs, rhs, ..] = trait_impl.substs.as_slice() && lhs != rhs { return; } @@ -216,22 +219,23 @@ impl LateLintPass<'_> for IncorrectImpls { item.span, "incorrect implementation of `partial_cmp` on an `Ord` type", |diag| { - let (help, app) = if let Some(other) = body.params.get(1) - && let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind - { - ( - Cow::Owned(format!("{{ Some(self.cmp({})) }}", other_ident.name)), - Applicability::Unspecified, - ) + let [_, other] = body.params else { + return; + }; + + let suggs = if let Some(other_ident) = other.pat.simple_ident() { + vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] } else { - (Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders) + vec![ + (block.span, "{ Some(self.cmp(other)) }".to_owned()), + (other.pat.span, "other".to_owned()), + ] }; - diag.span_suggestion( - block.span, + diag.multipart_suggestion( "change this to", - help, - app, + suggs, + Applicability::Unspecified, ); } ); diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.stderr b/tests/ui/incorrect_clone_impl_on_copy_type.stderr index 0021841aa860..7bcba8ba45a2 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.stderr +++ b/tests/ui/incorrect_clone_impl_on_copy_type.stderr @@ -16,7 +16,7 @@ LL | / fn clone_from(&mut self, source: &Self) { LL | | source.clone(); LL | | *self = source.clone(); LL | | } - | |_____^ help: remove this + | |_____^ help: remove it error: incorrect implementation of `clone` on a `Copy` type --> $DIR/incorrect_clone_impl_on_copy_type.rs:81:29 @@ -34,7 +34,7 @@ LL | / fn clone_from(&mut self, source: &Self) { LL | | source.clone(); LL | | *self = source.clone(); LL | | } - | |_____^ help: remove this + | |_____^ help: remove it error: aborting due to 4 previous errors diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed b/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed new file mode 100644 index 000000000000..dd4fdd98822c --- /dev/null +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed @@ -0,0 +1,114 @@ +//@run-rustfix +#![allow(unused)] +#![no_main] + +use std::cmp::Ordering; + +// lint + +#[derive(Eq, PartialEq)] +struct A(u32); + +impl Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for A { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +// do not lint + +#[derive(Eq, PartialEq)] +struct B(u32); + +impl Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +// lint, and give `_` a name + +#[derive(Eq, PartialEq)] +struct C(u32); + +impl Ord for C { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for C { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +// do not lint derived + +#[derive(Eq, Ord, PartialEq, PartialOrd)] +struct D(u32); + +// do not lint if ord is not manually implemented + +#[derive(Eq, PartialEq)] +struct E(u32); + +impl PartialOrd for E { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint since ord has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Ord for Uwu { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for Uwu { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint since `Rhs` is not `Self` + +#[derive(Eq, PartialEq)] +struct F(u32); + +impl Ord for F { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for F { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl PartialEq for F { + fn eq(&self, other: &u32) -> bool { + todo!(); + } +} + +impl PartialOrd for F { + fn partial_cmp(&self, other: &u32) -> Option { + todo!(); + } +} diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs index 25fe909a8d39..522e82299c0a 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs @@ -1,3 +1,4 @@ +//@run-rustfix #![allow(unused)] #![no_main] @@ -37,7 +38,7 @@ impl PartialOrd for B { } } -// lint, but we can't give a suggestion since &Self is not named +// lint, and give `_` a name #[derive(Eq, PartialEq)] struct C(u32); @@ -50,7 +51,7 @@ impl Ord for C { impl PartialOrd for C { fn partial_cmp(&self, _: &Self) -> Option { - todo!(); // don't run rustfix, or else this will cause it to fail to compile + todo!(); } } diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr index 86b3d37241a7..0e477798c406 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr @@ -1,5 +1,5 @@ error: incorrect implementation of `partial_cmp` on an `Ord` type - --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1 + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:18:1 | LL | / impl PartialOrd for A { LL | | fn partial_cmp(&self, other: &Self) -> Option { @@ -13,16 +13,19 @@ LL | | } = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default error: incorrect implementation of `partial_cmp` on an `Ord` type - --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1 + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:52:1 | -LL | / impl PartialOrd for C { -LL | | fn partial_cmp(&self, _: &Self) -> Option { - | | _________________________________________________________- -LL | || todo!(); // don't run rustfix, or else this will cause it to fail to compile -LL | || } - | ||_____- help: change this to: `{ Some(self.cmp(...)) }` -LL | | } - | |__^ +LL | / impl PartialOrd for C { +LL | | fn partial_cmp(&self, _: &Self) -> Option { +LL | | todo!(); +LL | | } +LL | | } + | |_^ + | +help: change this to + | +LL | fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } + | ~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 2 previous errors