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

fix incorrect suggestion for !(a as type >= b) #12626

Merged
merged 1 commit into from Apr 8, 2024
Merged

fix incorrect suggestion for !(a as type >= b) #12626

merged 1 commit into from Apr 8, 2024

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Apr 3, 2024 •
edited

fixes #12625

The expression !(a as type >= b) got simplified to a as type < b, but because of rust's parsing rules that < is interpreted as a start of generic arguments for type. This is fixed by recognizing this case and adding extra parens around the left-hand side of the comparison.

changelog: [nonminimal_bool]: fix incorrect suggestion for !(a as type >= b)

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 3, 2024
@folkertdev
Copy link
Contributor Author

Something I don't know how to check is whether this also works for cargo clippy --fix. Does that use the suggestion as given, or are further changes needed?

@J-ZhengLi
Copy link
Member

Something I don't know how to check is whether this also works for cargo clippy --fix. Does that use the suggestion as given, or are further changes needed?

Ok, I made some comments earlier which I then thought was wrong, so I panic and deleted it, so... sorry about that.

what I was saying is, the nonminimal_bool.rs test file is currently skipping rustfix with that @no-rustfix flag at the top, if you want to test the fixability, you can put your test cases in nonminimal_bool_methods.rs for now (there are some non-method tests anyway).

@folkertdev folkertdev force-pushed the incorrect-boolean-simplification branch from c3e85f3 to 38dbb0b Compare April 3, 2024 10:15
@folkertdev
Copy link
Contributor Author

thanks, fixed now!

@folkertdev folkertdev force-pushed the incorrect-boolean-simplification branch from 38dbb0b to 986ebb7 Compare April 3, 2024 16:20
blyxyas
blyxyas requested changes Apr 5, 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.

meow meow meow

clippy_lints/src/booleans.rs Outdated
Comment on lines 352 to 353
if let (ExprKind::Cast(..), BinOpKind::Ge) = (&lhs.kind, binop.node) {
if !(lhs_snippet.starts_with('(') && lhs_snippet.ends_with(')')) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you swap the order of these two so that the rare condition is processed earlier? (Performance meow meow)

Also, does this need to check for casts and >=? Are these the only variables where the suggestion is incorrect?

Copy link
Contributor Author

@folkertdev folkertdev Apr 7, 2024 •
edited

Choose a reason for hiding this comment

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

Could you swap the order of these two so that the rare condition is processed earlier? (Performance meow meow)

done

Also, does this need to check for casts and >=? Are these the only variables where the suggestion is incorrect?

no, <= is parsed as a single token, and is never seen as the start of a sequence of generic arguments. I believe < is the only operation where the naive transformation produces a result that does not parse.

@folkertdev folkertdev force-pushed the incorrect-boolean-simplification branch from 986ebb7 to 6a2cb33 Compare April 7, 2024 17:24
blyxyas
blyxyas approved these changes Apr 8, 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.

LGTM, thanks! ❤️

@blyxyas
Copy link
Member

blyxyas commented Apr 8, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 8, 2024

📌 Commit 6a2cb33 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 8, 2024

⌛ Testing commit 6a2cb33 with merge 2202493...

@bors
Copy link
Collaborator

bors commented Apr 8, 2024

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

@bors bors merged commit 2202493 into rust-lang:master Apr 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J-ZhengLi J-ZhengLi J-ZhengLi left review comments

@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.

Invalid clippy suggestion for !(a as type >= b)
5 participants
@folkertdev @rustbot @J-ZhengLi @blyxyas @bors