Accept self.cmp(other).into() as canonical PartialOrd impl

`non_canonical_partial_ord_impl` will now recognize two forms of
canonical implementations: `Some(self.cmp(other))` and
`self.cmp(other).into()`.

Author: samueltardieu

clippy_lints/src/non_canonical_impls.rs

@@ -1,6 +1,8 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{is_from_proc_macro, is_res_lang_ctor, last_path_segment, path_res, std_or_core};
+use clippy_utils::{
+    is_diag_trait_item, is_from_proc_macro, is_res_lang_ctor, last_path_segment, path_res, std_or_core,
+};
 use rustc_errors::Applicability;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, LangItem, Node, UnOp};
@@ -98,7 +100,7 @@ declare_clippy_lint! {
     ///
     /// impl PartialOrd for A {
     ///     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-    ///         Some(self.cmp(other))
+    ///         Some(self.cmp(other))   // or self.cmp(other).into()
     ///     }
     /// }
     /// ```
@@ -185,18 +187,18 @@ impl LateLintPass<'_> for NonCanonicalImpls {
 
             if block.stmts.is_empty()
                 && let Some(expr) = block.expr
-                && expr_is_cmp(cx, &expr.kind, impl_item, &mut needs_fully_qualified)
+                && expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified)
             {
                 return;
             }
             // Fix #12683, allow [`needless_return`] here
             else if block.expr.is_none()
                 && let Some(stmt) = block.stmts.first()
                 && let rustc_hir::StmtKind::Semi(Expr {
-                    kind: ExprKind::Ret(Some(Expr { kind: ret_kind, .. })),
+                    kind: ExprKind::Ret(Some(ret)),
                     ..
                 }) = stmt.kind
-                && expr_is_cmp(cx, ret_kind, impl_item, &mut needs_fully_qualified)
+                && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
             {
                 return;
             }
@@ -252,22 +254,29 @@ impl LateLintPass<'_> for NonCanonicalImpls {
 /// Return true if `expr_kind` is a `cmp` call.
 fn expr_is_cmp<'tcx>(
     cx: &LateContext<'tcx>,
-    expr_kind: &'tcx ExprKind<'tcx>,
+    expr: &'tcx Expr<'tcx>,
     impl_item: &ImplItem<'_>,
     needs_fully_qualified: &mut bool,
 ) -> bool {
+    let impl_item_did = impl_item.owner_id.def_id;
     if let ExprKind::Call(
         Expr {
             kind: ExprKind::Path(some_path),
             hir_id: some_hir_id,
             ..
         },
         [cmp_expr],
-    ) = expr_kind
+    ) = expr.kind
     {
         is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome)
             // Fix #11178, allow `Self::cmp(self, ..)` too
-            && self_cmp_call(cx, cmp_expr, impl_item.owner_id.def_id, needs_fully_qualified)
+            && self_cmp_call(cx, cmp_expr, impl_item_did, needs_fully_qualified)
+    } else if let ExprKind::MethodCall(_, recv, [], _) = expr.kind {
+        cx.tcx
+            .typeck(impl_item_did)
+            .type_dependent_def_id(expr.hir_id)
+            .is_some_and(|def_id| is_diag_trait_item(cx, def_id, sym::Into))
+            && self_cmp_call(cx, recv, impl_item_did, needs_fully_qualified)
     } else {
         false
     }

tests/ui/non_canonical_partial_ord_impl.fixed

@@ -162,3 +162,36 @@ impl PartialOrd for I {
         return Some(self.cmp(other));
     }
 }
+
+// #13640, do not lint
+
+#[derive(Eq, PartialEq)]
+struct J(u32);
+
+impl Ord for J {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for J {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        self.cmp(other).into()
+    }
+}
+
+// #13640, check that a simple `.into()` does not obliterate the lint
+
+#[derive(Eq, PartialEq)]
+struct K(u32);
+
+impl Ord for K {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for K {
+    //~^ non_canonical_partial_ord_impl
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
+}

tests/ui/non_canonical_partial_ord_impl.rs

@@ -166,3 +166,38 @@ impl PartialOrd for I {
         return Some(self.cmp(other));
     }
 }
+
+// #13640, do not lint
+
+#[derive(Eq, PartialEq)]
+struct J(u32);
+
+impl Ord for J {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for J {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        self.cmp(other).into()
+    }
+}
+
+// #13640, check that a simple `.into()` does not obliterate the lint
+
+#[derive(Eq, PartialEq)]
+struct K(u32);
+
+impl Ord for K {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl PartialOrd for K {
+    //~^ non_canonical_partial_ord_impl
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Ordering::Greater.into()
+    }
+}

tests/ui/non_canonical_partial_ord_impl.stderr

@@ -31,5 +31,18 @@ LL -     fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
 LL +     fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
    |
 
-error: aborting due to 2 previous errors
+error: non-canonical implementation of `partial_cmp` on an `Ord` type
+  --> tests/ui/non_canonical_partial_ord_impl.rs:198:1
+   |
+LL | /  impl PartialOrd for K {
+LL | |
+LL | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+   | | _____________________________________________________________-
+LL | ||         Ordering::Greater.into()
+LL | ||     }
+   | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
+LL | |  }
+   | |__^
+
+error: aborting due to 3 previous errors