Merge pull request #242 from alexeieleusis/cancel_subscriptions

Cancel subscriptions

lib/src/rules.dart

@@ -17,6 +17,7 @@ import 'package:linter/src/rules/avoid_init_to_null.dart';
 import 'package:linter/src/rules/avoid_return_types_on_setters.dart';
 import 'package:linter/src/rules/await_only_futures.dart';
 import 'package:linter/src/rules/camel_case_types.dart';
+import 'package:linter/src/rules/cancel_subscriptions.dart';
 import 'package:linter/src/rules/close_sinks.dart';
 import 'package:linter/src/rules/comment_references.dart';
 import 'package:linter/src/rules/constant_identifier_names.dart';
@@ -58,6 +59,7 @@ final Registry ruleRegistry = new Registry()
   ..register(new AvoidInitToNull())
   ..register(new AwaitOnlyFutures())
   ..register(new CamelCaseTypes())
+  ..register(new CancelSubscriptions())
   ..register(new CloseSinks())
   ..register(new CommentReferences())
   ..register(new ConstantIdentifierNames())

lib/src/rules/cancel_subscriptions.dart

@@ -0,0 +1,89 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library linter.src.rules.cancel_subscriptions;
+
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/element/type.dart';
+import 'package:linter/src/linter.dart';
+import 'package:linter/src/util/dart_type_utilities.dart';
+import 'package:linter/src/util/leak_detector_visitor.dart';
+
+const _desc = r'Cancel instances of dart.async.StreamSubscription.';
+
+const _details = r'''
+
+**DO** Invoke `cancel` on instances of `dart.async.StreamSubscription` to avoid
+memory leaks and unexpected behaviors.
+
+**BAD:**
+```
+class A {
+  StreamSubscription _subscriptionA; // LINT
+  void init(Stream stream) {
+    _subscriptionA = stream.listen((_) {});
+  }
+}
+```
+
+**BAD:**
+```
+void someFunction() {
+  StreamSubscription _subscriptionF; // LINT
+}
+```
+
+**GOOD:**
+```
+class B {
+  StreamSubscription _subscriptionB; // OK
+  void init(Stream stream) {
+    _subscriptionB = stream.listen((_) {});
+  }
+
+  void dispose(filename) {
+    _subscriptionB.cancel();
+  }
+}
+```
+
+**GOOD:**
+```
+void someFunctionOK() {
+  StreamSubscription _subscriptionB; // OK
+  _subscriptionB.cancel();
+}
+```
+''';
+
+class CancelSubscriptions extends LintRule {
+  _Visitor _visitor;
+
+  CancelSubscriptions() : super(
+      name: 'cancel_subscriptions',
+      description: _desc,
+      details: _details,
+      group: Group.errors,
+      maturity: Maturity.experimental) {
+    _visitor = new _Visitor(this);
+  }
+
+  @override
+  AstVisitor getVisitor() => _visitor;
+}
+
+class _Visitor extends LeakDetectorVisitor {
+  static const _cancelMethodName = 'cancel';
+
+  _Visitor(LintRule rule) : super(rule);
+
+  @override
+  String get methodName => _cancelMethodName;
+
+  @override
+  DartTypePredicate get predicate => _isSubscription;
+}
+
+bool _isSubscription(DartType type) =>
+    DartTypeUtilities.implementsInterface(type, 'StreamSubscription', 'dart.async');

lib/src/rules/close_sinks.dart

@@ -5,16 +5,12 @@
 library linter.src.rules.close_sinks;
 
 import 'package:analyzer/dart/ast/ast.dart';
-import 'package:analyzer/dart/ast/visitor.dart';
-import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:linter/src/linter.dart';
+import 'package:linter/src/util/dart_type_utilities.dart';
+import 'package:linter/src/util/leak_detector_visitor.dart';
 
-typedef void _VisitVariableDeclaration(VariableDeclaration node);
 
-typedef bool _Predicate(AstNode node);
-
-typedef _Predicate _PredicateBuilder(VariableDeclaration v);
 
 const _desc = r'Close instances of `dart.core.Sink`.';
 
