implement manual_partial_ord_impl

first try at this

Author: Centri3

CHANGELOG.md

@@ -4921,6 +4921,7 @@ Released 2018-09-13
 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
+[`manual_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_partial_ord_impl
 [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
 [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns
 [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid

clippy_lints/src/declared_lints.rs

@@ -277,6 +277,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::manual_let_else::MANUAL_LET_ELSE_INFO,
     crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO,
     crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO,
+    crate::manual_partial_ord_impl::MANUAL_PARTIAL_ORD_IMPL_INFO,
     crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO,
     crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO,
     crate::manual_retain::MANUAL_RETAIN_INFO,

clippy_lints/src/lib.rs

@@ -188,6 +188,7 @@ mod manual_is_ascii_check;
 mod manual_let_else;
 mod manual_main_separator_str;
 mod manual_non_exhaustive;
+mod manual_partial_ord_impl;
 mod manual_range_patterns;
 mod manual_rem_euclid;
 mod manual_retain;

clippy_lints/src/manual_partial_ord_impl.rs

@@ -0,0 +1,132 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::{
+    def_path_def_ids, diagnostics::span_lint_and_sugg, get_trait_def_id, match_def_path, path_res, ty::implements_trait,
+};
+use rustc_errors::Applicability;
+use rustc_hir::def::Res;
+use rustc_hir::*;
+use rustc_hir_analysis::hir_ty_to_ty;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::def_id::DefId;
+use std::cell::OnceCell;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is
+    /// necessary.
+    ///
+    /// ### Why is this bad?
+    /// If both `PartialOrd` and `Ord` are implemented, `PartialOrd` will wrap the returned value of
+    /// `Ord::cmp` in `Some`. Not doing this may silently introduce an error.
+    ///
+    /// ### Example
+    /// ```rust
+    /// #[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> {
+    ///         todo!();
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// #[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))
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.71.0"]
+    pub MANUAL_PARTIAL_ORD_IMPL,
+    nursery,
+    "default lint description"
+}
+impl_lint_pass!(ManualPartialOrdImpl => [MANUAL_PARTIAL_ORD_IMPL]);
+
+#[derive(Clone)]
+pub struct ManualPartialOrdImpl {
+    pub ord_def_id: OnceCell<DefId>,
+}
+
+impl LateLintPass<'_> for ManualPartialOrdImpl {
+    fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+        if_chain! {
+            if let ItemKind::Impl(imp) = &item.kind;
+            if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.owner_id);
+            if cx.tcx.is_diagnostic_item(sym!(PartialOrd), impl_trait_ref.skip_binder().def_id);
+            then {
+                lint_impl_body(self, cx, imp, item);
+            }
+        }
+    }
+}
+
+fn lint_impl_body(conf: &mut ManualPartialOrdImpl, cx: &LateContext<'_>, imp: &Impl<'_>, item: &Item<'_>) {
+    for imp_item in imp.items {
+        if_chain! {
+            if imp_item.ident.name == sym!(partial_cmp);
+            if let ImplItemKind::Fn(_, id) = cx.tcx.hir().impl_item(imp_item.id).kind;
+            then {
+                let body = cx.tcx.hir().body(id);
+                let ord_def_id = conf.ord_def_id.get_or_init(|| get_trait_def_id(cx, &["core", "cmp", "Ord"]).unwrap());
+                if let ExprKind::Block(block, ..)
+                    = body.value.kind && implements_trait(cx, hir_ty_to_ty(cx.tcx, imp.self_ty), *ord_def_id, &[])
+                {
+                    if_chain! {
+                        if block.stmts.is_empty();
+                        if let Some(expr) = block.expr;
+                        if let ExprKind::Call(Expr { kind: ExprKind::Path(path), ..}, [cmp_expr]) = expr.kind;
+                        if let QPath::Resolved(_, some_path) = path;
+                        if let Some(some_seg_one) = some_path.segments.get(0);
+                        if some_seg_one.ident.name == sym!(Some);
+                        if let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind;
+                        if cmp_path.ident.name == sym!(cmp);
+                        if let Res::Local(..) = path_res(cx, other_expr);
+                        then {}
+                        else {
+                            span_lint_and_then(
+                                cx,
+                                MANUAL_PARTIAL_ORD_IMPL,
+                                item.span,
+                                "manual implementation of `PartialOrd` when `Ord` is already implemented",
+                                |diag| {
+                                    if let Some(param) = body.params.get(1)
+                                        && let PatKind::Binding(_, _, param_ident, ..) = param.pat.kind
+                                    {
+                                        diag.span_suggestion(
+                                            block.span,
+                                            "change this to",
+                                            format!("{{ Some(self.cmp({})) }}",
+                                            param_ident.name),
+                                            Applicability::MaybeIncorrect
+                                        );
+                                    } else {
+                                        diag.help("return the value of `self.cmp` wrapped in `Some` instead");
+                                    };
+                                }
+                            );
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

tests/ui/manual_partial_ord_impl.rs

@@ -0,0 +1,57 @@
+#![allow(unused)]
+#![warn(clippy::manual_partial_ord_impl)]
+#![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> {
+        todo!();
+    }
+}
+
+// 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, but we cannot give a suggestion since &Self is not named
+
+#[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, _: &Self) -> Option<Ordering> {
+        todo!();
+    }
+}
+

tests/ui/manual_partial_ord_impl.stderr

@@ -0,0 +1,28 @@
+error: manual implementation of `PartialOrd` when `Ord` is already implemented
+  --> $DIR/manual_partial_ord_impl.rs:18:1
+   |
+LL | /  impl PartialOrd for A {
+LL | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+   | | _____________________________________________________________-
+LL | ||         todo!();
+LL | ||     }
+   | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
+LL | |  }
+   | |__^
+   |
+   = note: `-D clippy::manual-partial-ord-impl` implied by `-D warnings`
+
+error: manual implementation of `PartialOrd` when `Ord` is already implemented
+  --> $DIR/manual_partial_ord_impl.rs:52:1
+   |
+LL | / impl PartialOrd for C {
+LL | |     fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
+LL | |         todo!();
+LL | |     }
+LL | | }
+   | |_^
+   |
+   = help: return the value of `self.cmp` wrapped in `Some` instead
+
+error: aborting due to 2 previous errors
+