Make needless_bool less aggressive for chained ifs - #17598
Merged
Conversation
flip1995
added
the
I-sync-blocker
rustbot
added
the
S-waiting-on-community-reviews
Collaborator
|
Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews. In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews. |
rustbot
added
the
S-waiting-on-review
flip1995
commented
Aug 21, 2026
Comment on lines
+255
to
+262
| && let StmtKind::Semi(prev_if_expr) | StmtKind::Expr(prev_if_expr) = | ||
| block.stmts[block.stmts.len() - 2].kind | ||
| && let Some(higher::If { | ||
| cond: _, | ||
| then: prev_then, | ||
| r#else: None, | ||
| }) = higher::If::hir(prev_if_expr) | ||
| && let ExprKind::Ret(Some(_)) = peel_blocks_with_stmt(prev_then).kind |
Member
Author
There was a problem hiding this comment.
This part could in theory be shared with the if extraction in the above if-let chain. But that would mean putting it in a function with like 3 return values. Didn't seem worth it to me.
I intentionally do not check
- That the
prev_then_val == then_val(added a test for this) - That the
prev_then_func == then_func, I think this should be given due to the return type of the block already
|
Lintcheck changes for 6943d70
This comment will be updated if you push new changes |
Member
Author
|
All of the lintcheck findings are the exact patterns I tried to exclude from linting ✔️ |
Jarcho
requested changes
Aug 21, 2026
rustbot
added
S-waiting-on-author
Jarcho
approved these changes
Aug 21, 2026
In a function with multiple `if .. { return _; }`s with a trailing `!_` at the end, combining the last `if`+`!_` doesn't
make the code less complex necessarily. It just moves the "problem" one up. Piping this through the entire `if` chain in
the function either requires a non-trivial refactor or makes the code potentially less readable. Both are not acceptable
for a complexity lint.
This adds a check to the lint that makes sure that there is no other `if` statement before the last collapsible `if`
statement.
flip1995
force-pushed
the
fix-needless-aggressive-bool
branch
from
August 21, 2026 16:20
7b61af6 to
6943d70
Compare
flip1995
enabled auto-merge
August 21, 2026 16:22
rustbot
removed
the
S-waiting-on-community-reviews
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In a function with multiple
if .. { return _; }s with a trailing!_at the end, combining the lastif+!_doesn't make the code less complex necessarily. It just moves the "problem" one up. Piping this through the entireifchain in the function either requires a non-trivial refactor or makes the code potentially less readable. Both are not acceptable for a complexity lint.This adds a check to the lint that makes sure that there is no other
ifstatement before the last collapsibleifstatement.changelog: none
(Will be synced together with the commit that introduces it, so no changelog entry required)