Auto merge of #5848 - Ryan1729:add-derive_ord_xor_partial_ord-lint, r…

…=matthiaskrgr

Add derive_ord_xor_partial_ord lint

Fix #1621

Some remarks:
This PR follows the example of the analogous derive_hash_xor_partial_eq lint where possible.
I initially tried using the `match_path` function to identify `Ord` implementation like the derive_hash_xor_partial_eq lint currently does, for `Hash` implementations but that didn't work.

Specifically, the structs at the top level were getting paths that matched `&["$crate", "cmp", "Ord"]` instead of `&["std", "cmp", "Ord"]`. While trying to figure out what to do instead I saw the comment at the top of [clippy_lints/src/utils/paths.rs](https://github.com/rust-lang/rust-clippy/blob/f5d429cd762423901f946abd800dc2cd91366ccf/clippy_lints/src/utils/paths.rs#L5) that mentioned [this issue](#5393) and suggested to use diagnostic items instead of hardcoded paths whenever possible. I looked for a way to identify `Ord` implementations with diagnostic items, but (possibly because this was the first time I had heard of diagnostic items,) I was unable to find one.

Eventually I tried using `get_trait_def_id` and comparing `DefId` values directly and that seems to work as expected. Maybe there's a better approach however?

changelog: new lint: derive_ord_xor_partial_ord

CHANGELOG.md

@@ -1454,6 +1454,7 @@ Released 2018-09-13
 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
 [`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
 [`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
+[`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
 [`diverging_sub_expression`]: https://rust-lang.github.io/rust-clippy/master/index.html#diverging_sub_expression
 [`doc_markdown`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown
 [`double_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_comparisons

clippy_lints/src/derive.rs

@@ -1,6 +1,7 @@
 use crate::utils::paths;
 use crate::utils::{
-    is_automatically_derived, is_copy, match_path, span_lint_and_help, span_lint_and_note, span_lint_and_then,
+    get_trait_def_id, is_automatically_derived, is_copy, match_path, span_lint_and_help, span_lint_and_note,
+    span_lint_and_then,
 };
 use if_chain::if_chain;
 use rustc_hir::def_id::DefId;
@@ -43,6 +44,57 @@ declare_clippy_lint! {
     "deriving `Hash` but implementing `PartialEq` explicitly"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for deriving `Ord` but implementing `PartialOrd`
+    /// explicitly or vice versa.
+    ///
+    /// **Why is this bad?** The implementation of these traits must agree (for
+    /// example for use with `sort`) so it’s probably a bad idea to use a
+    /// default-generated `Ord` implementation with an explicitly defined
+    /// `PartialOrd`. In particular, the following must hold for any type
+    /// implementing `Ord`:
+    ///
+    /// ```text
+    /// k1.cmp(&k2) == k1.partial_cmp(&k2).unwrap()
+    /// ```
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust,ignore
+    /// #[derive(Ord, PartialEq, Eq)]
+    /// struct Foo;
+    ///
+    /// impl PartialOrd for Foo {
+    ///     ...
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// #[derive(PartialEq, Eq)]
+    /// struct Foo;
+    ///
+    /// impl PartialOrd for Foo {
+    ///     fn partial_cmp(&self, other: &Foo) -> Option<Ordering> {
+    ///        Some(self.cmp(other))
+    ///     }
+    /// }
+    ///
+    /// impl Ord for Foo {
+    ///     ...
+    /// }
+    /// ```
+    /// or, if you don't need a custom ordering:
+    /// ```rust,ignore
+    /// #[derive(Ord, PartialOrd, PartialEq, Eq)]
+    /// struct Foo;
+    /// ```
+    pub DERIVE_ORD_XOR_PARTIAL_ORD,
+    correctness,
+    "deriving `Ord` but implementing `PartialOrd` explicitly"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for explicit `Clone` implementations for `Copy`
     /// types.
@@ -103,7 +155,12 @@ declare_clippy_lint! {
     "deriving `serde::Deserialize` on a type that has methods using `unsafe`"
 }
 
-declare_lint_pass!(Derive => [EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_XOR_EQ, UNSAFE_DERIVE_DESERIALIZE]);
+declare_lint_pass!(Derive => [
+    EXPL_IMPL_CLONE_ON_COPY,
+    DERIVE_HASH_XOR_EQ,
+    DERIVE_ORD_XOR_PARTIAL_ORD,
+    UNSAFE_DERIVE_DESERIALIZE
+]);
 
 impl<'tcx> LateLintPass<'tcx> for Derive {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
@@ -116,6 +173,7 @@ impl<'tcx> LateLintPass<'tcx> for Derive {
             let is_automatically_derived = is_automatically_derived(&*item.attrs);
 
             check_hash_peq(cx, item.span, trait_ref, ty, is_automatically_derived);
+            check_ord_partial_ord(cx, item.span, trait_ref, ty, is_automatically_derived);
 
             if is_automatically_derived {
                 check_unsafe_derive_deserialize(cx, item, trait_ref, ty);
@@ -180,6 +238,60 @@ fn check_hash_peq<'tcx>(
     }
 }
 
+/// Implementation of the `DERIVE_ORD_XOR_PARTIAL_ORD` lint.
+fn check_ord_partial_ord<'tcx>(
+    cx: &LateContext<'tcx>,
+    span: Span,
+    trait_ref: &TraitRef<'_>,
+    ty: Ty<'tcx>,
+    ord_is_automatically_derived: bool,
+) {
+    if_chain! {
+        if let Some(ord_trait_def_id) = get_trait_def_id(cx, &paths::ORD);
+        if let Some(partial_ord_trait_def_id) = cx.tcx.lang_items().partial_ord_trait();
+        if let Some(def_id) = &trait_ref.trait_def_id();
+        if *def_id == ord_trait_def_id;
+        then {
+            // Look for the PartialOrd implementations for `ty`
+            cx.tcx.for_each_relevant_impl(partial_ord_trait_def_id, ty, |impl_id| {
+                let partial_ord_is_automatically_derived = is_automatically_derived(&cx.tcx.get_attrs(impl_id));
+
+                if partial_ord_is_automatically_derived == ord_is_automatically_derived {
+                    return;
+                }
+
+                let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
+
+                // Only care about `impl PartialOrd<Foo> for Foo`
+                // For `impl PartialOrd<B> for A, input_types is [A, B]
+                if trait_ref.substs.type_at(1) == ty {
+                    let mess = if partial_ord_is_automatically_derived {
+                        "you are implementing `Ord` explicitly but have derived `PartialOrd`"
+                    } else {
+                        "you are deriving `Ord` but have implemented `PartialOrd` explicitly"
+                    };
+
+                    span_lint_and_then(
+                        cx,
+                        DERIVE_ORD_XOR_PARTIAL_ORD,
+                        span,
+                        mess,
+                        |diag| {
+                            if let Some(local_def_id) = impl_id.as_local() {
+                                let hir_id = cx.tcx.hir().as_local_hir_id(local_def_id);
+                                diag.span_note(
+                                    cx.tcx.hir().span(hir_id),
+                                    "`PartialOrd` implemented here"
+                                );
+                            }
+                        }
+                    );
+                }
+            });
+        }
+    }
+}
+
 /// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint.
 fn check_copy_clone<'tcx>(cx: &LateContext<'tcx>, item: &Item<'_>, trait_ref: &TraitRef<'_>, ty: Ty<'tcx>) {
     if match_path(&trait_ref.path, &paths::CLONE_TRAIT) {

clippy_lints/src/lib.rs

@@ -513,6 +513,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &default_trait_access::DEFAULT_TRAIT_ACCESS,
         &dereference::EXPLICIT_DEREF_METHODS,
         &derive::DERIVE_HASH_XOR_EQ,
+        &derive::DERIVE_ORD_XOR_PARTIAL_ORD,
         &derive::EXPL_IMPL_CLONE_ON_COPY,
         &derive::UNSAFE_DERIVE_DESERIALIZE,
         &doc::DOC_MARKDOWN,
@@ -1230,6 +1231,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&copies::IFS_SAME_COND),
         LintId::of(&copies::IF_SAME_THEN_ELSE),
         LintId::of(&derive::DERIVE_HASH_XOR_EQ),
+        LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD),
         LintId::of(&doc::MISSING_SAFETY_DOC),
         LintId::of(&doc::NEEDLESS_DOCTEST_MAIN),
         LintId::of(&double_comparison::DOUBLE_COMPARISONS),
@@ -1648,6 +1650,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&copies::IFS_SAME_COND),
         LintId::of(&copies::IF_SAME_THEN_ELSE),
         LintId::of(&derive::DERIVE_HASH_XOR_EQ),
+        LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD),
         LintId::of(&drop_bounds::DROP_BOUNDS),
         LintId::of(&drop_forget_ref::DROP_COPY),
         LintId::of(&drop_forget_ref::DROP_REF),

src/lintlist/mod.rs

@@ -360,6 +360,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "derive",
     },
