diff --git a/CHANGELOG.md b/CHANGELOG.md index 776b04295f94..a17807250443 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index 59c62f1ae944..08d8100a8854 100644 --- a/clippy_lints/src/derive.rs +++ b/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 { + /// 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 for Foo` + // For `impl PartialOrd 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) { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f371942dbeec..996aad31d3e1 100644 --- a/clippy_lints/src/lib.rs +++ b/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), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 1879aae77fb6..119908b3cc45 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -360,6 +360,13 @@ pub static ref ALL_LINTS: Vec = 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", diff --git a/tests/ui/derive_ord_xor_partial_ord.rs b/tests/ui/derive_ord_xor_partial_ord.rs new file mode 100644 index 000000000000..b82dc518a3ba --- /dev/null +++ b/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 for DeriveBoth { + fn eq(&self, _: &u64) -> bool { + true + } +} + +impl PartialOrd for DeriveBoth { + fn partial_cmp(&self, _: &u64) -> Option { + Some(Ordering::Equal) + } +} + +#[derive(Ord, PartialEq, Eq)] +struct DeriveOrd; + +impl PartialOrd for DeriveOrd { + fn partial_cmp(&self, other: &Self) -> Option { + Some(other.cmp(self)) + } +} + +#[derive(Ord, PartialEq, Eq)] +struct DeriveOrdWithExplicitTypeVariable; + +impl PartialOrd for DeriveOrdWithExplicitTypeVariable { + fn partial_cmp(&self, other: &Self) -> Option { + 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() {} diff --git a/tests/ui/derive_ord_xor_partial_ord.stderr b/tests/ui/derive_ord_xor_partial_ord.stderr new file mode 100644 index 000000000000..66bc4d42ce8c --- /dev/null +++ b/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 { +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 for DeriveOrdWithExplicitTypeVariable { +LL | | fn partial_cmp(&self, other: &Self) -> Option { +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 +