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

Add lint equatable_if_let #7762

Merged
merged 1 commit into from
Oct 4, 2021
Merged

Add lint equatable_if_let #7762

merged 1 commit into from
Oct 4, 2021

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Oct 4, 2021

This is my attempt for #1716. There is a major false positive, which is people may implement PartialEq in a different way. It is unactionable at the moment so I put it into nursery.

There is a trait StructuralPartialEq for solving this problem which is promising but it has several problems currently:

  • Integers and tuples doesn't implement it.
  • Some types wrongly implement it, like Option<T> when T doesn't implement it.

I consider them bugs and against the propose of StructuralPartialEq. When they become fixed, this lint can become a useful lint with a single line change.

changelog: New lint: [equatable_if_let]

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 4, 2021
clippy_lints/src/pattern_equality.rs Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor

name idea: equatable_if_let

@HKalbasi
Copy link
Member Author

HKalbasi commented Oct 4, 2021

Originally extending it to matches! was in my mind (comment on this?) and single match is mentioned in the original issue, so maybe equatable_pattern_matching or equatable_pattern?

@camsteffen
Copy link
Contributor

camsteffen commented Oct 4, 2021

Linting matches! seems more picky to me so I wouldn't, at least not with this lint. The single match case is already linted to be changed to if let, so I consider if let to be the "base case" of this lint.

@HKalbasi
Copy link
Member Author

HKalbasi commented Oct 4, 2021

Make sense 👍

Is there any tool for renaming lint? They are everywhere in variety of cases.

@camsteffen
Copy link
Contributor

No but that's a good idea for clippy dev!

@HKalbasi HKalbasi changed the title Add lint pattern_equality Add lint equatable_if_let Oct 4, 2021
@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 4, 2021

📌 Commit f84ce1e has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented Oct 4, 2021

⌛ Testing commit f84ce1e with merge 6bacc1a...

bors added a commit that referenced this pull request Oct 4, 2021
Add lint `equatable_if_let`

This is my attempt for #1716. There is a major false positive, which is people may implement `PartialEq` in a different way. It is unactionable at the moment so I put it into `nursery`.

There is a trait `StructuralPartialEq` for solving this problem which is promising but it has several problems currently:
* Integers and tuples doesn't implement it.
* Some types wrongly implement it, like `Option<T>` when `T` doesn't implement it.

I consider them bugs and against the propose of `StructuralPartialEq`. When they become fixed, this lint can become a useful lint with a single line change.

changelog: New lint: [`equatable_if_let`]
@HKalbasi
Copy link
Member Author

HKalbasi commented Oct 4, 2021

I'm still renaming the lint :)

@camsteffen
Copy link
Contributor

The changelog diff is massive

@HKalbasi
Copy link
Member Author

HKalbasi commented Oct 4, 2021

Ah, my formatter seems don't like it. Now it is fixed.

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 4, 2021

📌 Commit 388a3d0 has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented Oct 4, 2021

⌛ Testing commit 388a3d0 with merge abe551e...

@bors
Copy link
Collaborator

bors commented Oct 4, 2021

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

@bors bors merged commit abe551e into rust-lang:master Oct 4, 2021
@camsteffen camsteffen mentioned this pull request Oct 9, 2021
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