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

significant_drop_in_scrutinee: Trigger lint also for scrutinees in while let and if let #12870

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

lrh2000
Copy link
Contributor

@lrh2000 lrh2000 commented May 30, 2024

This lint should also work for if let and while let, so this PR makes it actually work.

For while let, I can't think of any reason why this lint shouldn't be enabled. The only problem is that the lint suggests moving the significant drop above the while let, which is clearly invalid in the case of while let. I don't know if this is fixable, but this PR simply disables the wrong suggestions.

For if let, it seems that another lint called if_let_mutex has some overlapping functionality. But significant_drop_in_scrutinee is a bit stricter, as it will trigger even if the else branch does not try to lock the same mutex.

changelog: [significant_drop_in_scrutinee]: Trigger lint also for scrutinees in while let and if let.

r? @blyxyas (the third PR as promised in #12740 (comment), thanks for your review!)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 30, 2024
Comment on lines +1122 to 1138
} else {
if let Some(while_let) = higher::WhileLet::hir(expr) {
significant_drop_in_scrutinee::check_while_let(cx, expr, while_let.let_expr, while_let.if_then);
}
if !from_expansion {
redundant_pattern_match::check(cx, expr);
}
}
Copy link
Member

@J-ZhengLi J-ZhengLi Jun 4, 2024

Choose a reason for hiding this comment

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

nit (feel free to ignore as it might not be related), could this be written as:

} else if let Some(while_let) = higher::WhileLet::hir(expr) {
    significant_drop_in_scrutinee::check_while_let(cx, expr, while_let.let_expr, while_let.if_then);

    if !from_expansion {
        redundant_pattern_match::check(cx, while_let);
    }
}

and remove the other higher::WhileLet::hir(expr) in redundant_pattern_match::check, to improve readability for a little bit, plus it's a "redundant pattern matching" 😂. Unless that check function is called somewhere else.

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 for your comments. Well, your suggestion makes sense to me.

However, after having a closer look at redundant_pattern_match::check, I find out that this method needs expr to be passed to find_method_sugg_for_if_let (which in turn needs at least expr.span and expr.hir_id). It seems that such information is not available in WhileLet. I'm afraid it might not be worth refactoring such methods?

find_method_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false);

fn find_method_sugg_for_if_let<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
let_pat: &Pat<'_>,
let_expr: &'tcx Expr<'_>,
keyword: &'static str,
has_else: bool,
) {

@xFrednet
Copy link
Member

Hey @blyxyas & @lrh2000 , this is a ping from triage, since there hasn't been any activity in some time. What is the current status of this PR?

@lrh2000
Copy link
Contributor Author

lrh2000 commented Jul 1, 2024

@xFrednet I think I've replied to all the comments above. So perhaps the current "S-waiting-for-review" label still accurately reflects the status of this PR?

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.

Everything looks correct to me. I've done a lintcheck run and it doesn't seem to cause any false positives.

Thanks! 🌈 ❤️ And sorry for the delay, I was sorting out medical stuff.

@blyxyas
Copy link
Member

blyxyas commented Jul 2, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2024

📌 Commit 98f6c22 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 2, 2024

⌛ Testing commit 98f6c22 with merge 2051f83...

bors added a commit that referenced this pull request Jul 2, 2024
`significant_drop_in_scrutinee`: Trigger lint also for scrutinees in `while let` and `if let`

This lint should also work for `if let` and `while let`, so this PR makes it actually work.

For `while let`, I can't think of any reason why this lint shouldn't be enabled. The only problem is that the lint suggests moving the significant drop above the `while let`, which is clearly invalid in the case of `while let`. I don't know if this is fixable, but this PR simply disables the wrong suggestions.

For `if let`, it seems that another lint called `if_let_mutex` has some overlapping functionality. But `significant_drop_in_scrutinee` is a bit stricter, as it will trigger even if the `else` branch does not try to lock the same mutex.

changelog: [`significant_drop_in_scrutinee`]: Trigger lint also for scrutinees in `while let` and `if let`.

r? `@blyxyas` (the third PR as promised in #12740 (comment), thanks for your review!)
@bors
Copy link
Contributor

bors commented Jul 2, 2024

💔 Test failed - checks-action_test

@blyxyas
Copy link
Member

blyxyas commented Jul 2, 2024

Oh, clippy::too_many_lines. You can use #[allow(clippy::too_many_lines)] here :)

@xFrednet
Copy link
Member

xFrednet commented Jul 2, 2024

Or better #[expect(clippy::too_many_lines)] :P

@lrh2000
Copy link
Contributor Author

lrh2000 commented Jul 4, 2024

GitHub is confusing me. I got an email saying that some CI failed, but the GitHub UI says that all tests for commit 4567b2b passed?

Maybe because the clippy::too_many_lines only occurs after rebasing to master. I'll do the rebase in a subsequent force push.

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 Jul 8, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jul 8, 2024

📌 Commit 378962f has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 8, 2024

⌛ Testing commit 378962f with merge dfb9253...

@bors
Copy link
Contributor

bors commented Jul 8, 2024

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

@bors bors merged commit dfb9253 into rust-lang:master Jul 8, 2024
8 checks passed
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.

6 participants