Rollup 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](f5d429cd76/clippy_lints/src/utils/paths.rs (L5)) that mentioned [this issue](https://github.com/rust-lang/rust-clippy/issues/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
This commit is contained in:
Philipp Krones
GitHub
commit
888067c623
6 changed files with 264 additions and 2 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
68
tests/ui/derive_ord_xor_partial_ord.rs
Normal file
68
tests/ui/derive_ord_xor_partial_ord.rs
Normal file
|
|
@ -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() {}
|
||||
71
tests/ui/derive_ord_xor_partial_ord.stderr
Normal file
71
tests/ui/derive_ord_xor_partial_ord.stderr
Normal file
|
|
@ -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
|
||||
|
||||
Loading…
Add table
Add a link
Reference in a new issue