New lint [`manual_partial_ord_and_ord_impl`] #10788

Centri3 · 2023-05-15T23:52:06Z

Lints when both PartialOrd and Ord are manually implemented yet the body of PartialOrd::partial_cmp isn't Some(self.cmp(<other>)).

This is in correctness currently but I'm ok with elsewhere.

Closes #10744

changelog: new lint [needless_partial_ord_impl]
#10788

rustbot · 2023-05-15T23:52:11Z

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

bors · 2023-05-23T22:13:36Z

☔ The latest upstream changes (presumably #10810) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2023-06-03T12:06:52Z

@blyxyas Would you mind reviewing this PR as well? It looks like a nice new lint!

r? @xFrednet

blyxyas · 2023-06-03T12:34:40Z

Yep, will also review it!

blyxyas · 2023-06-03T13:29:54Z

I feel like manual_partial_ord_and_ord_impl is pretty long and doesn't really convey anything. Maybe something like partial_ord_same_as_ord?

blyxyas · 2023-06-03T18:53:44Z

clippy_lints/src/manual_partial_ord_and_ord_impl.rs

+                                            "change this to",
+                                            format!("{{ Some(self.cmp({})) }}", param_ident.name),
+                                            // TODO: This is always correct afaik.
+                                            Applicability::MaybeIncorrect


I would probably use Applicability::Unspecified until we have some tool (likely a new implementation to lintcheck) to measure breaking changes when suggestions change in a bigger scope (instead of the 26 current crates, a bigger dataset).

blyxyas · 2023-06-03T19:07:52Z

tests/ui/manual_partial_ord_and_ord_impl.rs

What happens if the PartialOrd implementation has side effects? 🤔
In that case, PartialOrd cannot be replaced with Some(self.cmp(other))

(this is a very edgy edge-case)

impl Ord for A { fn cmp(&self, other: &Self) -> Ordering { todo!(); } } impl PartialOrd for A { fn partial_cmp(&self, other: &Self) -> Option<Ordering> { fn_with_side_effects(); todo!(); } }

That would break it, yes. Though I'm not sure if this is too big of an issue since implementations of PartialOrd and Ord must agree; I'm not sure if this extends to printing to stdout or mutating a static and whatnot but generally, this would be a pretty terrible idea anyway.

There's also derive_ord_xor_partial_ord, which is similar and disallows the implementation even if it has side effects afaict.

blyxyas · 2023-06-03T19:08:55Z

Sorry for taking so long to review, it looks like a very useful lint, even tho not a lot of people manually implement Ord and PartialOrd ^^

Centri3 · 2023-06-09T09:07:14Z

lintcheck reveals 4 instances of needless_partial_ord_impl and 1 instance of needless_clone_impl in the wild: 2 in the log crate, 2 in regex, and the one for needless_clone_impl from ryu. They seem correct, but the suggestion most certainly isn't. If somebody can take a closer look at these that'd be much appreciated :D

blyxyas · 2023-06-10T20:52:11Z

Could you split these two lints into two different PRs so they're easier to review and easier for generating the changelog? (and a simpler Git structure in general)

xFrednet · 2023-06-11T17:17:54Z

Could you split these two lints into two different PRs so they're easier to review and easier for generating the changelog? (and a simpler Git structure in general)

From the newly created PRs, it looks like the lints were quite different. So asking this is reasonable. For further reference, you can just add multiple changelog entries to a PR and I'll combine them:

changelog: new lint XYZ
changelog: new lint ZZZ

works just fine. Also, a small warning, that at one point you might encounter contributors, who are not as proficient with git as you are. And in this regard, just to add even more to this totally side tracked comment: I can recommend playing a bit with aliases for git if you use the console. I have a few for fast rebasing of just fixing up the last commit. :)

bors · 2023-06-12T04:36:49Z

