Auto merge of #11188 - Centri3:#11178, r=blyxyas

Allow `Self::cmp(self, other)` as a correct impl

Fixes #11178

Also no longer checks if the method name is *just* cmp, but the path. That was an oversight on my part ^^

r? `@xFrednet`
(and `@blyxyas` too!)

changelog: [`incorrect_partial_ord_impl_on_ord_type`]: Now allows non-method calls to `cmp` like `Self::cmp(self, other)`

Author: bors

clippy_lints/src/incorrect_impls.rs

@@ -1,8 +1,9 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::paths::ORD_CMP;
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{get_parent_node, is_res_lang_ctor, last_path_segment, path_res};
+use clippy_utils::{get_parent_node, is_res_lang_ctor, last_path_segment, match_def_path, path_res, std_or_core};
 use rustc_errors::Applicability;
-use rustc_hir::def::Res;
+use rustc_hir::def_id::LocalDefId;
 use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, UnOp};
 use rustc_hir_analysis::hir_ty_to_ty;
 use rustc_lint::{LateContext, LateLintPass};
@@ -60,6 +61,10 @@ declare_clippy_lint! {
     /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently
     /// introduce an error upon refactoring.
     ///
+    /// ### Known issues
+    /// Code that calls the `.into()` method instead will be flagged as incorrect, despite `.into()`
+    /// wrapping it in `Some`.
+    ///
     /// ### Limitations
     /// Will not lint if `Self` and `Rhs` do not have the same type.
     ///
@@ -191,6 +196,11 @@ impl LateLintPass<'_> for IncorrectImpls {
                     trait_impl.substs,
                 )
         {
+            // If the `cmp` call likely needs to be fully qualified in the suggestion
+            // (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't
+            // access `cmp_expr` in the suggestion without major changes, as we lint in `else`.
+            let mut needs_fully_qualified = false;
+
             if block.stmts.is_empty()
                 && let Some(expr) = block.expr
                 && let ExprKind::Call(
@@ -202,9 +212,8 @@ impl LateLintPass<'_> for IncorrectImpls {
                         [cmp_expr],
                     ) = expr.kind
                 && is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome)
-                && let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind
-                && cmp_path.ident.name == sym::cmp
-                && let Res::Local(..) = path_res(cx, other_expr)
+                // Fix #11178, allow `Self::cmp(self, ..)` too
+                && self_cmp_call(cx, cmp_expr, impl_item.owner_id.def_id, &mut needs_fully_qualified)
             {} else {
                 // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
                 // suggestion tons more complex.
@@ -221,14 +230,29 @@ impl LateLintPass<'_> for IncorrectImpls {
                         let [_, other] = body.params else {
                             return;
                         };
+                        let Some(std_or_core) = std_or_core(cx) else {
+                            return;
+                        };
 
-                        let suggs = if let Some(other_ident) = other.pat.simple_ident() {
-                            vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
-                        } else {
-                            vec![
+                        let suggs = match (other.pat.simple_ident(), needs_fully_qualified) {
+                            (Some(other_ident), true) => vec![(
+                                block.span,
+                                format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name),
+                            )],
+                            (Some(other_ident), false) => {
+                                vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
+                            },
+                            (None, true) => vec![
+                                (
+                                    block.span,
+                                    format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"),
+                                ),
+                                (other.pat.span, "other".to_owned()),
+                            ],
+                            (None, false) => vec![
                                 (block.span, "{ Some(self.cmp(other)) }".to_owned()),
                                 (other.pat.span, "other".to_owned()),
-                            ]
+                            ],
                         };
 
                         diag.multipart_suggestion(
@@ -242,3 +266,31 @@ impl LateLintPass<'_> for IncorrectImpls {
         }
     }
 }
