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
[redundant_pattern_matching
]: include guard in suggestion
#11175
Conversation
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
This could probably add the guard (like |
// wow, the HIR for match guards in `PAT if let PAT = expr && expr => ...` is annoying! | ||
// `guard` here is `Guard::If` with the let expression somewhere deep in the tree of exprs, | ||
// counter to the intuition that it should be `Guard::IfLet`, so we need another check | ||
// to see that there aren't any let chains anywhere in the guard, as that would break | ||
// if we suggest `t.is_none() && (let X = y && z)` for: | ||
// `match t { None if let X = y && z => true, _ => false }` |
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.
I feel like this comment might be a bit confusing. If you ( the reviewer(s) ) already understand what this means, feel free to ignore.
Looking at the HIR here: https://godbolt.org/z/8413T1rjP (line 521-522), the guard is not Guard::IfLet
, but Guard::If
, with the let
expression being potentially somewhere deep in the expression tree, so just checking Guard::If
is not enough -- it may still be an if let guard despite the guard not being Guard::IfLet
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.
This is perhaps because it's desugared to something akin a let-chain, or at least something very similar, which is like And<Let, And<Expr, And<Expr, Let>>>
or something in the HIR, with Expr
being a regular &&
and Let
being && let
, so you can probably check the first in Binary::And
afaik but perhaps this would have issues on something like if true && let Some(x) = x
, I'm not sure. Better safe than sorry though so the current way is fine
Just if let Some(_) = x
on its own is IfLet
. I suppose it's because IfLet
only allows one Let
, while it needs a BinaryKind::And
for a let chain to be represented
redundant_pattern_matching
]: don't lint if match guards are presentredundant_pattern_matching
]: include guard in suggestion
@bors r+ comment seems fine to me. it's confusing but the situation is confusing |
[`redundant_pattern_matching`]: include guard in suggestion Fixes #11174 [`redundant_pattern_matching`]: include guard in suggestion
💔 Test failed - checks-action_test |
oops, I must have deleted the "changelog:" word from my PR description somehow. I edited it in now. |
@bors retry |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #11174
changelog: [
redundant_pattern_matching
]: include guard in suggestion