+    Lint {
+        name: "derive_ord_xor_partial_ord",
+        group: "correctness",
+        desc: "deriving `Ord` but implementing `PartialOrd` explicitly",
+        deprecation: None,
+        module: "derive",
+    },
     Lint {
         name: "diverging_sub_expression",
         group: "complexity",

tests/ui/derive_ord_xor_partial_ord.rs

@@ -0,0 +1,68 @@
+#![warn(clippy::derive_ord_xor_partial_ord)]
+
+use std::cmp::Ordering;
+
+#[derive(PartialOrd, Ord, PartialEq, Eq)]
+struct DeriveBoth;
+
+impl PartialEq<u64> for DeriveBoth {
+    fn eq(&self, _: &u64) -> bool {
+        true
+    }
+}
+
+impl PartialOrd<u64> for DeriveBoth {
+    fn partial_cmp(&self, _: &u64) -> Option<Ordering> {
+        Some(Ordering::Equal)
+    }
+}
+
+#[derive(Ord, PartialEq, Eq)]
+struct DeriveOrd;
+
+impl PartialOrd for DeriveOrd {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(other.cmp(self))
+    }
+}
+
+#[derive(Ord, PartialEq, Eq)]
+struct DeriveOrdWithExplicitTypeVariable;
+
+impl PartialOrd<DeriveOrdWithExplicitTypeVariable> for DeriveOrdWithExplicitTypeVariable {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(other.cmp(self))
+    }
+}
+
+#[derive(PartialOrd, PartialEq, Eq)]
+struct DerivePartialOrd;
+
+impl std::cmp::Ord for DerivePartialOrd {
+    fn cmp(&self, other: &Self) -> Ordering {
+        Ordering::Less
+    }
+}
+
+#[derive(PartialOrd, PartialEq, Eq)]
+struct ImplUserOrd;
+
+trait Ord {}
+
+// We don't want to lint on user-defined traits called `Ord`
+impl Ord for ImplUserOrd {}
+
+mod use_ord {
+    use std::cmp::{Ord, Ordering};
+
+    #[derive(PartialOrd, PartialEq, Eq)]
+    struct DerivePartialOrdInUseOrd;
+
+    impl Ord for DerivePartialOrdInUseOrd {
+        fn cmp(&self, other: &Self) -> Ordering {
+            Ordering::Less
+        }
+    }
+}
+
+fn main() {}

