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

Make comparison_to_empty work on if let/let chains #11029

Merged
merged 2 commits into from
Jul 23, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 26, 2023

This adds LetChain to clippy_utils::higher, other lints may benefit from such a change as well :D

changelog: Enhancement: [comparison_to_empty]: Now lints on if let

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 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 Jun 26, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Jun 27, 2023

This would work just as well by checking for ExprKind::Let rather then the whole if condition. ExprKind::Let can only exist as part or it.

@Centri3
Copy link
Member Author

Centri3 commented Jun 27, 2023

Done, I've kept LetChain though as I still think it can be useful in other cases

@Jarcho
Copy link
Contributor

Jarcho commented Jun 28, 2023

I'm not sure how useful it will end up. An iterator over binary operator chains might be useful, but I'd rather wait for a definite use case before adding it.

Generally util functions shouldn't be added without at least a single use case.

@Centri3
Copy link
Member Author

Centri3 commented Jun 28, 2023

I'm not sure how useful it will end up. An iterator over binary operator chains might be useful, but I'd rather wait for a definite use case before adding it.

Generally util functions shouldn't be added without at least a single use case.

#9665. It requires something like this (if it wants to support let chains), and my prototype implementation uses it.

@Jarcho
Copy link
Contributor

Jarcho commented Jun 28, 2023

If that's the case then it should be added with that PR so it can be reviewed with it's use case.

@bors
Copy link
Contributor

bors commented Jul 13, 2023

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

@Jarcho
Copy link
Contributor

Jarcho commented Jul 22, 2023

ping @Centri3

This is only waiting on a rebase and the removal of the let-chain util.

@Jarcho
Copy link
Contributor

Jarcho commented Jul 23, 2023

Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Jul 23, 2023

📌 Commit ae5d391 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 23, 2023

⌛ Testing commit ae5d391 with merge 356768b...

@bors
Copy link
Contributor

bors commented Jul 23, 2023

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

@bors bors merged commit 356768b into rust-lang:master Jul 23, 2023
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.

4 participants