-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
} 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); | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
rust-clippy/clippy_lints/src/matches/redundant_pattern_match.rs
Lines 173 to 180 in 48686ad
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 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? |
There was a problem hiding this 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.
@bors r+ |
`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!)
💔 Test failed - checks-action_test |
Oh, |
Or better |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! ❤️
@bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This lint should also work for
if let
andwhile 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 thewhile let
, which is clearly invalid in the case ofwhile 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 calledif_let_mutex
has some overlapping functionality. Butsignificant_drop_in_scrutinee
is a bit stricter, as it will trigger even if theelse
branch does not try to lock the same mutex.changelog: [
significant_drop_in_scrutinee
]: Trigger lint also for scrutinees inwhile let
andif let
.r? @blyxyas (the third PR as promised in #12740 (comment), thanks for your review!)