Merge pull request #638 from JPaulsen/fiX_linted_node_avoid_types_on_closure_parameters
Fix linted node: avoid_types_on_closure_parameters
diff --git a/lib/src/rules/cascade_invocations.dart b/lib/src/rules/cascade_invocations.dart
index 98296c9..5ed2654 100644
--- a/lib/src/rules/cascade_invocations.dart
+++ b/lib/src/rules/cascade_invocations.dart
@@ -9,6 +9,7 @@
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:linter/src/analyzer.dart';
+import 'package:linter/src/util/dart_type_utilities.dart';
const _desc = r'Cascade consecutive method invocations on the same reference.';
@@ -53,30 +54,49 @@
''';
-SimpleIdentifier _findTarget(AstNode node) {
- Expression expression;
- if (node is ExpressionStatement) {
- expression = node.expression;
+Element _getElementFromVariableDeclarationStatement(
+ VariableDeclarationStatement statement) {
+ final variables = statement.variables.variables;
+ if (variables.length == 1) {
+ return variables.first.element;
}
-
- if (expression is MethodInvocation &&
- expression.realTarget is SimpleIdentifier) {
- return expression.realTarget;
- }
-
- if (expression is PrefixedIdentifier &&
- expression.prefix is SimpleIdentifier) {
- return expression.prefix;
- }
-
- if (expression is AssignmentExpression &&
- expression.leftHandSide is PrefixedIdentifier) {
- return (expression.leftHandSide as PrefixedIdentifier).prefix;
- }
-
return null;
}
+ExecutableElement _getExecutableElementFromMethodInvocation(
+ MethodInvocation node) {
+ if (_isInvokedWithoutNullAwareOperator(node.operator)) {
+ final executableElement =
+ DartTypeUtilities.getCanonicalElementFromIdentifier(node.methodName);
+ if (executableElement is ExecutableElement) {
+ return executableElement;
+ }
+ }
+ return null;
+}
+
+Element _getPrefixElementFromExpression(Expression rawExpression) {
+ final expression = rawExpression.unParenthesized;
+ if (expression is PrefixedIdentifier) {
+ return DartTypeUtilities
+ .getCanonicalElementFromIdentifier(expression.prefix);
+ } else if (expression is PropertyAccess &&
+ _isInvokedWithoutNullAwareOperator(expression.operator)) {
+ return DartTypeUtilities
+ .getCanonicalElementFromIdentifier(expression.target);
+ }
+ return null;
+}
+
+Element _getTargetElementFromCascadeExpression(CascadeExpression node) =>
+ DartTypeUtilities.getCanonicalElementFromIdentifier(node.target);
+
+Element _getTargetElementFromMethodInvocation(MethodInvocation node) =>
+ DartTypeUtilities.getCanonicalElementFromIdentifier(node.target);
+
+bool _isInvokedWithoutNullAwareOperator(Token token) =>
+ token?.type == TokenType.PERIOD;
+
/// Rule to lint consecutive invocations of methods or getters on the same
/// reference that could be done with the cascade operator.
class CascadeInvocations extends LintRule {
@@ -96,57 +116,161 @@
AstVisitor getVisitor() => _visitor;
}
-class _Visitor extends SimpleAstVisitor {
- final LintRule rule;
- _Visitor(this.rule);
+/// A CascadableExpression is an object that is built from an expression and
+/// knows if it is able to join to another CascadableExpression.
+class _CascadableExpression {
+ static final NULL_CASCADABLE_EXPRESSION = new _CascadableExpression._internal(
+ null, [],
+ canJoin: false, canReceive: false, canBeCascaded: false);
- @override
- void visitExpressionStatement(ExpressionStatement node) {
- final prefixIdentifier = _findTarget(node);
- if (prefixIdentifier == null || node.parent is! Block) {
- return;
- }
+ final bool canJoin;
+ final bool canReceive;
+ final bool canBeCascaded;
- final Block body = node.parent;
- final previousNodes = body.statements.takeWhile((n) => n != node);
- if (previousNodes.isEmpty) {
- return;
- }
+ /// This is necessary when you have a variable declaration so that element
+ /// is critical and it can't be used as a parameter of a method invocation or
+ /// in the right part of an assignment in a following expression that we would
+ /// like to join to this.
+ final bool isCritical;
+ final Element element;
+ final List<AstNode> criticalNodes;
- final previousNode = previousNodes.last;
- final SimpleIdentifier previousIdentifier = _findTarget(previousNode);
- if (previousIdentifier != null &&
- previousIdentifier.staticElement == prefixIdentifier.staticElement &&
- previousIdentifier.staticElement is! PrefixElement &&
- previousIdentifier.bestElement is! ClassElement) {
- rule.reportLint(node);
+ factory _CascadableExpression.fromExpressionStatement(
+ ExpressionStatement statement) {
+ final expression = statement.expression.unParenthesized;
+ if (expression is AssignmentExpression) {
+ return new _CascadableExpression._fromAssignmentExpression(expression);
}
-
- if (previousNode is VariableDeclarationStatement &&
- _isInvokedWithoutNullAwareOperator(node)) {
- _reportIfDeclarationFollowedByMethodInvocation(
- previousNode, prefixIdentifier, node);
+ if (expression is MethodInvocation) {
+ return new _CascadableExpression._fromMethodInvocation(expression);
}
+ if (expression is CascadeExpression) {
+ return new _CascadableExpression._fromCascadeExpression(expression);
+ }
+ if (expression is PropertyAccess &&
+ _isInvokedWithoutNullAwareOperator(expression.operator)) {
+ return new _CascadableExpression._fromPropertyAccess(expression);
+ }
+ if (expression is PrefixedIdentifier) {
+ return new _CascadableExpression._fromPrefixedIdentifier(expression);
+ }
+ return NULL_CASCADABLE_EXPRESSION;
}
- void _reportIfDeclarationFollowedByMethodInvocation(
- VariableDeclarationStatement variablesDeclaration,
- SimpleIdentifier simpleIdentifier,
- AstNode node) {
- final variables = variablesDeclaration.variables.variables;
- if (variables.length == 1 &&
- variables.first.name.staticElement == simpleIdentifier.staticElement) {
- rule.reportLint(node);
+ factory _CascadableExpression.fromVariableDeclarationStatement(
+ VariableDeclarationStatement node) {
+ final element = _getElementFromVariableDeclarationStatement(node);
+ return new _CascadableExpression._internal(element, [],
+ canJoin: false,
+ canReceive: true,
+ canBeCascaded: false,
+ isCritical: true);
+ }
+
+ factory _CascadableExpression._fromAssignmentExpression(
+ AssignmentExpression node) {
+ final leftExpression = node.leftHandSide.unParenthesized;
+ if (leftExpression is SimpleIdentifier) {
+ return new _CascadableExpression._internal(
+ DartTypeUtilities.getCanonicalElement(leftExpression.bestElement),
+ [node.rightHandSide],
+ canJoin: false,
+ canReceive: true,
+ canBeCascaded: false,
+ isCritical: true);
}
+ // setters
+ return new _CascadableExpression._internal(
+ _getPrefixElementFromExpression(node.leftHandSide),
+ [node.rightHandSide],
+ canJoin: true,
+ canReceive: true,
+ canBeCascaded: true);
+ }
+
+ factory _CascadableExpression._fromCascadeExpression(
+ CascadeExpression node) =>
+ new _CascadableExpression._internal(
+ _getTargetElementFromCascadeExpression(node), node.cascadeSections,
+ canJoin: true, canReceive: true, canBeCascaded: true);
+
+ factory _CascadableExpression._fromMethodInvocation(MethodInvocation node) {
+ final executableElement = _getExecutableElementFromMethodInvocation(node);
+ bool isNonStatic = executableElement?.isStatic == false;
+ if (isNonStatic) {
+ return new _CascadableExpression._internal(
+ _getTargetElementFromMethodInvocation(node), [node.argumentList],
+ canJoin: true, canReceive: true, canBeCascaded: true);
+ }
+ return NULL_CASCADABLE_EXPRESSION;
+ }
+
+ factory _CascadableExpression._fromPrefixedIdentifier(
+ PrefixedIdentifier node) {
+ return new _CascadableExpression._internal(
+ DartTypeUtilities.getCanonicalElementFromIdentifier(node.prefix), [],
+ canJoin: true, canReceive: true, canBeCascaded: true);
+ }
+
+ factory _CascadableExpression._fromPropertyAccess(PropertyAccess node) {
+ return new _CascadableExpression._internal(
+ DartTypeUtilities.getCanonicalElementFromIdentifier(node.target), [],
+ canJoin: true, canReceive: true, canBeCascaded: true);
+ }
+
+ _CascadableExpression._internal(this.element, this.criticalNodes,
+ {this.canJoin,
+ this.canReceive,
+ this.canBeCascaded,
+ this.isCritical: false});
+
+ bool compatibleWith(_CascadableExpression expressionBox) =>
+ element != null &&
+ expressionBox.canReceive &&
+ canJoin &&
+ (canBeCascaded || expressionBox.canBeCascaded) &&
+ element == expressionBox.element &&
+ !_hasCriticalDependencies(expressionBox);
+
+ bool _hasCriticalDependencies(_CascadableExpression expressionBox) {
+ bool _isCriticalNode(AstNode node) =>
+ DartTypeUtilities.getCanonicalElementFromIdentifier(node) ==
+ expressionBox.element;
+ return expressionBox.isCritical &&
+ criticalNodes.any((node) =>
+ _isCriticalNode(node) ||
+ DartTypeUtilities.traverseNodesInDFS(node).any(_isCriticalNode));
}
}
-bool _isInvokedWithoutNullAwareOperator(AstNode node) {
- if (node is ExpressionStatement && node.expression is MethodInvocation) {
- final tokenType = (node.expression as MethodInvocation).operator.type;
- return tokenType == TokenType.PERIOD ||
- tokenType == TokenType.PERIOD_PERIOD;
- }
+class _Visitor extends SimpleAstVisitor {
+ final LintRule rule;
- return false;
+ _Visitor(this.rule);
+
+ @override
+ visitBlock(Block node) {
+ if (node.statements.length < 2) {
+ return;
+ }
+ var previousExpressionBox =
+ _CascadableExpression.NULL_CASCADABLE_EXPRESSION;
+ for (final statement in node.statements) {
+ var currentExpressionBox =
+ _CascadableExpression.NULL_CASCADABLE_EXPRESSION;
+ if (statement is VariableDeclarationStatement) {
+ currentExpressionBox =
+ new _CascadableExpression.fromVariableDeclarationStatement(
+ statement);
+ }
+ if (statement is ExpressionStatement) {
+ currentExpressionBox =
+ new _CascadableExpression.fromExpressionStatement(statement);
+ }
+ if (currentExpressionBox.compatibleWith(previousExpressionBox)) {
+ rule.reportLint(statement);
+ }
+ previousExpressionBox = currentExpressionBox;
+ }
+ }
}
diff --git a/test/rules/cascade_invocations.dart b/test/rules/cascade_invocations.dart
index 1065506..bf4e79a 100644
--- a/test/rules/cascade_invocations.dart
+++ b/test/rules/cascade_invocations.dart
@@ -6,9 +6,16 @@
import 'dart:math' as math;
+void alreadyDeclared() {
+ List list;
+ print("intermediate statement");
+ list = [];
+ list.clear(); // LINT
+}
+
void noCascade() {
List<int> list = [];
- list.removeWhere((i) => i % 5 == 0); // LINT
+ list.clear(); // LINT
list.clear(); // LINT
}
@@ -17,15 +24,27 @@
void noCascadeIntermediate() {
List<int> list = [];
print(list);
- list.removeWhere((i) => i % 5 == 0);
+ list.clear();
list.clear(); // LINT
+ list.toString(); // LINT
+ // toString is not void and it is not a problem.
+}
+
+void cascadeIntermediate() {
+ List<int> list = [];
+ print(list);
+ list.clear();
+ list.clear(); // LINT
+ list..clear(); // LINT
+ list..clear(); // LINT
+ list..clear(); // LINT
}
class HasMethodWithNoCascade {
List<int> list = [];
void noCascade() {
- list.removeWhere((i) => i % 5 == 0);
+ list.clear();
list.clear(); // LINT
}
}
@@ -43,6 +62,9 @@
foo.bar; // LINT
foo.foo(); // LINT
foo.bar = 8; // LINT
+ foo?.bar = 8; // OK
+ foo.bar = 0; // OK
+ foo?.bar; // OK
}
void alternatingReferences() {
@@ -68,6 +90,8 @@
void cascade() {
final foo = new Foo();
foo?.baz();
+ foo.baz(); // OK
+ foo.baz(); // LINT
}
void prefixLibrary() {
@@ -80,8 +104,60 @@
static int decrement() => 0;
}
- void cascadeStatic() {
- StaticFoo.increment();
- StaticFoo.decrement();
- }
+void cascadeStatic() {
+ StaticFoo.increment();
+ StaticFoo.decrement();
+}
+
+// Bug 339
+class Identifier339 {
+ String value;
+ String system;
+}
+
+class Resource339 {
+ String id;
+ dynamic identifier;
+}
+
+class Practitioner339 extends Resource339{
+ int foo;
+
+ String build() => null;
+
+ void bar(dynamic) {}
+}
+
+class Entry339 {
+ Resource339 resource;
+}
+
+void main339() {
+ final entry = new Entry339();
+ final resource = (entry.resource as Practitioner339);
+ resource.identifier = [ // OK
+ new Identifier339()
+ ..system = 'urn:system'
+ ..value = 'mdc_${resource.id}'
+ ];
+}
+
+void otherDependencies1() {
+ final entry = new Entry339();
+ final resource = (entry.resource as Practitioner339);
+ resource.identifier = resource; // OK
+}
+
+void otherDependencies2() {
+ final entry = new Entry339();
+ final resource = (entry.resource as Practitioner339);
+ resource.bar(resource); // OK
+}
+
+// assignment
+void otherDependencies3() {
+ var entry, resource;
+ entry = new Entry339();
+ resource = (entry.resource as Practitioner339);
+ resource.bar(resource); // OK
}
