Add derive_ord_xor_partial_ord lint #5848

Ryan1729 · 2020-07-27T05:55:25Z

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 that mentioned this issue 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

specifically: cargo dev new_lint --name derive_ord_xor_partial_ord --category correctness --pass late

…e_hash_xor_partial_eq code later

…_xor_partial_eq

… which is showing one error when there should be four

…cessary

rust-highfive · 2020-07-27T05:55:28Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthiaskrgr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

matthiaskrgr · 2020-07-28T23:13:03Z

Thanks!
@bors r+

bors · 2020-07-28T23:13:04Z

📌 Commit 94b10a6 has been approved by matthiaskrgr

bors · 2020-07-28T23:13:12Z

⌛ Testing commit 94b10a6 with merge c896119...

…=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 · 2020-07-28T23:20:32Z

💔 Test failed - checks-action_test

matthiaskrgr · 2020-07-29T17:00:57Z

@bors retry

bors · 2020-07-29T17:01:05Z

⌛ Testing commit 94b10a6 with merge 4bc4fd9...

…=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 · 2020-07-29T17:29:21Z

💔 Test failed - checks-action_test

matthiaskrgr · 2020-08-03T11:56:06Z

Tests around tests/ui-toml/conf_deprecated_key/clippy.toml are failing for some reason.

flip1995 · 2020-08-04T09:10:52Z

@bors retry rollup=always

…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

@ghost

Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

@ghost

Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

@ghost

Rollup of 5 pull requests Successful merges: - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost

@ghost

Rollup of 5 pull requests Successful merges: - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

Ryan1729 added 9 commits July 26, 2020 20:40

run cargo dev new_lint

5a64496

specifically: cargo dev new_lint --name derive_ord_xor_partial_ord --category correctness --pass late

move derive_ord_xor_partial_ord into derive mod so we can reuse deriv…

fc20ee6

…e_hash_xor_partial_eq code later

add test for derive_ord_xor_partial_ord based on test for derive_hash…

0722991

…_xor_partial_eq

initial implementation based on code for derive_hash_xor_partial_eq…

068acbd

… which is showing one error when there should be four

use get_trait_def_id to check for Ord trait

a8d6eda

update reference since we see the expected four errors

6c3e459

remove is_local check since getting the def_id directly makes it unne…

7dc9748

…cessary

add description for derive_ord_xor_partial_ord

431924c

run cargo dev fmt and fix overly long line

668b747

rust-highfive assigned matthiaskrgr Jul 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 27, 2020

Ryan1729 added 3 commits July 27, 2020 00:21

s/pord/partial_ord/ to fix dogfood failure

ca03f2b

fill in lint description for DERIVE_ORD_XOR_PARTIAL_ORD

12a6eee

run cargo dev update_lints

94b10a6

matthiaskrgr added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 28, 2020

matthiaskrgr added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-bors Status: The marked PR was approved and is only waiting bors labels Jul 31, 2020

flip1995 removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Aug 3, 2020

flip1995 added the S-waiting-on-bors Status: The marked PR was approved and is only waiting bors label Aug 3, 2020

flip1995 mentioned this pull request Aug 4, 2020

Rollup of 6 pull requests #5866

Closed

flip1995 mentioned this pull request Aug 4, 2020

Rollup of 5 pull requests #5868

Merged

bors merged commit 888067c into rust-lang:master Aug 4, 2020

lo48576 mentioned this pull request Sep 18, 2020

False positive in derive_hash_xor_eq with multiple Eq implementations #2025

Open

ebroto mentioned this pull request Oct 8, 2020

ICE with wasm target #6140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add derive_ord_xor_partial_ord lint #5848

Add derive_ord_xor_partial_ord lint #5848

Ryan1729 commented Jul 27, 2020

rust-highfive commented Jul 27, 2020

matthiaskrgr commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

matthiaskrgr commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

matthiaskrgr commented Aug 3, 2020

flip1995 commented Aug 4, 2020

Add derive_ord_xor_partial_ord lint #5848

Add derive_ord_xor_partial_ord lint #5848

Conversation

Ryan1729 commented Jul 27, 2020

rust-highfive commented Jul 27, 2020

matthiaskrgr commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

matthiaskrgr commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

matthiaskrgr commented Aug 3, 2020

flip1995 commented Aug 4, 2020