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

Rename incorrect_impls to non_canonical_impls, move them to warn by default #11358

Merged
merged 1 commit into from Sep 9, 2023

Conversation

Alexendoo
Copy link
Member

@Alexendoo Alexendoo commented Aug 19, 2023

The wording/category of these feel too strong to me, I would expect most of the time it's linting the implementations aren't going to be incorrect, just unnecessary

changelog: rename incorrect_clone_impl_on_copy_type to [non_canonical_clone_impl]
changelog: rename incorrect_partial_ord_impl_on_ord_type to [non_canonical_partial_ord_impl]
changelog: Move [non_canonical_clone_impl], [non_canonical_partial_ord_impl] to suspicious

@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2023

r? @Jarcho

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 19, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Aug 20, 2023

Both of the lints are checking the function body for the most obvious implementation so the name and lint category are both justified.

@Alexendoo
Copy link
Member Author

(added a note mentioning that it's a requirement for Clone/Copy and Ord/PartialOrd to be consistent)

Not being the most obvious implementation doesn't make them incorrect, the before example in incorrect_clone_impl_on_copy_type is correct, just not in our canonical form

@Jarcho
Copy link
Contributor

Jarcho commented Aug 22, 2023

The lint on PartialOrd really would be better as suspicious. Assuming the impl is correct now, it's still something which could drift in the future when something changes.

manual_clone_impl_on_copy_type also doesn't really make sense as a name when expl_impl_clone_on_copy is already a lint. non_canonical_clone_impl would be a better name for what's actually happening.

@bors
Copy link
Collaborator

bors commented Aug 22, 2023

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

@Alexendoo
Copy link
Member Author

Whoops, apparently if you forget to commit your changes and push a commit equal to the target branch it'll close the PR for you

@Alexendoo Alexendoo reopened this Sep 7, 2023
@Alexendoo Alexendoo changed the title Rename incorrect_impls to manual_impls, move them to warn by default Rename incorrect_impls to non_canonical_impls, move them to warn by default Sep 7, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Sep 8, 2023

Yay, more perplexing behaviour from github!

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 8, 2023

📌 Commit 71e716a has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 8, 2023

⌛ Testing commit 71e716a with merge 25ac29f...

bors added a commit that referenced this pull request Sep 8, 2023
Rename incorrect_impls to non_canonical_impls, move them to warn by default

The wording/category of these feel too strong to me, I would expect most of the time it's linting the implementations aren't going to be *incorrect*, just unnecessary

changelog: rename `incorrect_clone_impl_on_copy_type` to [`non_canonical_clone_impl`]
changelog: rename `incorrect_partial_ord_impl_on_ord_type` to [`non_canonical_partial_ord_impl`]
changelog: Move [`non_canonical_clone_impl`], [`non_canonical_partial_ord_impl`] to suspicious
@bors
Copy link
Collaborator

bors commented Sep 8, 2023

💔 Test failed - checks-action_test

@Jarcho
Copy link
Contributor

Jarcho commented Sep 8, 2023

Looks like you need to re-bless the tests. Should also change the error message which still mentions that it's an incorrect impl.

@Alexendoo
Copy link
Member Author

That's odd.. I don't see that change locally

@Alexendoo
Copy link
Member Author

I do after rebasing 🤷

@Jarcho
Copy link
Contributor

Jarcho commented Sep 9, 2023

One more time. @bors r+

@bors
Copy link
Collaborator

bors commented Sep 9, 2023

📌 Commit b99921a has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 9, 2023

⌛ Testing commit b99921a with merge ec6f1bd...

@bors
Copy link
Collaborator

bors commented Sep 9, 2023

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

@bors bors merged commit ec6f1bd into rust-lang:master Sep 9, 2023
8 checks passed
@Alexendoo Alexendoo deleted the incorrect-to-manual-impls branch September 9, 2023 12:05
@sfackler
Copy link
Member

sfackler commented Oct 6, 2023

It looks like this missed the 1.73.0 release?

@Alexendoo
Copy link
Member Author

Yeah, it's still incorrect_... on 1.73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants