Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derived PartialOrd and implemented Ord #1621

Closed
ghost opened this issue Mar 14, 2017 · 3 comments · Fixed by #5848
Closed

Derived PartialOrd and implemented Ord #1621

ghost opened this issue Mar 14, 2017 · 3 comments · Fixed by #5848
Labels
good-first-issue These issues are a good way to get started with Clippy L-correctness Lint: Belongs in the correctness lint group L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types

Comments

@ghost
Copy link

ghost commented Mar 14, 2017

Suggestion: if a user writes #[derive(PartialOrd)] and then implements Ord manually, we should issue a warning. This is almost certainly wrong and will lead to problems.

The same applies to PartialEq and Eq.

See rust-lang/rust#40192 for an example.

@oli-obk oli-obk added L-correctness Lint: Belongs in the correctness lint group L-unnecessary Lint: Warn about unnecessary code good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types labels Mar 15, 2017
@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Dec 28, 2018

Derived PartialEq and implemented Eq is mostly harmless, I don't think it should be linted on. It's necessary if a structure has floating point types which can be proven to not store NaN, and there is no code in Eq trait.

Also, I think writing #[derive(Ord)] and implementing PartialOrd manually should be warned against as well (in fact, it probably should be denied by default - as there is no reason you ever would want to do that, while with #[derive(PartialOrd)] and implementing Ord I can see an use-case for implementing Ord for floating number types).

@KamilaBorowska
Copy link
Contributor

Specifically, I think this is the best approach about it:

  • impl PartialEq, #[derive(Eq)] - No warnings, deriving Eq is always fine as it has no extra methods.
  • #[derive(PartialEq)], impl Eq - Unnecessary warning, if it's not necessary (it's considered necessary if struct has generic parameters without Eq bound or has PartialEq-only types).
  • impl PartialOrd, #[derive(Ord)] - Correctness warning.
  • #[derive(PartialOrd)], impl Ord - Correctness warning if Ord implementation isn't trivial (an example of trivial implementation would be self.partial_cmp(other).unwrap()). Unnecessary warning if it's not necessary (it's considered necessary if struct has generic parameters without Ord bound or has PartialOrd-only types).

@Ryan1729
Copy link
Contributor

I would like to work on this. Specifically, I would like to create a new derive_ord_xor_partial_ord lint which would work analogously to the derive_hash_xor_partial_eq lint.

bors added a commit that referenced this issue Jul 28, 2020
…=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
bors added a commit that referenced this issue Jul 29, 2020
…=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
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Aug 4, 2020
…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#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
@bors bors closed this as completed in 888067c Aug 4, 2020
luckysori added a commit to get10101/itchysats that referenced this issue Jul 25, 2022
This is important because deriving `PartialOrd` means that it looks at
all the struct's fields in order. For this type we want to compare the
`timestamp`, but do not care about the `digits`! More importantly,
someone could unintentionally change this behaviour by reordering the
fields.

We also remove the `Ord` derived implementation because:

- It's unused.
- Deriving `Ord` when manually implementing `PartialOrd` is a very bad
  idea[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
luckysori added a commit to get10101/itchysats that referenced this issue Jul 25, 2022
This is important because deriving `PartialOrd` normally means that it
looks at all the struct's fields _in order_. For this type we want to
compare the `timestamp`, but do not care about the `digits`! More
importantly, someone could unintentionally change this behaviour by
reordering the fields.

We also have to ignore the `digits` field for other derived traits,
because inconsistencies there could cause trouble[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
luckysori added a commit to get10101/itchysats that referenced this issue Jul 26, 2022
This is important because deriving `PartialOrd` normally means that it
looks at all the struct's fields _in order_. For this type we want to
compare the `timestamp`, but do not care about the `digits`! More
importantly, someone could unintentionally change this behaviour by
reordering the fields.

We also have to ignore the `digits` field for other derived traits,
because inconsistencies there could cause trouble[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
luckysori added a commit to get10101/itchysats that referenced this issue Jul 26, 2022
This is important because deriving `PartialOrd` normally means that it
looks at all the struct's fields _in order_. For this type we want to
compare the `timestamp`, but do not care about the `digits`! More
importantly, someone could unintentionally change this behaviour by
reordering the fields.

We also have to ignore the `digits` field for other derived traits,
because inconsistencies there could cause trouble[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
luckysori added a commit to get10101/itchysats that referenced this issue Jul 26, 2022
This is important because deriving `PartialOrd` normally means that it
looks at all the struct's fields _in order_. For this type we want to
compare the `timestamp`, but do not care about the `digits`! More
importantly, someone could unintentionally change this behaviour by
reordering the fields.

We also have to ignore the `digits` field for other derived traits,
because inconsistencies there could cause trouble[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
luckysori added a commit to get10101/itchysats that referenced this issue Jul 26, 2022
This is important because deriving `PartialOrd` normally means that it
looks at all the struct's fields _in order_. For this type we want to
compare the `timestamp`, but do not care about the `digits`! More
importantly, someone could unintentionally change this behaviour by
reordering the fields.

We also have to ignore the `digits` field for other derived traits,
because inconsistencies there could cause trouble[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
luckysori added a commit to get10101/itchysats that referenced this issue Jul 26, 2022
This is important because deriving `PartialOrd` normally means that it
looks at all the struct's fields _in order_. For this type we want to
compare the `timestamp`, but do not care about the `digits`! More
importantly, someone could unintentionally change this behaviour by
reordering the fields.

We also have to ignore the `digits` field for other derived traits,
because inconsistencies there could cause trouble[1].

[1]: rust-lang/rust-clippy#1621.

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue These issues are a good way to get started with Clippy L-correctness Lint: Belongs in the correctness lint group L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants