-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add derive_ord_xor_partial_ord lint #5848
Add derive_ord_xor_partial_ord lint #5848
Conversation
specifically: cargo dev new_lint --name derive_ord_xor_partial_ord --category correctness --pass late
…e_hash_xor_partial_eq code later
… which is showing one error when there should be four
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. |
Thanks! |
📌 Commit 94b10a6 has been approved by |
…=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
💔 Test failed - checks-action_test |
@bors retry |
…=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
💔 Test failed - checks-action_test |
Tests around |
@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
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
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
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
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
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 identifyOrd
implementation like the derive_hash_xor_partial_eq lint currently does, forHash
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 identifyOrd
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 comparingDefId
values directly and that seems to work as expected. Maybe there's a better approach however?changelog: new lint: derive_ord_xor_partial_ord