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.

Sign up for GitHub

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

Jump to bottom

Allow Self::cmp(self, other) as a correct impl #11188

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Allow Self::cmp(self, other) as a correct impl #11188

merged 1 commit into from
Jul 20, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jul 19, 2023

Fixes #11178

Also no longer checks if the method name is just cmp, but the path. That was an oversight on my part ^^

r? @xFrednet
(and @blyxyas too!)

changelog: [incorrect_partial_ord_impl_on_ord_type]: Now allows non-method calls to cmp like Self::cmp(self, other)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 19, 2023
@Centri3
Copy link
Member Author

Centri3 commented Jul 19, 2023

This got into beta (https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=a2f01f29c980dc6da61c1ef714cc9709), so perhaps it should be backported?

@rustbot label +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 19, 2023
blyxyas
blyxyas reviewed Jul 19, 2023
View reviewed changes
clippy_lints/src/incorrect_impls.rs Outdated Show resolved Hide resolved
clippy_utils/src/paths.rs
@@ -161,3 +161,4 @@ pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"];
pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"];
pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];
pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't sym::cmp work?

Copy link
Member Author

@Centri3 Centri3 Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, take a look at this code for example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=bb80a74abe4315e2379584abce972f3b

If it both defines a method cmp and also implements Ord, and Ord is not in scope (due to #![no_implicit_prelude]), it'll call the method instead - not Ord::cmp. Yet it still doesn't lint

But the suggestion is wrong and should be fixed...

@Centri3 Centri3 force-pushed the #11178 branch 2 times, most recently from f61b46a to 91d1767 Compare July 19, 2023 19:11
@kmeisthax kmeisthax mentioned this pull request Jul 20, 2023
Closed
@xFrednet
Copy link
Member

Hey @blyxyas can I give you this review? (I'm visiting my hometown for the next 3 weeks. It's totally fine if not, I should have enough time :))

@blyxyas
Copy link
Member

blyxyas commented Jul 20, 2023

Skill issue.
@bors r? @blyxyas

@rustbot rustbot assigned blyxyas and unassigned xFrednet Jul 20, 2023
@xFrednet
Copy link
Member

IDK, if having you do my work so that I can have more free time is a skill issue 😉 /s

I appreciate it! This way I'll have some time to work on Marker :D

@blyxyas
Copy link
Member

blyxyas commented Jul 20, 2023 •
edited
Loading

IDK, if having you do my work so that I can have more free time is a skill issue

I mean, phrasing it like that sounds like I'm a loser /s

@xFrednet
Copy link
Member

Meaning it was a good come back, after you destroyed me ^^

blyxyas
blyxyas reviewed Jul 20, 2023
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just going to wait until CI is cleared to merge ^^

@blyxyas
Copy link
Member

blyxyas commented Jul 20, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 20, 2023

📌 Commit a4c367d has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit a4c367d with merge ff21287...

bors added a commit that referenced this pull request Jul 20, 2023
@bors
Auto merge of #11188 - Centri3:#11178, r=blyxyas
ff21287 
Allow `Self::cmp(self, other)` as a correct impl

Fixes #11178

Also no longer checks if the method name is *just* cmp, but the path. That was an oversight on my part ^^

r? `@xFrednet`
(and `@blyxyas` too!)

changelog: [`incorrect_partial_ord_impl_on_ord_type`]: Now allows non-method calls to `cmp` like `Self::cmp(self, other)`
@blyxyas
Copy link
Member

blyxyas commented Jul 20, 2023

@bors r-

@blyxyas
Copy link
Member

blyxyas commented Jul 20, 2023

@xFrednet you said to give this a review, but not about merging. Do you want to do a second-check or do I just merge?

@bors
Copy link
Contributor

bors commented Jul 20, 2023

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: ff21287 (ff21287236547b30e8ce6d0cc35a6d8c55a525cc)

@xFrednet
Copy link
Member

I meant that you take over the full review ^^. But I'm happy to delegate your orders or bors:

Dear lord bors, I'm pleased to announce that we have two new members in our powerful alliance. After their training, we have full trust in their magic powers. You already got the letter pigeon, informing you that they received r+ rights on this magnificent repository.

I'm happy to pass along the r+ from princess @blyxyas (who is apparently more skilled in time management than me). During the quest to review #11188, she has slain multiple bugs. Her support @Centri3 artfully crafted multiple commits, therefore fortifying our stronghold against the gobbling.

Please honer their contributions, by merging their changes into the great history book called MASTER

