-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Make collapsible_if
recognize the let_chains
feature
#14481
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
Conversation
dee48da
to
cae2375
Compare
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.
Was the 3rd commit auto-applied with dogfood fix
?
clippy_lints/src/collapsible_if.rs
Outdated
/// If `block` is a block with either one expression or a statement containing an expression, | ||
/// return the expression. | ||
fn expr_block<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<'tcx>> { | ||
match block.stmts { | ||
[] => block.expr, | ||
[stmt] => { | ||
if let StmtKind::Semi(expr) = stmt.kind { | ||
Some(expr) | ||
} else { | ||
None | ||
} | ||
}, | ||
_ => None, | ||
} | ||
} |
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.
Could this better be implemented with clippy_utils::peel_blocks
and then just "unpack" the statement or return the expression?
Or are you intentionally only removing 1 block?
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 am intentionally removing only one block, the top-level one in the then
parts, as multiple blocks would probably indicate a design choice, and ought to be flagged by another lint if this is not needed.
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.
Makes sense. In that case, please extend the comment to note that this is intentionally not using peel_blocks
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.
Done.
clippy_lints/src/collapsible_if.rs
Outdated
Self { | ||
let_chains_enabled: tcx.features().enabled(sym::let_chains), |
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.
let_chains_enabled: tcx.features().enabled(sym::let_chains), | |
let_chains_enabled: tcx.features().let_chains(), |
I think this is the better approach. With that, the person stabilizing this feature in rustc
is also forced to remove this check.
Yes, totally automated, with the addition of this additional whitespace reformatting on top at a few places where |
1bf1a03
to
dee46f2
Compare
Until `if let` chains are stabilized, we do not collapse them together or with other `if` expressions unless the `let_chains` feature is enabled. This is the case for example in Clippy sources.
Since Clippy uses the `let_chains` feature, there are many occasions to collapse `if` and `if let` statements.
dee46f2
to
79c6911
Compare
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!
Until
if let
chains are stabilized, we do not collapse them together or with otherif
expressions unless thelet_chains
feature is enabled. This is the case for example in Clippy sources.This was made possible by converting the
collapsible_if
to a late lint to get access to the set of enabled features. This allows this PR to supersede #14455 and no longer require an additional configuration option.The three commits are, in order:
let_chains
feature detection and action, and testschangelog: [
collapsible_if
]: recognize the rust compilerlet_chains
featurer? @flip1995