tests/ui/derive_ord_xor_partial_ord.stderr

@@ -0,0 +1,71 @@
+error: you are deriving `Ord` but have implemented `PartialOrd` explicitly
+  --> $DIR/derive_ord_xor_partial_ord.rs:20:10
+   |
+LL | #[derive(Ord, PartialEq, Eq)]
+   |          ^^^
+   |
+   = note: `-D clippy::derive-ord-xor-partial-ord` implied by `-D warnings`
+note: `PartialOrd` implemented here
+  --> $DIR/derive_ord_xor_partial_ord.rs:23:1
+   |
+LL | / impl PartialOrd for DeriveOrd {
+LL | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+LL | |         Some(other.cmp(self))
+LL | |     }
+LL | | }
+   | |_^
+   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: you are deriving `Ord` but have implemented `PartialOrd` explicitly
+  --> $DIR/derive_ord_xor_partial_ord.rs:29:10
+   |
+LL | #[derive(Ord, PartialEq, Eq)]
+   |          ^^^
+   |
+note: `PartialOrd` implemented here
+  --> $DIR/derive_ord_xor_partial_ord.rs:32:1
+   |
+LL | / impl PartialOrd<DeriveOrdWithExplicitTypeVariable> for DeriveOrdWithExplicitTypeVariable {
+LL | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+LL | |         Some(other.cmp(self))
+LL | |     }
+LL | | }
+   | |_^
+   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: you are implementing `Ord` explicitly but have derived `PartialOrd`
+  --> $DIR/derive_ord_xor_partial_ord.rs:41:1
+   |
+LL | / impl std::cmp::Ord for DerivePartialOrd {
+LL | |     fn cmp(&self, other: &Self) -> Ordering {
+LL | |         Ordering::Less
+LL | |     }
+LL | | }
+   | |_^
+   |
+note: `PartialOrd` implemented here
+  --> $DIR/derive_ord_xor_partial_ord.rs:38:10
+   |
+LL | #[derive(PartialOrd, PartialEq, Eq)]
+   |          ^^^^^^^^^^
+   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: you are implementing `Ord` explicitly but have derived `PartialOrd`
+  --> $DIR/derive_ord_xor_partial_ord.rs:61:5
+   |
+LL | /     impl Ord for DerivePartialOrdInUseOrd {
+LL | |         fn cmp(&self, other: &Self) -> Ordering {
+LL | |             Ordering::Less
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+note: `PartialOrd` implemented here
+  --> $DIR/derive_ord_xor_partial_ord.rs:58:14
+   |
+LL |     #[derive(PartialOrd, PartialEq, Eq)]
+   |              ^^^^^^^^^^
+   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 4 previous errors
+