@@ -64,155 +60,32 @@ void someFunctionOK() {
 ''';
 
 class CloseSinks extends LintRule {
+  _Visitor _visitor;
+
   CloseSinks() : super(
       name: 'close_sinks',
       description: _desc,
       details: _details,
       group: Group.errors,
-      maturity: Maturity.experimental);
+      maturity: Maturity.experimental) {
+    _visitor = new _Visitor(this);
+  }
 
   @override
-  AstVisitor getVisitor() => new _Visitor(this);
+  AstVisitor getVisitor() => _visitor;
 }
 
-class _Visitor extends SimpleAstVisitor {
-  static _PredicateBuilder _isSinkReturn =
-      (VariableDeclaration v) =>
-      (n) => n is ReturnStatement &&
-      n.expression is SimpleIdentifier &&
-      (n.expression as SimpleIdentifier).token.lexeme == v.name.token.lexeme;
-
-  static _PredicateBuilder _hasConstructorFieldInitializers =
-      (VariableDeclaration v) =>
-      (n) => n is ConstructorFieldInitializer &&
-      n.fieldName.name == v.name.token.lexeme;
-
-  static _PredicateBuilder _hasFieldFormalParemeter =
-      (VariableDeclaration v) =>
-      (n) => n is FieldFormalParameter &&
-      n.identifier.name == v.name.token.lexeme;
+class _Visitor extends LeakDetectorVisitor {
+  static const _closeMethodName = 'close';
 
-  static List<_PredicateBuilder> _variablePredicateBuiders = [_isSinkReturn];
-  static List<_PredicateBuilder> _fieldPredicateBuiders =
-    [_hasConstructorFieldInitializers, _hasFieldFormalParemeter];
-
-  final LintRule rule;
-
-  _Visitor(this.rule);
+  _Visitor(LintRule rule) : super(rule);
 
   @override
-  void visitVariableDeclarationStatement(VariableDeclarationStatement node) {
-    FunctionBody function =
-        node.getAncestor((a) => a is FunctionBody);
-    node.variables.variables.forEach(
-        _buildVariableReporter(function, _variablePredicateBuiders));
-  }
+  String get methodName => _closeMethodName;
 
   @override
-  void visitFieldDeclaration(FieldDeclaration node) {
-    ClassDeclaration classDecl =
-        node.getAncestor((a) => a is ClassDeclaration);
-    node.fields.variables.forEach(
-        _buildVariableReporter(classDecl, _fieldPredicateBuiders));
-  }
-
-  /// Builds a function that reports the variable node if the set of nodes
-  /// inside the [container] node is empty for all the predicates resulting
-  /// from building (predicates) with the provided [predicateBuilders] evaluated
-  /// in the variable.
-  _VisitVariableDeclaration _buildVariableReporter(AstNode container,
-      List<_PredicateBuilder> predicateBuilders) =>
-          (VariableDeclaration sink) {
-        if (!_implementsDartCoreSink(sink.element.type)) {
-          return;
-        }
-
-        List<AstNode> containerNodes = _traverseNodesInDFS(container);
-
-        List<Iterable<AstNode>> validators = <Iterable<AstNode>>[];
-        predicateBuilders.forEach((f) {
-          validators.add(containerNodes.where(f(sink)));
-        });
-
-        validators.add(_findSinkAssignments(containerNodes, sink));
-        validators.add(_findNodesClosingSink(containerNodes, sink));
-        validators.add(_findCloseCallbackNodes(containerNodes, sink));
-        // If any function is invoked with our sink, we supress lints. This is
-        // because it is not so uncommon to close the sink there. We might not
-        // have access to the body of such function at analysis time, so trying
-        // to infer if the close method is invoked there is not always possible.
-        // TODO: Should there be another lint more relaxed that omits this step?
-        validators.add(_findMethodInvocations(containerNodes, sink));
-
-        // Read this as: validators.forAll((i) => i.isEmpty).
-        if (!validators.any((i) => !i.isEmpty)) {
-          rule.reportLint(sink);
-        }
-      };
-}
-
-Iterable<AstNode> _findSinkAssignments(Iterable<AstNode> containerNodes,
-    VariableDeclaration sink) =>
-    containerNodes.where((n) {
-      return n is AssignmentExpression &&
-          ((n.leftHandSide is SimpleIdentifier &&
-              // Assignment to sink as variable.
-              (n.leftHandSide as SimpleIdentifier).token.lexeme ==
-                  sink.name.token.lexeme) ||
-              // Assignment to sink as setter.
-              (n.leftHandSide is PropertyAccess &&
-                  (n.leftHandSide as PropertyAccess)
-                      .propertyName.token.lexeme == sink.name.token.lexeme))
-          // Being assigned another reference.
-          && n.rightHandSide is SimpleIdentifier;
-    });
-
-Iterable<AstNode> _findMethodInvocations(Iterable<AstNode> containerNodes,
-    VariableDeclaration sink) {
-  Iterable<MethodInvocation> prefixedIdentifiers =
-  containerNodes.where((n) => n is MethodInvocation);
-  return prefixedIdentifiers.where((n) =>
-  n.argumentList.arguments.map((e) => e is SimpleIdentifier ? e.name : '')
-      .contains(sink.name.token.lexeme));
-}
-
-Iterable<AstNode> _findCloseCallbackNodes(Iterable<AstNode> containerNodes,
-    VariableDeclaration sink) {
-  Iterable<PrefixedIdentifier> prefixedIdentifiers =
-      containerNodes.where((n) => n is PrefixedIdentifier);
-  return prefixedIdentifiers.where((n) =>
-    n.prefix.token.lexeme == sink.name.token.lexeme &&
-        n.identifier.token.lexeme == 'close');
-}
-
-Iterable<AstNode> _findNodesClosingSink(Iterable<AstNode> classNodes,
-    VariableDeclaration variable) => classNodes.where(
-    (n) => n is MethodInvocation &&
-        n.methodName.name == 'close' &&
-        ((n.target is SimpleIdentifier &&
-            (n.target as SimpleIdentifier).name == variable.name.name) ||
-            (n.getAncestor((a) => a == variable) != null)));
-
-bool _implementsDartCoreSink(DartType type) {
-  ClassElement element = type.element;
-  return !element.isSynthetic &&
-      type is InterfaceType &&
-      element.allSupertypes.any(_isDartCoreSink);
+  DartTypePredicate get predicate => _isSink;
 }
 
-bool _isDartCoreSink(InterfaceType interface) =>
-    interface.name == 'Sink' &&
-        interface.element.library.name == 'dart.core';
-
-/// Builds the list resulting from traversing the node in DFS and does not
-/// include the node itself.
-List<AstNode> _traverseNodesInDFS(AstNode node) {
-  List<AstNode> nodes = [];
-  node.childEntities
-      .where((c) => c is AstNode)
-      .forEach((c) {
-    nodes.add(c);
-    nodes.addAll(_traverseNodesInDFS(c));
-  });
-  return nodes;
-}
+bool _isSink(DartType type) =>
+    DartTypeUtilities.implementsInterface(type, 'Sink', 'dart.core');

lib/src/rules/iterable_contains_unrelated_type.dart

@@ -8,7 +8,7 @@ import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:linter/src/linter.dart';
-import 'package:linter/src/rules/unrelated_type_equality_checks.dart';
+import 'package:linter/src/util/dart_type_utilities.dart';
 
 const _desc = r'Invocation of Iterable<E>.contains with references of unrelated'
     r' types.';
@@ -195,7 +195,8 @@ class _Visitor extends SimpleAstVisitor {
             ?.element
             ?.type;
     Expression argument = node.argumentList.arguments.first;
-    if (unrelatedTypes(argument.bestType, _findIterableTypeArgument(type))) {
+    if (DartTypeUtilities.unrelatedTypes(
+        argument.bestType, _findIterableTypeArgument(type))) {
       rule.reportLint(node);
     }
   }

lib/src/rules/unrelated_type_equality_checks.dart

@@ -7,9 +7,8 @@ library linter.src.rules.unrelated_type_equality_checks;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
-import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/dart/element/type.dart';
 import 'package:linter/src/linter.dart';
+import 'package:linter/src/util/dart_type_utilities.dart';
 
 const String _desc = r'Equality operator (==) invocation with references of'
     r' unrelated types.';
@@ -168,23 +167,4 @@ class _Visitor extends SimpleAstVisitor {
 bool _hasNonComparableOperands(BinaryExpression node) =>
     node.leftOperand is! NullLiteral &&
         node.rightOperand is! NullLiteral &&
-        unrelatedTypes(node.leftOperand.bestType, node.rightOperand.bestType);
-
-bool unrelatedTypes(DartType leftType, DartType rightType) {
-  if (leftType == null || leftType.isBottom || leftType.isDynamic ||
-      rightType == null || rightType.isBottom || rightType.isDynamic) {
-    return false;
-    }
-    if (leftType == rightType ||
-        leftType.isMoreSpecificThan(rightType) ||
-        rightType.isMoreSpecificThan(leftType)) {
-      return false;
-    }
-    Element leftElement = leftType.element;
-    Element rightElement = rightType.element;
-    if (leftElement is ClassElement && rightElement is ClassElement) {
-      return leftElement.supertype.isObject ||
-          leftElement.supertype != rightElement.supertype;
-    }
-    return false;
-}
+        DartTypeUtilities.unrelatedTypes(node.leftOperand.bestType, node.rightOperand.bestType);