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

Fix manual_range_contains false negative with chains of && and || #8884

Merged
merged 2 commits into from
May 31, 2022

Conversation

evantypanski
Copy link
Contributor

Fixes #8745

Since the precedence for && is the same as itself the HIR for a chain of && ends up with a right skewed tree like:

     &&
    /  \
  &&   c2
 /  \
... c1

So only the leftmost && was actually "fully" checked, the top level was just c2 and && so the manual_range_contains lint won't apply. This change makes it also check c2 with c1.

There's a bit of a hacky solution in the second commit to check if the number of open/closing parens in the snippet match. This is to prevent a case like ((x % 2 == 0) || (x < 0)) || (x >= 10) from offering a suggestion like ((x % 2 == 0) || !(0..10).contains(&x) which now won't compile.

Any suggestions for that paren hack welcome, kinda new to working on this so not too sure about possible solutions :) it's weird because I don't know how else to check for parens in HIR considering they're removed when lowering AST.

changelog: Fix [manual_range_contains] false negative with chains of && and ||

@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 May 24, 2022
@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 31, 2022

📌 Commit 257f097 has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented May 31, 2022

⌛ Testing commit 257f097 with merge 5b1a4c0...

@bors
Copy link
Collaborator

bors commented May 31, 2022

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

@bors bors merged commit 5b1a4c0 into rust-lang:master May 31, 2022
@evantypanski evantypanski deleted the manual_range_contains_multiple branch May 31, 2022 17:37
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.

manual_range_contains doesn't catch multiple range checks in && expressions
4 participants