Skip to content

Commit 888067c

Browse files
flip1995flip1995
authored
Rollup merge of rust-lang#5848 - Ryan1729:add-derive_ord_xor_partial_ord-lint, r=matthiaskrgr
Add derive_ord_xor_partial_ord lint Fix rust-lang#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](rust-lang/rust-clippy#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
2 parents 378ba2e + 94b10a6 commit 888067c

File tree

6 files changed

+264
-2
lines changed

6 files changed

+264
-2
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -1454,6 +1454,7 @@ Released 2018-09-13
14541454
[`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
14551455
[`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
14561456
[`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
1457+
[`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
14571458
[`diverging_sub_expression`]: https://rust-lang.github.io/rust-clippy/master/index.html#diverging_sub_expression
14581459
[`doc_markdown`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown
14591460
[`double_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_comparisons

‎clippy_lints/src/derive.rs

+114-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use crate::utils::paths;
22
use crate::utils::{
3-
is_automatically_derived, is_copy, match_path, span_lint_and_help, span_lint_and_note, span_lint_and_then,
3+
get_trait_def_id, is_automatically_derived, is_copy, match_path, span_lint_and_help, span_lint_and_note,
4+
span_lint_and_then,
45
};
56
use if_chain::if_chain;
67
use rustc_hir::def_id::DefId;
@@ -43,6 +44,57 @@ declare_clippy_lint! {
4344
"deriving `Hash` but implementing `PartialEq` explicitly"
4445
}
4546

47+
declare_clippy_lint! {
48+
/// **What it does:** Checks for deriving `Ord` but implementing `PartialOrd`
49+
/// explicitly or vice versa.
50+
///
51+
/// **Why is this bad?** The implementation of these traits must agree (for
52+
/// example for use with `sort`) so it’s probably a bad idea to use a
53+
/// default-generated `Ord` implementation with an explicitly defined
54+
/// `PartialOrd`. In particular, the following must hold for any type
55+
/// implementing `Ord`:
56+
///
57+
/// ```text
58+
/// k1.cmp(&k2) == k1.partial_cmp(&k2).unwrap()
59+
/// ```
60+
///
61+
/// **Known problems:** None.
62+
///
63+
/// **Example:**
64+
///
65+
/// ```rust,ignore
66+
/// #[derive(Ord, PartialEq, Eq)]
67+
/// struct Foo;
68+
///
69+
/// impl PartialOrd for Foo {
70+
/// ...
71+
/// }
72+
/// ```
73+
/// Use instead:
74+
/// ```rust,ignore
75+
/// #[derive(PartialEq, Eq)]
76+
/// struct Foo;
77+
///
78+
/// impl PartialOrd for Foo {
79+
/// fn partial_cmp(&self, other: &Foo) -> Option<Ordering> {
80+
/// Some(self.cmp(other))
81+
/// }
82+
/// }
83+
///
84+
/// impl Ord for Foo {
85+
/// ...
86+
/// }
87+
/// ```
88+
/// or, if you don't need a custom ordering:
89+
/// ```rust,ignore
90+
/// #[derive(Ord, PartialOrd, PartialEq, Eq)]
91+
/// struct Foo;
92+
/// ```
93+
pub DERIVE_ORD_XOR_PARTIAL_ORD,
94+
correctness,
95+
"deriving `Ord` but implementing `PartialOrd` explicitly"
96+
}
97+
4698
declare_clippy_lint! {
4799
/// **What it does:** Checks for explicit `Clone` implementations for `Copy`
48100
/// types.
@@ -103,7 +155,12 @@ declare_clippy_lint! {
103155
"deriving `serde::Deserialize` on a type that has methods using `unsafe`"
104156
}
105157

106-
declare_lint_pass!(Derive => [EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_XOR_EQ, UNSAFE_DERIVE_DESERIALIZE]);
158+
declare_lint_pass!(Derive => [
159+
EXPL_IMPL_CLONE_ON_COPY,
160+
DERIVE_HASH_XOR_EQ,
161+
DERIVE_ORD_XOR_PARTIAL_ORD,
162+
UNSAFE_DERIVE_DESERIALIZE
163+
]);
107164

108165
impl<'tcx> LateLintPass<'tcx> for Derive {
109166
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
@@ -116,6 +173,7 @@ impl<'tcx> LateLintPass<'tcx> for Derive {
116173
let is_automatically_derived = is_automatically_derived(&*item.attrs);
117174

118175
check_hash_peq(cx, item.span, trait_ref, ty, is_automatically_derived);
176+
check_ord_partial_ord(cx, item.span, trait_ref, ty, is_automatically_derived);
119177

120178
if is_automatically_derived {
121179
check_unsafe_derive_deserialize(cx, item, trait_ref, ty);
@@ -180,6 +238,60 @@ fn check_hash_peq<'tcx>(
180238
}
181239
}
182240

241+
/// Implementation of the `DERIVE_ORD_XOR_PARTIAL_ORD` lint.
242+
fn check_ord_partial_ord<'tcx>(
243+
cx: &LateContext<'tcx>,
244+
span: Span,
245+
trait_ref: &TraitRef<'_>,
246+
ty: Ty<'tcx>,
247+
ord_is_automatically_derived: bool,
248+
) {
249+
if_chain! {
250+
if let Some(ord_trait_def_id) = get_trait_def_id(cx, &paths::ORD);
251+
if let Some(partial_ord_trait_def_id) = cx.tcx.lang_items().partial_ord_trait();
252+
if let Some(def_id) = &trait_ref.trait_def_id();
253+
if *def_id == ord_trait_def_id;
254+
then {
255+
// Look for the PartialOrd implementations for `ty`
256+
cx.tcx.for_each_relevant_impl(partial_ord_trait_def_id, ty, |impl_id| {
257+
let partial_ord_is_automatically_derived = is_automatically_derived(&cx.tcx.get_attrs(impl_id));
258+
259+
if partial_ord_is_automatically_derived == ord_is_automatically_derived {
260+
return;
261+
}
262+
263+
let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
264+
265+
// Only care about `impl PartialOrd<Foo> for Foo`
266+
// For `impl PartialOrd<B> for A, input_types is [A, B]
267+
if trait_ref.substs.type_at(1) == ty {
268+
let mess = if partial_ord_is_automatically_derived {
269+
"you are implementing `Ord` explicitly but have derived `PartialOrd`"
270+
} else {
271+
"you are deriving `Ord` but have implemented `PartialOrd` explicitly"
272+
};
273+
274+
span_lint_and_then(
275+
cx,
276+
DERIVE_ORD_XOR_PARTIAL_ORD,
277+
span,
278+
mess,
279+
|diag| {
280+
if let Some(local_def_id) = impl_id.as_local() {
281+
let hir_id = cx.tcx.hir().as_local_hir_id(local_def_id);
282+
diag.span_note(
283+
cx.tcx.hir().span(hir_id),
284+
"`PartialOrd` implemented here"
285+
);
286+
}
287+
}
288+
);
289+
}
290+
});
291+
}
292+
}
293+
}
294+
183295
/// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint.
184296
fn check_copy_clone<'tcx>(cx: &LateContext<'tcx>, item: &Item<'_>, trait_ref: &TraitRef<'_>, ty: Ty<'tcx>) {
185297
if match_path(&trait_ref.path, &paths::CLONE_TRAIT) {

‎clippy_lints/src/lib.rs

+3
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
513513
&default_trait_access::DEFAULT_TRAIT_ACCESS,
514514
&dereference::EXPLICIT_DEREF_METHODS,
515515
&derive::DERIVE_HASH_XOR_EQ,
516+
&derive::DERIVE_ORD_XOR_PARTIAL_ORD,
516517
&derive::EXPL_IMPL_CLONE_ON_COPY,
517518
&derive::UNSAFE_DERIVE_DESERIALIZE,
518519
&doc::DOC_MARKDOWN,
@@ -1230,6 +1231,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12301231
LintId::of(&copies::IFS_SAME_COND),
12311232
LintId::of(&copies::IF_SAME_THEN_ELSE),
12321233
LintId::of(&derive::DERIVE_HASH_XOR_EQ),
1234+
LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD),
12331235
LintId::of(&doc::MISSING_SAFETY_DOC),
12341236
LintId::of(&doc::NEEDLESS_DOCTEST_MAIN),
12351237
LintId::of(&double_comparison::DOUBLE_COMPARISONS),
@@ -1648,6 +1650,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
16481650
LintId::of(&copies::IFS_SAME_COND),
16491651
LintId::of(&copies::IF_SAME_THEN_ELSE),
16501652
LintId::of(&derive::DERIVE_HASH_XOR_EQ),
1653+
LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD),
16511654
LintId::of(&drop_bounds::DROP_BOUNDS),
16521655
LintId::of(&drop_forget_ref::DROP_COPY),
16531656
LintId::of(&drop_forget_ref::DROP_REF),

‎src/lintlist/mod.rs

+7
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
360360
deprecation: None,
361361
module: "derive",
362362
},
363+
Lint {
364+
name: "derive_ord_xor_partial_ord",
365+
group: "correctness",
366+
desc: "deriving `Ord` but implementing `PartialOrd` explicitly",
367+
deprecation: None,
368+
module: "derive",
369+
},
363370
Lint {
364371
name: "diverging_sub_expression",
365372
group: "complexity",

‎tests/ui/derive_ord_xor_partial_ord.rs

+68
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
#![warn(clippy::derive_ord_xor_partial_ord)]
2+
3+
use std::cmp::Ordering;
4+
5+
#[derive(PartialOrd, Ord, PartialEq, Eq)]
6+
struct DeriveBoth;
7+
8+
impl PartialEq<u64> for DeriveBoth {
9+
fn eq(&self, _: &u64) -> bool {
10+
true
11+
}
12+
}
13+
14+
impl PartialOrd<u64> for DeriveBoth {
15+
fn partial_cmp(&self, _: &u64) -> Option<Ordering> {
16+
Some(Ordering::Equal)
17+
}
18+
}
19+
20+
#[derive(Ord, PartialEq, Eq)]
21+
struct DeriveOrd;
22+
23+
impl PartialOrd for DeriveOrd {
24+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
25+
Some(other.cmp(self))
26+
}
27+
}
28+
29+
#[derive(Ord, PartialEq, Eq)]
30+
struct DeriveOrdWithExplicitTypeVariable;
31+
32+
impl PartialOrd<DeriveOrdWithExplicitTypeVariable> for DeriveOrdWithExplicitTypeVariable {
33+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
34+
Some(other.cmp(self))
35+
}
36+
}
37+
38+
#[derive(PartialOrd, PartialEq, Eq)]
39+
struct DerivePartialOrd;
40+
41+
impl std::cmp::Ord for DerivePartialOrd {
42+
fn cmp(&self, other: &Self) -> Ordering {
43+
Ordering::Less
44+
}
45+
}
46+
47+
#[derive(PartialOrd, PartialEq, Eq)]
48+
struct ImplUserOrd;
49+
50+
trait Ord {}
51+
52+
// We don't want to lint on user-defined traits called `Ord`
53+
impl Ord for ImplUserOrd {}
54+
55+
mod use_ord {
56+
use std::cmp::{Ord, Ordering};
57+
58+
#[derive(PartialOrd, PartialEq, Eq)]
59+
struct DerivePartialOrdInUseOrd;
60+
61+
impl Ord for DerivePartialOrdInUseOrd {
62+
fn cmp(&self, other: &Self) -> Ordering {
63+
Ordering::Less
64+
}
65+
}
66+
}
67+
68+
fn main() {}

‎tests/ui/derive_ord_xor_partial_ord.stderr

+71
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
error: you are deriving `Ord` but have implemented `PartialOrd` explicitly
2+
--> $DIR/derive_ord_xor_partial_ord.rs:20:10
3+
|
4+
LL | #[derive(Ord, PartialEq, Eq)]
5+
| ^^^
6+
|
7+
= note: `-D clippy::derive-ord-xor-partial-ord` implied by `-D warnings`
8+
note: `PartialOrd` implemented here
9+
--> $DIR/derive_ord_xor_partial_ord.rs:23:1
10+
|
11+
LL | / impl PartialOrd for DeriveOrd {
12+
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
13+
LL | | Some(other.cmp(self))
14+
LL | | }
15+
LL | | }
16+
| |_^
17+
= note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
18+
19+
error: you are deriving `Ord` but have implemented `PartialOrd` explicitly
20+
--> $DIR/derive_ord_xor_partial_ord.rs:29:10
21+
|
22+
LL | #[derive(Ord, PartialEq, Eq)]
23+
| ^^^
24+
|
25+
note: `PartialOrd` implemented here
26+
--> $DIR/derive_ord_xor_partial_ord.rs:32:1
27+
|
28+
LL | / impl PartialOrd<DeriveOrdWithExplicitTypeVariable> for DeriveOrdWithExplicitTypeVariable {
29+
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
30+
LL | | Some(other.cmp(self))
31+
LL | | }
32+
LL | | }
33+
| |_^
34+
= note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
35+
36+
error: you are implementing `Ord` explicitly but have derived `PartialOrd`
37+
--> $DIR/derive_ord_xor_partial_ord.rs:41:1
38+
|
39+
LL | / impl std::cmp::Ord for DerivePartialOrd {
40+
LL | | fn cmp(&self, other: &Self) -> Ordering {
41+
LL | | Ordering::Less
42+
LL | | }
43+
LL | | }
44+
| |_^
45+
|
46+
note: `PartialOrd` implemented here
47+
--> $DIR/derive_ord_xor_partial_ord.rs:38:10
48+
|
49+
LL | #[derive(PartialOrd, PartialEq, Eq)]
50+
| ^^^^^^^^^^
51+
= note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
52+
53+
error: you are implementing `Ord` explicitly but have derived `PartialOrd`
54+
--> $DIR/derive_ord_xor_partial_ord.rs:61:5
55+
|
56+
LL | / impl Ord for DerivePartialOrdInUseOrd {
57+
LL | | fn cmp(&self, other: &Self) -> Ordering {
58+
LL | | Ordering::Less
59+
LL | | }
60+
LL | | }
61+
| |_____^
62+
|
63+
note: `PartialOrd` implemented here
64+
--> $DIR/derive_ord_xor_partial_ord.rs:58:14
65+
|
66+
LL | #[derive(PartialOrd, PartialEq, Eq)]
67+
| ^^^^^^^^^^
68+
= note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
69+
70+
error: aborting due to 4 previous errors
71+

0 commit comments

Comments
 (0)