Skip to content

migrate prefer early return rule with tests #232

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.

Sign up for GitHub

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

Open
Islam-Shaaban-Ibrahim wants to merge 1 commit into
base: analysis_server_migration
Choose a base branch
from
+143 −396
Open

migrate prefer early return rule with tests #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 29 additions & 30 deletions lib/src/lints/prefer_early_return/prefer_early_return_rule.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import 'package:analyzer/error/listener.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';
import 'package:analyzer/analysis_rule/analysis_rule.dart';
import 'package:analyzer/analysis_rule/rule_context.dart';
import 'package:analyzer/analysis_rule/rule_visitor_registry.dart';
import 'package:analyzer/error/error.dart';
import 'package:solid_lints/src/lints/prefer_early_return/visitors/prefer_early_return_visitor.dart';
import 'package:solid_lints/src/models/rule_config.dart';
import 'package:solid_lints/src/models/solid_lint_rule.dart';

/// A rule which highlights `if` statements that span the entire body,
/// and suggests replacing them with a reversed boolean check
Expand Down Expand Up @@ -31,38 +31,37 @@ import 'package:solid_lints/src/models/solid_lint_rule.dart';
/// c;
/// }
/// ```
class PreferEarlyReturnRule extends SolidLintRule {
/// This lint rule represents the error if
/// 'if' statements should be reversed
class PreferEarlyReturnRule extends AnalysisRule {
/// Lint name
static const String lintName = 'prefer_early_return';

PreferEarlyReturnRule._(super.config);
/// Lint code
static const LintCode _code = LintCode(
lintName,
"Use reverse if to reduce nesting",
);

/// Creates a new instance of [PreferEarlyReturnRule]
/// based on the lint configuration.
factory PreferEarlyReturnRule.createRule(CustomLintConfigs configs) {
final rule = RuleConfig(
configs: configs,
name: lintName,
problemMessage: (_) => "Use reverse if to reduce nesting",
);
/// Creates an instance of [PreferEarlyReturnRule]
PreferEarlyReturnRule()
: super(
name: lintName,
description: 'Use reverse if to reduce nesting',
);

return PreferEarlyReturnRule._(rule);
}
@override
LintCode get diagnosticCode => _code;

@override
void run(
CustomLintResolver resolver,
DiagnosticReporter reporter,
CustomLintContext context,
void registerNodeProcessors(
RuleVisitorRegistry registry,
RuleContext context,
) {
context.registry.addBlockFunctionBody((node) {
final visitor = PreferEarlyReturnVisitor();
node.accept(visitor);

for (final element in visitor.nodes) {
reporter.atNode(element, code);
}
});
registry.addBlockFunctionBody(
this,
PreferEarlyReturnVisitor(
rule: this,
context: context,
),
Comment on lines +61 to +64
Copy link
Copy Markdown
Collaborator

@solid-vovabeloded solid-vovabeloded Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's extract it into a separate variable for better readability.

);
Comment on lines +59 to +65
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

}
}
90 changes: 23 additions & 67 deletions lib/src/lints/prefer_early_return/visitors/prefer_early_return_visitor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,81 +1,37 @@
import 'package:analyzer/analysis_rule/rule_context.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:solid_lints/src/lints/prefer_early_return/visitors/return_statement_visitor.dart';
import 'package:solid_lints/src/lints/prefer_early_return/visitors/throw_expression_visitor.dart';
import 'package:solid_lints/src/lints/prefer_early_return/prefer_early_return_rule.dart';

/// The AST visitor that will collect all unnecessary if statements
/// Visitor for [PreferEarlyReturnRule].
class PreferEarlyReturnVisitor extends RecursiveAstVisitor<void> {
final _nodes = <AstNode>[];
/// The rule associated with the visitor.
final PreferEarlyReturnRule rule;

/// All unnecessary if statements and conditional expressions.
Iterable<AstNode> get nodes => _nodes;
/// The context associated with the visitor.
final RuleContext context;

@override
void visitBlockFunctionBody(BlockFunctionBody node) {
super.visitBlockFunctionBody(node);

if (node.block.statements.isEmpty) return;

final (ifStatements, nextStatement) = _getStartIfStatements(node);
if (ifStatements.isEmpty) return;

// limit visitor to only work with functions
// that don't have a return statement or the return statement is empty
final nextStatementIsEmptyReturn =
nextStatement is ReturnStatement && nextStatement.expression == null;
final nextStatementIsNull = nextStatement == null;

if (!nextStatementIsEmptyReturn && !nextStatementIsNull) return;

_handleIfStatement(ifStatements.last);
}

void _handleIfStatement(IfStatement node) {
if (_isElseIfStatement(node)) return;
if (_hasElseStatement(node)) return;
if (_hasReturnStatement(node)) return;
if (_hasThrowExpression(node)) return;
/// Constructor for [PreferEarlyReturnVisitor].
PreferEarlyReturnVisitor({
required this.rule,
required this.context,
});

_nodes.add(node);
}

// returns a list of if statements at the start of the function
// and the next statement after it
// examples:
// [if, if, if, return] -> ([if, if, if], return)
// [if, if, if, _doSomething, return] -> ([if, if, if], _doSomething)
// [if, if, if] -> ([if, if, if], null)
(List<IfStatement>, Statement?) _getStartIfStatements(
BlockFunctionBody body,
) {
final List<IfStatement> ifStatements = [];
for (final statement in body.block.statements) {
if (statement is IfStatement) {
ifStatements.add(statement);
} else {
return (ifStatements, statement);
}
@override
void visitIfStatement(IfStatement node) {
if (_shouldReport(node)) {
context.currentUnit?.diagnosticReporter.atNode(
Copy link
Copy Markdown
Collaborator

@solid-vovabeloded solid-vovabeloded Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between the current implementation andrule.reportAtNode?

node,
rule.diagnosticCode,
);
}
return (ifStatements, null);
}

bool _hasElseStatement(IfStatement node) {
return node.elseStatement != null;
super.visitIfStatement(node);
}

bool _isElseIfStatement(IfStatement node) {
return node.elseStatement != null && node.elseStatement is IfStatement;
}

bool _hasReturnStatement(Statement node) {
final visitor = ReturnStatementVisitor();
node.accept(visitor);
return visitor.nodes.isNotEmpty;
}
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;
}
Comment on lines +32 to 36
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
  }

Copy link
Copy Markdown
Collaborator

@solid-vovabeloded solid-vovabeloded Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Islam-Shaaban-Ibrahim, pay attention to this comment.

}
Loading
Loading