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

Disable "if_not_else" lints from firing on else-ifs #7895

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

ahmedkrmn
Copy link
Contributor

@ahmedkrmn ahmedkrmn commented Oct 29, 2021

Fixes #7892

  1. Convert ['if_not_else'] to LateLintPass and use clippy_utils::is_else_clause for checking.
  2. Update tests.

changelog: [if_not_else] now ignores else if statements.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 29, 2021
@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

📌 Commit 282d313 has been approved by Manishearth

bors added a commit that referenced this pull request Oct 29, 2021
Disable "if_not_else" lints from firing on else-ifs

Fixes #7892

1. Convert `['if_not_else']` to `LateLintPass` and use `clippy_utils::is_else_clause` for checking.
2. Update tests.
@bors
Copy link
Collaborator

bors commented Oct 29, 2021

⌛ Testing commit 282d313 with merge 47d1bec...

@Manishearth
Copy link
Member

@bors r-

CI is failing

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

💔 Test failed - checks-action_test

@xFrednet
Copy link
Member

xFrednet commented Oct 29, 2021

The error can be seen here It looks like a false positive, it might have something to do with how while loops are desugared 🤔

Could you also include a changelog entry in your PR, the CI will otherwise fail during the merge. You can just add something like this: "changelog: [if_not_else] now ignores else if statements" 🙃

@dswij
Copy link
Member

dswij commented Oct 29, 2021

The error can be seen here It looks like a false positive, it might have something to do with how while loops are desugared 🤔

This might makes sense since it's now using a late pass rather than an early pass

@xFrednet
Copy link
Member

Yes, but it seems weird that is_else_clause would cause this FP, as it's also used more often.

@ahmedkrmn
Copy link
Contributor Author

Thanks @Manishearth, @xFrednet and @dswij!

Yes, but it seems weird that is_else_clause would cause this FP, as it's also used more often.

@xFrednet, I've removed the is_else_clause call and reverted back to the old behavior but with using LateLintPass instead of EarlyLintPass and the test is still failing on the same while loop. I will continue debugging and see if I can find the culprit.

@xFrednet
Copy link
Member

That's good to know! Reach out, if you need more help debugging 🙃

if in_external_macro(cx.sess, item.span) {
impl LateLintPass<'_> for IfNotElse {
fn check_expr(&mut self, cx: &LateContext<'_>, item: &Expr<'_>) {
if in_external_macro(cx.sess(), item.span) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fix it if my suspicion is right - changing to LateLintPass means that desugaring occurs before the lint runs. from_expansion will catch desugaring in addition to macros.

Suggested change
if in_external_macro(cx.sess(), item.span) {
if item.span.from_expansion() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @camsteffen. This is my first time working with rustc. I didn't know this life saver function existed!
I've been debugging the issue. After dumping the expression trees, I could see that the while loop was indeed represented as an ExprKind::If when using LateLintPasswhich caused this problem. I thought about reverting back to EarlyLintPass and looking for a workaround, but your comment came just in time!

1. Convert IfNotElse to LateLintPass and use clippy_utils::is_else_clause for checking.
2. Handle the case where the span comes from desugaring.
3. Update tests.
@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

📌 Commit 2f327aa has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

⌛ Testing commit 2f327aa with merge 4c70c18...

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

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

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.

if_not_else lints firing on else-ifs
7 participants