+
+/// Returns whether this is any of `self.cmp(..)`, `Self::cmp(self, ..)` or `Ord::cmp(self, ..)`.
+fn self_cmp_call<'tcx>(
+    cx: &LateContext<'tcx>,
+    cmp_expr: &'tcx Expr<'tcx>,
+    def_id: LocalDefId,
+    needs_fully_qualified: &mut bool,
+) -> bool {
+    match cmp_expr.kind {
+        ExprKind::Call(path, [_self, _other]) => path_res(cx, path)
+            .opt_def_id()
+            .is_some_and(|def_id| match_def_path(cx, def_id, &ORD_CMP)),
+        ExprKind::MethodCall(_, _, [_other], ..) => {
+            // We can set this to true here no matter what as if it's a `MethodCall` and goes to the
+            // `else` branch, it must be a method named `cmp` that isn't `Ord::cmp`
+            *needs_fully_qualified = true;
+
+            // It's a bit annoying but `typeck_results` only gives us the CURRENT body, which we
+            // have none, not of any `LocalDefId` we want, so we must call the query itself to avoid
+            // an immediate ICE
+            cx.tcx
+                .typeck(def_id)
+                .type_dependent_def_id(cmp_expr.hir_id)
+                .is_some_and(|def_id| match_def_path(cx, def_id, &ORD_CMP))
+        },
+        _ => false,
+    }
+}

clippy_utils/src/paths.rs

@@ -161,3 +161,4 @@ pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
 pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"];
 pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"];
 pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];
+pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"];

tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed

@@ -1,5 +1,4 @@
 //@run-rustfix
-#![allow(unused)]
 #![no_main]
 
 use std::cmp::Ordering;
@@ -112,3 +111,35 @@ impl PartialOrd<u32> for F {
         todo!();
     }
 }
+
+// #11178, do not lint
+
+#[derive(Eq, PartialEq)]
+struct G(u32);
+
+impl Ord for G {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for G {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(Self::cmp(self, other))
+    }
+}
+
+#[derive(Eq, PartialEq)]
+struct H(u32);
+
+impl Ord for H {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for H {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(Ord::cmp(self, other))
+    }
+}

tests/ui/incorrect_partial_ord_impl_on_ord_type.rs

@@ -1,5 +1,4 @@
 //@run-rustfix
-#![allow(unused)]
 #![no_main]
 
 use std::cmp::Ordering;
@@ -116,3 +115,35 @@ impl PartialOrd<u32> for F {
         todo!();
     }
 }
+
+// #11178, do not lint
+
+#[derive(Eq, PartialEq)]
+struct G(u32);
+
+impl Ord for G {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for G {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(Self::cmp(self, other))
+    }
+}
+
+#[derive(Eq, PartialEq)]
+struct H(u32);
+
+impl Ord for H {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for H {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(Ord::cmp(self, other))
+    }
+}

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:18:1
+  --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1
    |
 LL | /  impl PartialOrd for A {
 LL | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -13,7 +13,7 @@ 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:52:1
+  --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1
    |
 LL | / impl PartialOrd for C {
 LL | |     fn partial_cmp(&self, _: &Self) -> Option<Ordering> {

tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs

@@ -0,0 +1,51 @@
+// This test's filename is... a bit verbose. But it ensures we suggest the correct code when `Ord`
+// is not in scope.
+#![no_main]
+#![no_implicit_prelude]
+
+extern crate std;
+
+use std::cmp::{self, Eq, Ordering, PartialEq, PartialOrd};
+use std::option::Option::{self, Some};
+use std::todo;
+
+// lint
+
+#[derive(Eq, PartialEq)]
+struct A(u32);
+
+impl cmp::Ord for A {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for A {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        // NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't
+        // automatically applied
+        todo!();
+    }
+}
+
+#[derive(Eq, PartialEq)]
+struct B(u32);
+
+impl B {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl cmp::Ord for B {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for B {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        // This calls `B.cmp`, not `Ord::cmp`!
+        Some(self.cmp(other))
+    }
+}

tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.stderr

@@ -0,0 +1,31 @@
+error: incorrect implementation of `partial_cmp` on an `Ord` type
+  --> $DIR/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs:23:1
+   |
+LL | /  impl PartialOrd for A {
+LL | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+   | | _____________________________________________________________-
+LL | ||         // NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't
+LL | ||         // automatically applied
+LL | ||         todo!();
+LL | ||     }
+   | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
+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_fully_qual.rs:46:1
+   |
+LL | /  impl PartialOrd for B {
+LL | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+   | | _____________________________________________________________-
+LL | ||         // This calls `B.cmp`, not `Ord::cmp`!
+LL | ||         Some(self.cmp(other))
+LL | ||     }
+   | ||_____- help: change this to: `{ Some(std::cmp::Ord::cmp(self, other)) }`
+LL | |  }
+   | |__^
+
+error: aborting due to 2 previous errors
+