☔ The latest upstream changes (presumably #10921) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-06-12T12:33:45Z

☔ The latest upstream changes (presumably #10827) made this pull request unmergeable. Please resolve the merge conflicts.

blyxyas

Some small changes and I think it's pretty ready to be merged 👍 (The other or self comment has to be addressed tho, I actually think it's a bug)

clippy_lints/src/needless_impls.rs

tests/ui/needless_partial_ord_impl.stderr

bors · 2023-06-15T00:25:44Z

☔ The latest upstream changes (presumably #10934) made this pull request unmergeable. Please resolve the merge conflicts.

blyxyas

I think this is complete. Thanks! ❤️
cc @xFrednet

xFrednet

Looks good overall, I have a few minor comments, and then it should be ready for bors consumption :D

clippy_lints/src/incorrect_impls.rs

xFrednet · 2023-07-05T20:52:39Z

tests/ui/incorrect_partial_ord_impl_on_ord_type.rs

+impl PartialOrd for B {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}


I've just checked and self.cmp(other).into() also wraps the result into Some. If it's simple to implement, can you also check for this kind of body and accept it as a correct implementation?

I feel like handling all cases like that is a bit challenging, you could also do Some(Self::cmp(self, other)) I'm pretty sure which should be correct but isn't caught

tests/ui/incorrect_partial_ord_impl_on_ord_type.rs

clippy_lints/src/incorrect_impls.rs

Centri3 · 2023-07-06T19:08:19Z

regex test failed, see #11111

bors · 2023-07-07T19:49:40Z

☔ The latest upstream changes (presumably #11122) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2023-07-07T20:35:08Z

This version looks good to me. Could you rebase on master and also squash the changes into 1-4 commits? Then everything should be ready. Here you can also r=blyxyas,xFrednet afterwards :)

Thank you for the update and review you two!

Bors, you know what to do :D

bors · 2023-07-07T20:35:10Z

✌️ @Centri3, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

first try at this

cargo dev fmt cargo test passes cargo test passes refactor a lil Update bool_comparison.stderr heavily refactor + bump `clippy::version` refactor refactor check bounds to increase accuracy, and add todos

Centri3 · 2023-07-08T18:22:48Z

@bors r=xFrednet,blyxyas

bors · 2023-07-08T18:22:49Z

📌 Commit a5dfb68 has been approved by xFrednet,blyxyas

It is now in the queue for this repository.

bors · 2023-07-08T18:22:57Z

⌛ Testing commit a5dfb68 with merge e5db198...

bors · 2023-07-08T18:38:51Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,blyxyas
Pushing e5db198 to master...

rustbot assigned llogiq May 15, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 15, 2023

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from f3b63ba to cbd63a5 Compare May 24, 2023 05:27

rustbot assigned xFrednet and unassigned llogiq Jun 3, 2023

xFrednet assigned blyxyas Jun 3, 2023

blyxyas reviewed Jun 3, 2023

View reviewed changes

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch 2 times, most recently from e4ac6ba to c10fc0f Compare June 5, 2023 09:48

Centri3 mentioned this pull request Jun 6, 2023

Clone should be *self for types which are always Copy #10700

Closed

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from 1deaa44 to 7f43e0f Compare June 9, 2023 09:09

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch 2 times, most recently from 0a7ec70 to cfcf04b Compare June 11, 2023 07:53

Centri3 mentioned this pull request Jun 11, 2023

add lint [incorrect_clone_impl_on_copy_type] #10925

Merged

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch 2 times, most recently from 2f48612 to f3875c7 Compare June 12, 2023 10:40

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from 40227bb to 1af0124 Compare June 12, 2023 16:35

blyxyas requested changes Jun 12, 2023

View reviewed changes

clippy_lints/src/needless_impls.rs Outdated Show resolved Hide resolved

clippy_lints/src/needless_impls.rs Outdated Show resolved Hide resolved

tests/ui/needless_partial_ord_impl.stderr Outdated Show resolved Hide resolved

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from c5c1a77 to c515a30 Compare June 18, 2023 17:30

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from c515a30 to f2cccc2 Compare June 30, 2023 20:17

blyxyas approved these changes Jul 5, 2023

View reviewed changes

xFrednet reviewed Jul 5, 2023

View reviewed changes

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch 2 times, most recently from 2a3354f to 8f302dc Compare July 6, 2023 03:09

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from 8f302dc to 862f82b Compare July 7, 2023 01:28

xFrednet approved these changes Jul 7, 2023

View reviewed changes

Centri3 added 4 commits July 8, 2023 13:02

implement manual_partial_ord_impl

2a1fd22

first try at this

rename to manual_partial_ord_and_ord_impl

004e89d

cargo dev fmt cargo test passes cargo test passes refactor a lil Update bool_comparison.stderr heavily refactor + bump `clippy::version` refactor refactor check bounds to increase accuracy, and add todos

use other instead of self

844afbf

refactor

a5dfb68

Centri3 force-pushed the duplicate_manual_partial_ord_impl branch from 862f82b to a5dfb68 Compare July 8, 2023 18:12

bors merged commit e5db198 into rust-lang:master Jul 8, 2023

Centri3 deleted the duplicate_manual_partial_ord_impl branch July 8, 2023 18:39

dtolnay mentioned this pull request Jul 18, 2023

incorrect_partial_ord_impl_on_ord_type false positive: Some(Ord::cmp(self, other)) #11178

Closed

blyxyas mentioned this pull request Oct 3, 2023

Changelog for Rust 1.73 🖊️ #11605

Merged

New lint [manual_partial_ord_and_ord_impl] #10788

New lint [manual_partial_ord_and_ord_impl] #10788

Uh oh!

Conversation

Centri3 commented May 15, 2023 • edited by xFrednet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 15, 2023

Uh oh!

bors commented May 23, 2023

Uh oh!

xFrednet commented Jun 3, 2023

Uh oh!

blyxyas commented Jun 3, 2023

Uh oh!

blyxyas commented Jun 3, 2023

Uh oh!

blyxyas Jun 3, 2023

Choose a reason for hiding this comment

Uh oh!

blyxyas Jun 3, 2023

Choose a reason for hiding this comment

Uh oh!

Centri3 Jun 3, 2023

Choose a reason for hiding this comment

Uh oh!

blyxyas commented Jun 3, 2023

Uh oh!

Centri3 commented Jun 9, 2023

Uh oh!

blyxyas commented Jun 10, 2023

Uh oh!

xFrednet commented Jun 11, 2023

Uh oh!

bors commented Jun 12, 2023

Uh oh!

bors commented Jun 12, 2023

Uh oh!

blyxyas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Jun 15, 2023

Uh oh!

blyxyas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xFrednet Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Centri3 Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Centri3 commented Jul 6, 2023

Uh oh!

bors commented Jul 7, 2023

Uh oh!

xFrednet commented Jul 7, 2023

Uh oh!

bors commented Jul 7, 2023

Uh oh!

Centri3 commented Jul 8, 2023

Uh oh!

bors commented Jul 8, 2023

Uh oh!

bors commented Jul 8, 2023

Uh oh!

bors commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

New lint [`manual_partial_ord_and_ord_impl`] #10788

New lint [`manual_partial_ord_and_ord_impl`] #10788

Centri3 commented May 15, 2023 •

edited by xFrednet

Loading

blyxyas left a comment •

edited

Loading