diff --git a/lib/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart b/lib/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart index 90cd9c7d..7293c416 100644 --- a/lib/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart +++ b/lib/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart @@ -1,11 +1,8 @@ -import 'package:analyzer/dart/ast/ast.dart'; -import 'package:analyzer/dart/element/element.dart'; -import 'package:analyzer/error/listener.dart'; -import 'package:custom_lint_builder/custom_lint_builder.dart'; -import 'package:solid_lints/src/common/visitors/select_expression_identifiers_visitor.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/avoid_unnecessary_return_variable/visitors/avoid_unnecessary_return_variable_visitor.dart'; -import 'package:solid_lints/src/models/rule_config.dart'; -import 'package:solid_lints/src/models/solid_lint_rule.dart'; /// An `avoid_unnecessary_return_variable` rule which forbids returning /// an immutable variable if it can be rewritten in return statement itself. @@ -32,109 +29,37 @@ import 'package:solid_lints/src/models/solid_lint_rule.dart'; /// } /// ``` /// -class AvoidUnnecessaryReturnVariableRule extends SolidLintRule { - /// This lint rule represents the error - /// when unnecessary return variable statement is found +class AvoidUnnecessaryReturnVariableRule extends AnalysisRule { + /// The name of the lint rule. static const lintName = 'avoid_unnecessary_return_variable'; - AvoidUnnecessaryReturnVariableRule._(super.config); - - /// Creates a new instance of [AvoidUnnecessaryReturnVariableRule] - /// based on the lint configuration. - factory AvoidUnnecessaryReturnVariableRule.createRule( - CustomLintConfigs configs, - ) { - final rule = RuleConfig( - configs: configs, - name: lintName, - problemMessage: (_) => """ + /// The message shown when the lint is triggered. + static const String lintMessage = """ Avoid creating unnecessary variable only for return. -Rewrite the variable evaluation into return statement instead.""", - ); +Rewrite the variable evaluation into return statement instead."""; - return AvoidUnnecessaryReturnVariableRule._(rule); - } + /// Lint code. + static const LintCode _code = LintCode( + lintName, + lintMessage, + ); - @override - void run( - CustomLintResolver resolver, - DiagnosticReporter reporter, - CustomLintContext context, - ) { - context.registry.addReturnStatement( - (statement) { - _checkReturnStatement( - statement: statement, - reporter: reporter, - context: context, + /// Creates a new instance of [AvoidUnnecessaryReturnVariableRule] + AvoidUnnecessaryReturnVariableRule() + : super( + name: lintName, + description: lintMessage, ); - }, - ); - } - - void _checkReturnStatement({ - required ReturnStatement statement, - required DiagnosticReporter reporter, - required CustomLintContext context, - }) { - final expr = statement.expression; - if (expr is! SimpleIdentifier) return; - - final element = expr.element; - if (element is! LocalVariableElement) return; - final returnVariableElement = element; - - if (!returnVariableElement.isFinal && !returnVariableElement.isConst) { - return; - } - - final blockBody = statement.parent; - if (blockBody == null) return; - - final visitor = AvoidUnnecessaryReturnVariableVisitor( - returnVariableElement, - statement, - ); - blockBody.visitChildren(visitor); - - if (!visitor.hasBadStatementCount()) return; - //it is 100% bad if return statement follows declaration - if (!visitor.foundTokensBetweenDeclarationAndReturn) { - reporter.atNode(statement, code); - return; - } - - final declaration = visitor.variableDeclaration; - - //check if immutable - final initializer = declaration?.initializer; - if (initializer == null) return; - - if (!_isExpressionImmutable(initializer)) return; - - reporter.atNode(statement, code); - } - - bool _isExpressionImmutable(Expression expr) { - final visitor = SelectExpressionIdentifiersVisitor(); - expr.accept(visitor); - - return visitor.identifiers.every(_isSimpleIdentifierImmutable); - } - - bool _isSimpleIdentifierImmutable(SimpleIdentifier identifier) { - switch (identifier.element) { - case final VariableElement variable: - return variable.isFinal || variable.isConst; - - case ClassElement _: - return true; - - case GetterElement(:final PropertyInducingElement variable): - return variable.isFinal || variable.isConst; - } + @override + LintCode get diagnosticCode => _code; - return false; + @override + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, + ) { + final visitor = AvoidUnnecessaryReturnVariableVisitor(this); + registry.addReturnStatement(this, visitor); } } diff --git a/lib/src/lints/avoid_unnecessary_return_variable/visitors/avoid_unnecessary_return_variable_visitor.dart b/lib/src/lints/avoid_unnecessary_return_variable/visitors/avoid_unnecessary_return_variable_visitor.dart index bbc7455d..cdcea4dd 100644 --- a/lib/src/lints/avoid_unnecessary_return_variable/visitors/avoid_unnecessary_return_variable_visitor.dart +++ b/lib/src/lints/avoid_unnecessary_return_variable/visitors/avoid_unnecessary_return_variable_visitor.dart @@ -1,79 +1,79 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; -import 'package:collection/collection.dart'; +import 'package:solid_lints/src/common/visitors/select_expression_identifiers_visitor.dart'; +import 'package:solid_lints/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart'; +import 'package:solid_lints/src/lints/avoid_unnecessary_return_variable/visitors/return_variable_usage_visitor.dart'; -/// This visitor is searches all uses of a single local variable, -/// including variable declaration. -class AvoidUnnecessaryReturnVariableVisitor extends RecursiveAstVisitor { - /// The problem expects that exactly 1 mention of return variable. - /// VariableDeclarationStatement doesn't count when visiting SimpleIdentifier. - /// Any other amount of variable mentions implies that it is used somewhere - /// except return, so its existence is justified. - static const _badStatementCount = 1; +/// Visitor for [AvoidUnnecessaryReturnVariableRule]. +class AvoidUnnecessaryReturnVariableVisitor extends SimpleAstVisitor { + /// The rule associated with this visitor. + final AvoidUnnecessaryReturnVariableRule rule; - final LocalVariableElement _returnVariableElement; + /// Creates a new instance of [AvoidUnnecessaryReturnVariableVisitor]. + AvoidUnnecessaryReturnVariableVisitor(this.rule); - bool _foundTokensBetweenDeclarationAndReturn = false; - VariableDeclaration? _variableDeclaration; - int _variableStatementCounter = 0; + @override + void visitReturnStatement(ReturnStatement node) { + final expr = node.expression; + if (expr is! SimpleIdentifier) { + return; + } - final ReturnStatement _returnStatement; + final element = expr.element; + if (element is! LocalVariableElement) { + return; + } - /// After visiting holds info about whether there are any tokens - /// between variable declaration and return statement - bool get foundTokensBetweenDeclarationAndReturn => - _foundTokensBetweenDeclarationAndReturn; + if (!element.isFinal && !element.isConst) { + return; + } - /// Returns statement of local variable declaration - VariableDeclaration? get variableDeclaration => _variableDeclaration; + final block = node.parent; + if (block == null) return; - /// Creates a new instance of [AvoidUnnecessaryReturnVariableVisitor]. - AvoidUnnecessaryReturnVariableVisitor( - this._returnVariableElement, - this._returnStatement, - ); + final returnVariableUsageVisitor = + ReturnVariableUsageVisitor(node, element); - /// Defines whether the variables is used in return statement only. - bool hasBadStatementCount() => - _variableStatementCounter == _badStatementCount; + block.visitChildren(returnVariableUsageVisitor); + if (!returnVariableUsageVisitor.hasBadStatementCount()) return; - @override - void visitVariableDeclarationStatement(VariableDeclarationStatement node) { - if (_collectVariableDeclaration(node)) { - _checkTokensInBetween(node, _returnStatement); + //it is 100% bad if return statement follows declaration + if (!returnVariableUsageVisitor.foundTokensBetweenDeclarationAndReturn) { + rule.reportAtNode(node); + return; } - super.visitVariableDeclarationStatement(node); - } - @override - void visitSimpleIdentifier(SimpleIdentifier node) { - if (node.element?.id == _returnVariableElement.id) { - _variableStatementCounter++; - } + final declaration = returnVariableUsageVisitor.variableDeclaration; + + //check if immutable + final initializer = declaration?.initializer; + if (initializer == null) return; - super.visitSimpleIdentifier(node); + if (!_isExpressionImmutable(initializer)) return; + + rule.reportAtNode(node); } - bool _collectVariableDeclaration(VariableDeclarationStatement node) { - final targetVariable = node.variables.variables.firstWhereOrNull( - (v) => v.declaredFragment?.element.id == _returnVariableElement.id, - ); - if (targetVariable == null) return false; + bool _isExpressionImmutable(Expression expr) { + final visitor = SelectExpressionIdentifiersVisitor(); + expr.accept(visitor); - _variableDeclaration = targetVariable; - return true; + return visitor.identifiers.every(_isSimpleIdentifierImmutable); } - void _checkTokensInBetween( - VariableDeclarationStatement variableDeclaration, - ReturnStatement returnStatement, - ) { - final tokenBeforeReturn = - _returnStatement.findPrevious(_returnStatement.beginToken); + bool _isSimpleIdentifierImmutable(SimpleIdentifier identifier) { + switch (identifier.element) { + case final VariableElement variable: + return variable.isFinal || variable.isConst; + + case ClassElement _: + return true; - if (tokenBeforeReturn != variableDeclaration.endToken) { - _foundTokensBetweenDeclarationAndReturn = true; + case GetterElement(:final PropertyInducingElement variable): + return variable.isFinal || variable.isConst; } + + return false; } } diff --git a/lib/src/lints/avoid_unnecessary_return_variable/visitors/return_variable_usage_visitor.dart b/lib/src/lints/avoid_unnecessary_return_variable/visitors/return_variable_usage_visitor.dart new file mode 100644 index 00000000..ff46b0f0 --- /dev/null +++ b/lib/src/lints/avoid_unnecessary_return_variable/visitors/return_variable_usage_visitor.dart @@ -0,0 +1,76 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:collection/collection.dart'; +import 'package:solid_lints/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart'; + +/// Visitor for [AvoidUnnecessaryReturnVariableRule] which checks whether the +/// return variable is used somewhere except return statement and +/// whether it is immutable. +class ReturnVariableUsageVisitor extends RecursiveAstVisitor { + final ReturnStatement _returnStatement; + final LocalVariableElement _returnVariableElement; + + /// After visiting holds the declaration of return variable + VariableDeclaration? variableDeclaration; + + /// After visiting holds info about whether there are any tokens + bool foundTokensBetweenDeclarationAndReturn = false; + + /// The problem expects that exactly 1 mention of return variable. + /// VariableDeclarationStatement doesn't count when visiting SimpleIdentifier. + /// Any other amount of variable mentions implies that it is used somewhere + /// except return, so its existence is justified. + static const _badStatementCount = 1; + + int _variableStatementCounter = 0; + + /// Defines whether the variables is used in return statement only. + bool hasBadStatementCount() => + _variableStatementCounter == _badStatementCount; + + /// Creates a new instance of [ReturnVariableUsageVisitor]. + ReturnVariableUsageVisitor( + this._returnStatement, + this._returnVariableElement, + ); + + @override + void visitVariableDeclarationStatement(VariableDeclarationStatement node) { + if (_collectVariableDeclaration(node)) { + _checkTokensInBetween(node, _returnStatement); + } + super.visitVariableDeclarationStatement(node); + } + + @override + void visitSimpleIdentifier(SimpleIdentifier node) { + if (node.element?.id == _returnVariableElement.id) { + _variableStatementCounter++; + } + + super.visitSimpleIdentifier(node); + } + + bool _collectVariableDeclaration(VariableDeclarationStatement node) { + final targetVariable = node.variables.variables.firstWhereOrNull( + (v) => v.declaredFragment?.element.id == _returnVariableElement.id, + ); + if (targetVariable == null) return false; + variableDeclaration = targetVariable; + + return true; + } + + void _checkTokensInBetween( + VariableDeclarationStatement variableDeclaration, + ReturnStatement returnStatement, + ) { + final tokenBeforeReturn = + _returnStatement.findPrevious(_returnStatement.beginToken); + + if (tokenBeforeReturn != variableDeclaration.endToken) { + foundTokensBetweenDeclarationAndReturn = true; + } + } +} diff --git a/lint_test/avoid_unnecessary_return_variable_test.dart b/lint_test/avoid_unnecessary_return_variable_test.dart deleted file mode 100644 index 1e068180..00000000 --- a/lint_test/avoid_unnecessary_return_variable_test.dart +++ /dev/null @@ -1,125 +0,0 @@ -// ignore_for_file: unused_local_variable, newline_before_return, no_empty_block - -/// Test the avoid_unnecessary_return_variable. -/// Good code, trivial case. -int returnVarTestGoodTrivial() { - return 1; -} - -/// Test the avoid_unnecessary_return_variable. -/// Returning mutable variable should not trigger the lint. -int returnVarTestReturnMutable() { - var a = 1; - a++; - - return a; -} - -int returnVarTestReturnParameter(int param) { - return param; -} - -/// Test the avoid_unnecessary_return_variable. -/// Caching mutable variable value. -/// Unpredictable: may be useful to cache value -/// before operation can change it. -int returnVarTestCachedMutable() { - var a = 1; - final result = a; - _doNothing(); - - return result; -} - -/// Test the avoid_unnecessary_return_variable. -/// Caching mutable variable value, but return goes -/// right after declaration, which makes it bad. -int returnVarTestReturnFollowsDeclaration() { - var a = 1; - final result = a; - - //Some comment here - - //expect_lint: avoid_unnecessary_return_variable - return result; -} - -/// Test the avoid_unnecessary_return_variable. -/// Caching another method result. -/// Unpredictable: may be useful to cache value -/// before operation can change it. -int returnVarTestCachedAnotherMethodResult() { - var a = 1; - final result = _testValueEval(); - _doNothing(); - - return result; -} - -/// Test the avoid_unnecessary_return_variable. -/// Caching value of object's field. -/// Unpredictable: may be useful to cache value -/// before operation can change it. -int returnVarTestCachedObjectField() { - final obj = _TestClass(); - final result = obj.varField; - _doNothing(); - - return result; -} - -/// Test the avoid_unnecessary_return_variable. -/// Good: variable is created not only for return -/// but is used in following expressions as well. -int returnVarTestUsedVariable() { - var a = 1; - final result = 2; - a += result; - - return result; -} - -/// Test the avoid_unnecessary_return_variable. -/// Bad code, trivial example. -int returnVarTestBadTrivial() { - final result = 1; - - //expect_lint: avoid_unnecessary_return_variable - return result; -} - -/// Test the avoid_unnecessary_return_variable. -/// Bad code: result expression is immutable, -/// so can be written in return statement directly. -int returnVarTestBadImmutableExpression() { - const constLocal = 1; - final finalLocal = 1; - final testObj = _TestClass(); - final result = constLocal + - finalLocal + - 1 + //const literal - _TestClass.constValue + - _TestClass.finalValue + - testObj.finalField; - _doNothing(); - - //expect_lint: avoid_unnecessary_return_variable - return result; -} - -int _testValueEval() { - return 1; -} - -/// This method is a placeholder for unpredictable behaviour -/// which can potentially change any mutable variables -void _doNothing() {} - -//ignore: prefer_match_file_name -class _TestClass { - static const constValue = 1; - static final finalValue = 1; - //ignore: member_ordering - final finalField = 1; - var varField = 1; -} diff --git a/test/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule_test.dart b/test/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule_test.dart new file mode 100644 index 00000000..9bf78468 --- /dev/null +++ b/test/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule_test.dart @@ -0,0 +1,180 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:solid_lints/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidUnnecessaryReturnVariableTest); + }); +} + +@reflectiveTest +class AvoidUnnecessaryReturnVariableTest extends AnalysisRuleTest { + @override + void setUp() { + rule = AvoidUnnecessaryReturnVariableRule(); + super.setUp(); + } + + @override + String get analysisRule => AvoidUnnecessaryReturnVariableRule.lintName; + + void test_does_not_report_if_return_good_trivial() async { + await assertNoDiagnostics( + r''' +int returnVarTestTrivial() { + return 1; +} + ''', + ); + } + + void test_does_not_report_if_return_is_mutable() async { + await assertNoDiagnostics( + r''' +int returnVarTestMutable() { + var a = 1; + a++; + + return a; +} + ''', + ); + } + + void test_does_not_report_if_returns_parameter() async { + await assertNoDiagnostics( + r''' +int returnVarTestReturnParameter(int param) { + return param; +} + ''', + ); + } + + void test_does_not_report_if_return_is_cached_mutable() async { + await assertNoDiagnostics( + r''' +int returnVarTestCachedMutable() { + var a = 1; + final result = a; + _doNothing(); + + return result; +} + +void _doNothing() {} + ''', + ); + } + + void test_reports_if_return_follows_declaration() async { + await assertDiagnostics(r''' +int returnVarTestReturnFollowsDeclaration() { + var a = 1; + final result = a; + + //Some comment here + + return result; +} + ''', [lint(105, 14)]); + } + + void test_does_not_report_if_return_is_cached_another_method_result() async { + await assertNoDiagnostics( + r''' +int returnVarTestCachedAnotherMethodResult() { + var a = 1; + final result = _testValueEval(); + _doNothing(); + + return result; +} + +int _testValueEval() { + return 1; +} + +void _doNothing() {} +''', + ); + } + + void test_does_not_report_if_return_is_cached_object_field() async { + await assertNoDiagnostics( + r''' +int returnVarTestCachedObjectField() { + final obj = _TestClass(); + final result = obj.varField; + _doNothing(); + + return result; +} + +class _TestClass { + static const constValue = 1; + static final finalValue = 1; + //ignore: member_ordering + final finalField = 1; + var varField = 1; +} + +void _doNothing() {} +''', + ); + } + + void test_does_not_report_if_return_used_variable() async { + await assertNoDiagnostics( + r''' +int returnVarTestUsedVariable() { + var a = 1; + final result = 2; + a += result; + + return result; +} +''', + ); + } + + void test_reports_if_return_is_bad_trivial() async { + await assertDiagnostics(r''' +int returnVarTestBadTrivial() { + final result = 1; + + return result; +} +''', [lint(55, 14)]); + } + + void test_reports_if_return_is_bad_immutable_expression() async { + await assertDiagnostics(r''' +int returnVarTestBadImmutableExpression() { + const constLocal = 1; + final finalLocal = 1; + final testObj = _TestClass(); + final result = constLocal + + finalLocal + + 1 + //const literal + _TestClass.constValue + + _TestClass.finalValue + + testObj.finalField; + _doNothing(); + + return result; +} + +class _TestClass { + static const constValue = 1; + static final finalValue = 1; + //ignore: member_ordering + final finalField = 1; + var varField = 1; +} + +void _doNothing() {} +''', [lint(304, 14)]); + } +}