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

Extend NONMINIMAL_BOOL lint #12248

Merged
merged 2 commits into from Feb 11, 2024
Merged

Extend NONMINIMAL_BOOL lint #12248

merged 2 commits into from Feb 11, 2024

Conversation

GuillaumeGomez
Copy link
Member

Fixes #5794.

r? @blyxyas

changelog: Extend NONMINIMAL_BOOL lint

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 8, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 8, 2024
Closed
blyxyas
blyxyas approved these changes Feb 9, 2024
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.

This could be merged, but to fix #5794 this would need a test for !a != 12

@GuillaumeGomez
Copy link
Member Author

Noted. Gonna add support for this case.

@GuillaumeGomez
Copy link
Member Author

Euh wait, no. It's not the same, unless a is a bool type. I'll add a check only for this case.

@GuillaumeGomez GuillaumeGomez force-pushed the extend-NONMINIMAL_BOOL branch 3 times, most recently from f38ead7 to f122149 Compare February 9, 2024 20:03
@Centri3
Copy link
Member

Centri3 commented Feb 10, 2024

Shouldn't this be true for integers as well? e.g., !a != 12 where a is 12 is true, as Not flips each bit of the integer

@Centri3
Copy link
Member

Centri3 commented Feb 10, 2024

Oh wow nvm, I thought this was for bool only. It already lints integers too

@GuillaumeGomez
Copy link
Member Author

Anything else to be done here then? :)

blyxyas
blyxyas approved these changes Feb 11, 2024
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.

Just a mini-nit, could you squash the commits?

clippy_lints/src/booleans.rs Outdated
Comment on lines 91 to 98
ExprKind::Unary(UnOp::Not, sub) => check_inverted_condition(cx, expr.span, sub),
ExprKind::Binary(op, left, right) if matches!(op.node, BinOpKind::Eq | BinOpKind::Ne) => {
Copy link
Member

Choose a reason for hiding this comment

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

this should have a comment talking about what case they cover, just for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

@GuillaumeGomez
Copy link
Member Author

Just a mini-nit, could you squash the commits?

I squashed the commits in two commits as I still want to keep the "inverted boolean in comparison" in its own commit as it feels like a different kind of check.

@blyxyas
Copy link
Member

blyxyas commented Feb 11, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 11, 2024

📌 Commit 5e7c437 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 11, 2024

⌛ Testing commit 5e7c437 with merge 75f57cf...

@bors
Copy link
Collaborator

bors commented Feb 11, 2024

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

@bors bors merged commit 75f57cf into rust-lang:master Feb 11, 2024
5 checks passed
@GuillaumeGomez GuillaumeGomez deleted the extend-NONMINIMAL_BOOL branch February 12, 2024 09:45
@RalfJung
Copy link
Member

I think this probably introduced #12371

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.

Lint double negation !count != 0
6 participants
@GuillaumeGomez @Centri3 @blyxyas @bors @RalfJung @rustbot