Add new rule: dont_create_a_return_var (#220)

* Issue #92, new rule `dont_create_a_return_var`:
- added rule class (no rule implementation)
- added test cases

* Issue #92: added rule in analysis_options.yaml

* Issue #92: added new test case

* Issue #92: added naive implementation for dont_create_a_return_var_rule

* Issue #92: fixed conflicting lints in test

* Issue #92: added test case where return var defined from mutable, but return goes just after (bad practice, not valid caching case)

* Issue #92: implemented advanced rule with visitor, checking where variable is defined as well as how often is used

* Issue #92: fixed formatting with 'dart format'

* Issue #92: changed if chain to switch statement (more readable)

* Issue #92: renaming rule to avoid_unnecessary_return_variable

* Issue #92: small readability improvements

* #Issue 92: renamed test file accordingly

Author: tozpeak

lib/analysis_options.yaml

@@ -45,6 +45,7 @@ custom_lint:
 
     - avoid_non_null_assertion
     - avoid_returning_widgets
+    - avoid_unnecessary_return_variable
     - avoid_unnecessary_setstate
     - avoid_unnecessary_type_assertions
     - avoid_unnecessary_type_casts

lib/solid_lints.dart

@@ -7,6 +7,7 @@ import 'package:solid_lints/src/lints/avoid_global_state/avoid_global_state_rule
 import 'package:solid_lints/src/lints/avoid_late_keyword/avoid_late_keyword_rule.dart';
 import 'package:solid_lints/src/lints/avoid_non_null_assertion/avoid_non_null_assertion_rule.dart';
 import 'package:solid_lints/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.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_setstate/avoid_unnecessary_set_state_rule.dart';
 import 'package:solid_lints/src/lints/avoid_unnecessary_type_assertions/avoid_unnecessary_type_assertions_rule.dart';
 import 'package:solid_lints/src/lints/avoid_unnecessary_type_casts/avoid_unnecessary_type_casts_rule.dart';
@@ -67,6 +68,7 @@ class _SolidLints extends PluginBase {
       PreferEarlyReturnRule.createRule(configs),
       AvoidFinalWithGetterRule.createRule(configs),
       NamedParametersOrderingRule.createRule(configs),
+      AvoidUnnecessaryReturnVariableRule.createRule(configs),
     ];
 
     // Return only enabled rules

lib/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart

@@ -0,0 +1,155 @@
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/element/element2.dart';
+import 'package:analyzer/error/listener.dart';
+import 'package:custom_lint_builder/custom_lint_builder.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.
+///
+/// See more here: https://github.com/solid-software/solid_lints/issues/92
+///
+/// ### Example
+///
+/// #### BAD:
+///
+/// ```dart
+/// void x() {
+///   final y = 1;
+///
+///   return y;
+/// }
+/// ```
+///
+/// #### GOOD:
+///
+/// ```dart
+/// void x() {
+///   return 1;
+/// }
+/// ```
+///
+class AvoidUnnecessaryReturnVariableRule extends SolidLintRule {
+  /// This lint rule represents the error
+  /// when unnecessary return variable statement is found
+  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: (_) => """
+Avoid creating unnecessary variable only for return.
+Rewrite the variable evaluation into return statement instead.""",
+    );
+
+    return AvoidUnnecessaryReturnVariableRule._(rule);
+  }
+
+  @override
+  void run(
+    CustomLintResolver resolver,
+    ErrorReporter reporter,
+    CustomLintContext context,
+  ) {
+    context.registry.addReturnStatement(
+      (statement) {
+        _checkReturnStatement(
+          statement: statement,
+          reporter: reporter,
+          context: context,
+        );
+      },
+    );
+  }
+
+  void _checkReturnStatement({
+    required ReturnStatement statement,
+    required ErrorReporter reporter,
+    required CustomLintContext context,
+  }) {
+    final expr = statement.expression;
+    if (expr is! SimpleIdentifier) return;
+
+    final element = expr.element;
+    if (element is! LocalVariableElement2) 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) {
+    switch (expr) {
+      case Literal _:
+        return true;
+
+      case final PrefixedIdentifier prefixed:
+        return _isExpressionImmutable(prefixed.prefix) &&
+            _isExpressionImmutable(prefixed.identifier);
+
+      case final BinaryExpression binExpr:
+        return _isExpressionImmutable(binExpr.leftOperand) &&
+            _isExpressionImmutable(binExpr.rightOperand);
+
+      case final SimpleIdentifier identifier:
+        return _isSimpleIdentifierImmutable(identifier);
+    }
+
+    return false;
+  }
+
+  bool _isSimpleIdentifierImmutable(SimpleIdentifier identifier) {
+    switch (identifier.element) {
+      case final VariableElement2 variable:
+        return variable.isFinal || variable.isConst;
+
+      case ClassElement2 _:
+        return true;
+
+      case final GetterElement getter:
+        if (getter.variable3 case final PropertyInducingElement2 property
+            when property.isFinal || property.isConst) {
+          return true;
+        }
+    }
+
+    return false;
+  }
+}

lib/src/lints/avoid_unnecessary_return_variable/visitors/avoid_unnecessary_return_variable_visitor.dart

@@ -0,0 +1,79 @@
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/visitor.dart';
+import 'package:analyzer/dart/element/element2.dart';
+import 'package:collection/collection.dart';
+
+/// This visitor is searches all uses of a single local variable,
+/// including variable declaration.
+class AvoidUnnecessaryReturnVariableVisitor extends RecursiveAstVisitor<void> {
+  /// 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 existance is justified.
+  static const _badStatementCount = 1;
+
+  final LocalVariableElement2 _returnVariableElement;
+
+  bool _foundTokensBetweenDeclarationAndReturn = false;
+  VariableDeclaration? _variableDeclaration;
+  int _variableStatementCounter = 0;
+
+  final ReturnStatement _returnStatement;
+
+  /// After visiting holds info about whether there are any tokens
+  /// between variable declaration and return statement
+  bool get foundTokensBetweenDeclarationAndReturn =>
+      _foundTokensBetweenDeclarationAndReturn;
+
+  /// Returns statement of local variable declaration
+  VariableDeclaration? get variableDeclaration => _variableDeclaration;
+
+  /// Creates a new instance of [AvoidUnnecessaryReturnVariableVisitor].
+  AvoidUnnecessaryReturnVariableVisitor(
+    this._returnVariableElement,
+    this._returnStatement,
+  );
+
+  /// Defines whether the variables is used in return statement only.
+  bool hasBadStatementCount() =>
+      _variableStatementCounter == _badStatementCount;
+
+  @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.declaredElement2?.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;
+    }
+  }
+}

lint_test/analysis_options.yaml

@@ -89,3 +89,4 @@ custom_lint:
         - required
         - nullable
         - default
+    - avoid_unnecessary_return_variable

lint_test/avoid_unnecessary_return_variable_test.dart

@@ -0,0 +1,125 @@
+// 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;
+}