From 6f791f1077dcb69eca3ff99183ba162fd1352b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexei=20Eleusis=20D=C3=ADaz=20Vera?= Date: Sat, 11 Jun 2016 22:53:04 -0700 Subject: [PATCH] Close instances of `dart.core.Sink`. --- lib/src/rules.dart | 2 + lib/src/rules/close_sinks.dart | 218 ++++++++++++++++++++++++++++++ test/_data/close_sinks/src/a.dart | 109 +++++++++++++++ test/integration_test.dart | 30 ++++ 4 files changed, 359 insertions(+) create mode 100644 lib/src/rules/close_sinks.dart create mode 100644 test/_data/close_sinks/src/a.dart diff --git a/lib/src/rules.dart b/lib/src/rules.dart index e938b167c..b71b87d17 100644 --- a/lib/src/rules.dart +++ b/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/close_sinks.dart'; import 'package:linter/src/rules/comment_references.dart'; import 'package:linter/src/rules/constant_identifier_names.dart'; import 'package:linter/src/rules/control_flow_in_finally.dart'; @@ -57,6 +58,7 @@ final Registry ruleRegistry = new Registry() ..register(new AvoidInitToNull()) ..register(new AwaitOnlyFutures()) ..register(new CamelCaseTypes()) + ..register(new CloseSinks()) ..register(new CommentReferences()) ..register(new ConstantIdentifierNames()) ..register(new UnrelatedTypeEqualityChecks()) diff --git a/lib/src/rules/close_sinks.dart b/lib/src/rules/close_sinks.dart new file mode 100644 index 000000000..e703763ef --- /dev/null +++ b/lib/src/rules/close_sinks.dart @@ -0,0 +1,218 @@ +// 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.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'; + +typedef void _VisitVariableDeclaration(VariableDeclaration node); + +typedef bool _Predicate(AstNode node); + +typedef _Predicate _PredicateBuilder(VariableDeclaration v); + +const _desc = r'Close instances of `dart.core.Sink`.'; + +const _details = r''' + +**DO** invoke `close` on instances of `dart.core.Sink` to avoid memory leaks and +unexpected behaviors. + +**BAD:** +``` +class A { + IOSink _sinkA; + void init(filename) { + _sinkA = new File(filename).openWrite(); // LINT + } +} +``` + +**BAD:** +``` +void someFunction() { + IOSink _sinkF; // LINT +} +``` + +**GOOD:** +``` +class B { + IOSink _sinkB; + void init(filename) { + _sinkB = new File(filename).openWrite(); // OK + } + + void dispose(filename) { + _sinkB.close(); + } +} +``` + +**GOOD:** +``` +void someFunctionOK() { + IOSink _sinkFOK; // OK + _sinkFOK.close(); +} +``` +'''; + +class CloseSinks extends LintRule { + CloseSinks() : super( + name: 'close_sinks', + description: _desc, + details: _details, + group: Group.errors, + maturity: Maturity.experimental); + + @override + AstVisitor getVisitor() => new _Visitor(this); +} + +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; + + static List<_PredicateBuilder> _variablePredicateBuiders = [_isSinkReturn]; + static List<_PredicateBuilder> _fieldPredicateBuiders = + [_hasConstructorFieldInitializers, _hasFieldFormalParemeter]; + + final LintRule rule; + + _Visitor(this.rule); + + @override + void visitVariableDeclarationStatement(VariableDeclarationStatement node) { + FunctionBody function = + node.getAncestor((a) => a is FunctionBody); + node.variables.variables.forEach( + _buildVariableReporter(function, _variablePredicateBuiders)); + } + + @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 containerNodes = _traverseNodesInDFS(container); + + List> validators = >[]; + 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 _findSinkAssignments(Iterable 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 _findMethodInvocations(Iterable containerNodes, + VariableDeclaration sink) { + Iterable 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 _findCloseCallbackNodes(Iterable containerNodes, + VariableDeclaration sink) { + Iterable 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 _findNodesClosingSink(Iterable 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); +} + +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 _traverseNodesInDFS(AstNode node) { + List nodes = []; + node.childEntities + .where((c) => c is AstNode) + .forEach((c) { + nodes.add(c); + nodes.addAll(_traverseNodesInDFS(c)); + }); + return nodes; +} diff --git a/test/_data/close_sinks/src/a.dart b/test/_data/close_sinks/src/a.dart new file mode 100644 index 000000000..13a0647f6 --- /dev/null +++ b/test/_data/close_sinks/src/a.dart @@ -0,0 +1,109 @@ +// 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. + +import 'dart:io'; +import 'dart:async'; +import 'package:mockito/mockito.dart'; + +class MockIOSink extends Mock implements IOSink {} + +class A { + IOSink _sinkA; // LINT + void init(filename) { + _sinkA = new File(filename).openWrite(); + } +} + +class B { + IOSink _sinkB; + void init(filename) { + _sinkB = new File(filename).openWrite(); // OK + } + + void dispose(filename) { + _sinkB.close(); + } +} + +class C { + final IOSink _sinkC; // OK + + C(this._sinkC); +} + +class C1 { + final IOSink _sinkC1; // OK + final Object unrelated; + + C1.initializer(IOSink sink, blah) : this._sinkC1 = sink, this.unrelated = blah; +} + +class C2 { + IOSink _sinkC2; // OK + + void initialize(IOSink sink){ + this._sinkC2 = sink; + } +} + +class C3 { + IOSink _sinkC3; // OK + + void initialize(IOSink sink){ + _sinkC3 = sink; + } +} + +class D { + IOSink init(filename) { + IOSink _sinkF = new File(filename).openWrite(); // OK + return _sinkF; + } +} + +void someFunction() { + IOSink _sinkF; // LINT +} + +void someFunctionOK() { + IOSink _sinkFOK; // OK + _sinkFOK.close(); +} + +IOSink someFunctionReturningIOSink() { + IOSink _sinkF = new File('filename').openWrite(); // OK + return _sinkF; +} + +void startChunkedConversion(Socket sink) { + IOSink stringSink; + if (sink is IOSink) { + stringSink = sink; + } else { + stringSink = new MockIOSink(); + } +} + +void onListen(Stream stream) { + StreamController controllerListen = new StreamController(); + stream.listen( + (int event) { + event.toString(); + }, + onError: controllerListen.addError, + onDone: controllerListen.close + ); +} + +void someFunctionClosing(StreamController controller) {} +void controllerPassedAsArgument() { + StreamController controllerArgument = new StreamController(); + someFunctionClosing(controllerArgument); +} + +void fluentInvocation() { + StreamController cascadeController = new StreamController() + ..add(null) + ..close(); +} diff --git a/test/integration_test.dart b/test/integration_test.dart index c0e869dcd..2b53f5cfa 100644 --- a/test/integration_test.dart +++ b/test/integration_test.dart @@ -178,6 +178,36 @@ defineTests() { }); }); + group('close_sinks', () { + IOSink currentOut = outSink; + CollectingSink collectingOut = new CollectingSink(); + setUp(() { + exitCode = 0; + outSink = collectingOut; + }); + tearDown(() { + collectingOut.buffer.clear(); + outSink = currentOut; + exitCode = 0; + }); + + test('close sinks', () { + dartlint.main([ + 'test/_data/close_sinks', + '--rules=close_sinks' + ]); + expect(exitCode, 1); + expect( + collectingOut.trim(), + stringContainsInOrder( + [ + 'IOSink _sinkA; // LINT', + 'IOSink _sinkF; // LINT', + '1 file analyzed, 2 issues found, in' + ])); + }); + }); + group('examples', () { test('lintconfig.yaml', () { var src = readFile('example/lintconfig.yaml');