refactor

5 files changed, 173 insertions, 49 deletions

diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs
index f7894f9c17b..25e1cb36b74 100644
--- a/clippy_lints/src/incorrect_impls.rs
+++ b/clippy_lints/src/incorrect_impls.rs
@@ -4,13 +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::{ExprKind, ImplItem, ImplItemKind, Node, 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 std::borrow::Cow;
+use rustc_span::{sym, symbol::kw};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -61,31 +60,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<Ordering> {
-    ///         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,17 +112,18 @@ 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 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 ItemKind::Impl(imp) = item.kind else {
+            return;
+        };
         let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir().impl_item(impl_item.impl_item_id()).kind else {
             return;
         };
@@ -124,7 +132,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,
@@ -136,9 +144,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,
@@ -160,7 +168,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,
                 );
@@ -169,7 +177,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
@@ -198,12 +206,9 @@ impl LateLintPass<'_> for IncorrectImpls {
                 && cmp_path.ident.name == sym::cmp
                 && let Res::Local(..) = path_res(cx, other_expr)
             {} else {
-                // If lhs and rhs are not the same type, bail. This makes creating a valid
+                // If `Self` 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;
                 }
 
@@ -213,22 +218,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 0021841aa86..7bcba8ba45a 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 00000000000..dd4fdd98822
--- /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<Ordering> { 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<Ordering> {
+        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<Ordering> { 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<Ordering> {
+        todo!();
+    }
+}
+
+// do not lint since ord has more restrictive bounds
+
+#[derive(Eq, PartialEq)]
+struct Uwu<A>(A);
+
+impl<A: std::fmt::Debug + Ord + PartialOrd> Ord for Uwu<A> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl<A: Ord + PartialOrd> PartialOrd for Uwu<A> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        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<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl PartialEq<u32> for F {
+    fn eq(&self, other: &u32) -> bool {
+        todo!();
+    }
+}
+
+impl PartialOrd<u32> for F {
+    fn partial_cmp(&self, other: &u32) -> Option<Ordering> {
+        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 25fe909a8d3..522e82299c0 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<Ordering> {
-        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 86b3d37241a..0e477798c40 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<Ordering> {
@@ -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<Ordering> {
-   | | _________________________________________________________-
-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<Ordering> {
+LL | |         todo!();
+LL | |     }
+LL | | }
+   | |_^
+   |
+help: change this to
+   |
+LL |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
+   |                           ~~~~~                             ~~~~~~~~~~~~~~~~~~~~~~~~~
 
 error: aborting due to 2 previous errors