You know, I'm always not sure if these commend are funny or come off as totally deranged. 😅 Just for the protocol, the intention is to be funny, humor can be weird ¯\_(ツ)_/¯

@bors
Copy link
Contributor

bors commented Jul 20, 2023

📌 Commit a4c367d has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit a4c367d with merge ee8a429...

@bors
Copy link
Contributor

bors commented Jul 20, 2023

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

@bors bors merged commit ee8a429 into rust-lang:master Jul 20, 2023
@Centri3 Centri3 deleted the #11178 branch July 21, 2023 00:11
@flip1995 flip1995 removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 17, 2023
@flip1995
Copy link
Member

No backport necessary: This lint isn't in beta yet and this fix is already synced.

wbinnssmith added a commit to vercel/turborepo that referenced this pull request Oct 6, 2023
@wbinnssmith
Update rust-toolchain to nightly-2023-10-06
f352b3c 
This:
- Updates rust-toolchain to nightly-2023-10-06
- Removes allowing clippy rules for needless_pass_by_ref_mut and non_canonical_partial_ord_impl as the linked issues appear to have been resolved in [0] and [1] respectively
- Fixes apparently legitimate issues now reported by clippy

Test Plan: CI

[0] rust-lang/rust-clippy#11492
[1] rust-lang/rust-clippy#11188
@wbinnssmith wbinnssmith mentioned this pull request Oct 6, 2023
Merged
wbinnssmith added a commit to vercel/turborepo that referenced this pull request Oct 10, 2023
@wbinnssmith
Update rust-toolchain to nightly-2023-10-06 (#6128)
b3f79b1 
This:
- Updates rust-toolchain to nightly-2023-10-06
- Removes allowing clippy rules for needless_pass_by_ref_mut and
non_canonical_partial_ord_impl as the linked issues appear to have been
resolved in [0] and [1] respectively
- Fixes apparently legitimate issues now reported by clippy

Test Plan: CI

[0] rust-lang/rust-clippy#11492
[1] rust-lang/rust-clippy#11188


Closes WEB-1732
Zertsov pushed a commit to vercel/turborepo that referenced this pull request Oct 11, 2023
@wbinnssmith @Zertsov
Update rust-toolchain to nightly-2023-10-06 (#6128)
68fd985 
This:
- Updates rust-toolchain to nightly-2023-10-06
- Removes allowing clippy rules for needless_pass_by_ref_mut and
non_canonical_partial_ord_impl as the linked issues appear to have been
resolved in [0] and [1] respectively
- Fixes apparently legitimate issues now reported by clippy

Test Plan: CI

[0] rust-lang/rust-clippy#11492
[1] rust-lang/rust-clippy#11188


Closes WEB-1732
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
@wbinnssmith
Update rust-toolchain to nightly-2023-10-06 (vercel/turborepo#6128)
7bfc970 
This:
- Updates rust-toolchain to nightly-2023-10-06
- Removes allowing clippy rules for needless_pass_by_ref_mut and
non_canonical_partial_ord_impl as the linked issues appear to have been
resolved in [0] and [1] respectively
- Fixes apparently legitimate issues now reported by clippy

Test Plan: CI

[0] rust-lang/rust-clippy#11492
[1] rust-lang/rust-clippy#11188


Closes WEB-1732
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
@wbinnssmith
Update rust-toolchain to nightly-2023-10-06 (vercel/turborepo#6128)
c517e19 
This:
- Updates rust-toolchain to nightly-2023-10-06
- Removes allowing clippy rules for needless_pass_by_ref_mut and
non_canonical_partial_ord_impl as the linked issues appear to have been
resolved in [0] and [1] respectively
- Fixes apparently legitimate issues now reported by clippy

Test Plan: CI

[0] rust-lang/rust-clippy#11492
[1] rust-lang/rust-clippy#11188


Closes WEB-1732
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
@wbinnssmith
Update rust-toolchain to nightly-2023-10-06 (vercel/turborepo#6128)
a52e169 
This:
- Updates rust-toolchain to nightly-2023-10-06
- Removes allowing clippy rules for needless_pass_by_ref_mut and
non_canonical_partial_ord_impl as the linked issues appear to have been
resolved in [0] and [1] respectively
- Fixes apparently legitimate issues now reported by clippy

Test Plan: CI

[0] rust-lang/rust-clippy#11492
[1] rust-lang/rust-clippy#11188


Closes WEB-1732
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas approved these changes

Assignees

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

incorrect_partial_ord_impl_on_ord_type false positive: Some(Ord::cmp(self, other))
6 participants
@Centri3 @xFrednet @blyxyas @bors @flip1995 @rustbot