migrate prefer early return rule with tests#232
migrate prefer early return rule with tests#232Islam-Shaaban-Ibrahim wants to merge 1 commit intoanalysis_server_migrationfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the prefer_early_return lint rule to align with the AnalysisRule API, simplifying the rule definition and its associated visitor. Review feedback identifies a regression where if statements with else blocks are no longer excluded from reporting, potentially leading to undesirable lint triggers. There is also a concern regarding the use of RecursiveAstVisitor with addBlockFunctionBody, which may result in duplicate diagnostics for nested functions.
| bool _shouldReport(IfStatement node) { | ||
| final parent = node.parent; | ||
|
|
||
| bool _hasThrowExpression(Statement node) { | ||
| final visitor = ThrowExpressionVisitor(); | ||
| node.accept(visitor); | ||
| return visitor.nodes.isNotEmpty; | ||
| return parent is Block && parent.statements.length == 1; | ||
| } |
There was a problem hiding this comment.
The _shouldReport logic is missing a check for else statements. The prefer_early_return rule should not trigger on if statements that have an else block, as reversing the condition in those cases doesn't typically lead to an early return and can make the code less readable. The previous implementation correctly excluded these cases.
bool _shouldReport(IfStatement node) {
final parent = node.parent;
return parent is Block &&
parent.statements.length == 1 &&
node.elseStatement == null;
}
There was a problem hiding this comment.
@Islam-Shaaban-Ibrahim, pay attention to this comment.
| registry.addBlockFunctionBody( | ||
| this, | ||
| PreferEarlyReturnVisitor( | ||
| rule: this, | ||
| context: context, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Using RecursiveAstVisitor in combination with registry.addBlockFunctionBody will lead to duplicate diagnostics if the code contains nested functions or closures. The visitor will traverse into the nested function's body, and the registry will also trigger the visitor for that same nested body independently.
Consider registering for IfStatement directly using registry.addIfStatement and performing the block check there, or use a non-recursive visitor.
| bool _shouldReport(IfStatement node) { | ||
| final parent = node.parent; | ||
|
|
||
| bool _hasThrowExpression(Statement node) { | ||
| final visitor = ThrowExpressionVisitor(); | ||
| node.accept(visitor); | ||
| return visitor.nodes.isNotEmpty; | ||
| return parent is Block && parent.statements.length == 1; | ||
| } |
There was a problem hiding this comment.
@Islam-Shaaban-Ibrahim, pay attention to this comment.
| @override | ||
| void visitIfStatement(IfStatement node) { | ||
| if (_shouldReport(node)) { | ||
| context.currentUnit?.diagnosticReporter.atNode( |
There was a problem hiding this comment.
What is the difference between the current implementation andrule.reportAtNode?
| PreferEarlyReturnVisitor( | ||
| rule: this, | ||
| context: context, | ||
| ), |
There was a problem hiding this comment.
Let's extract it into a separate variable for better readability.
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please make sure you have migrated all the currently existing tests.
2 participants
No description provided.