From e627d8b85231efa81ac8571aab6977a618a975e4 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Fri, 9 Aug 2019 07:12:00 +0000 Subject: [PATCH] [nnbd_migration] use postdominators to do hard edge detection in most cases Change-Id: I6af934b4a4795a0cfd74519985b24ae171485880 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111941 Reviewed-by: Paul Berry Commit-Queue: Mike Fairhurst --- pkg/analyzer/lib/src/dart/ast/ast.dart | 2 - pkg/nnbd_migration/lib/src/edge_builder.dart | 177 +++++-- .../lib/src/utilities/scoped_set.dart | 80 +++ pkg/nnbd_migration/test/api_test.dart | 66 +++ .../test/edge_builder_test.dart | 457 ++++++++++++++++++ pkg/nnbd_migration/test/test_all.dart | 2 + .../test/utilities/scoped_set_test.dart | 182 +++++++ .../test/utilities/test_all.dart | 13 + 8 files changed, 947 insertions(+), 32 deletions(-) create mode 100644 pkg/nnbd_migration/lib/src/utilities/scoped_set.dart create mode 100644 pkg/nnbd_migration/test/utilities/scoped_set_test.dart create mode 100644 pkg/nnbd_migration/test/utilities/test_all.dart diff --git a/pkg/analyzer/lib/src/dart/ast/ast.dart b/pkg/analyzer/lib/src/dart/ast/ast.dart index c5b19d603c1e..87d899b95583 100644 --- a/pkg/analyzer/lib/src/dart/ast/ast.dart +++ b/pkg/analyzer/lib/src/dart/ast/ast.dart @@ -4661,7 +4661,6 @@ class ForPartsWithDeclarationsImpl extends ForPartsImpl @override Token get beginToken => _variableList?.beginToken ?? super.beginToken; - @override Iterable get childEntities => new ChildEntities() ..add(_variableList) @@ -4704,7 +4703,6 @@ class ForPartsWithExpressionImpl extends ForPartsImpl @override Token get beginToken => initialization?.beginToken ?? super.beginToken; - @override Iterable get childEntities => new ChildEntities() ..add(_initialization) diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart index 34f8bfc06e3f..6b588906c9e2 100644 --- a/pkg/nnbd_migration/lib/src/edge_builder.dart +++ b/pkg/nnbd_migration/lib/src/edge_builder.dart @@ -7,6 +7,7 @@ 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:analyzer/src/dart/ast/ast.dart'; import 'package:analyzer/src/dart/element/handle.dart'; import 'package:analyzer/src/dart/element/inheritance_manager3.dart'; import 'package:analyzer/src/dart/element/member.dart'; @@ -22,6 +23,7 @@ import 'package:nnbd_migration/src/edge_origin.dart'; import 'package:nnbd_migration/src/expression_checks.dart'; import 'package:nnbd_migration/src/node_builder.dart'; import 'package:nnbd_migration/src/nullability_node.dart'; +import 'package:nnbd_migration/src/utilities/scoped_set.dart'; /// Test class mixing in _AssignmentChecker, to allow [checkAssignment] to be /// more easily unit tested. @@ -122,11 +124,13 @@ class EdgeBuilder extends GeneralizingAstVisitor /// code that can be proven unreachable by the migration tool. final _guards = []; - /// Indicates whether the statement or expression being visited is within - /// conditional control flow. If `true`, this means that the enclosing - /// function might complete normally without executing the current statement - /// or expression. - bool _inConditionalControlFlow = false; + /// The scope of locals (parameters, variables) that are post-dominated by the + /// current node as we walk the AST. We use a [ScopedSet] so that outer + /// scopes may track their post-dominators separately from inner scopes. + /// + /// Note that this is not guaranteed to be complete. It is used to make hard + /// edges on a best-effort basis. + final _postDominatedLocals = _ScopedLocalSet(); NullabilityNode _lastConditionalNode; @@ -209,10 +213,10 @@ class EdgeBuilder extends GeneralizingAstVisitor DecoratedType visitAssertStatement(AssertStatement node) { _handleAssignment(node.condition, _notNullType); if (identical(_conditionInfo?.condition, node.condition)) { - if (!_inConditionalControlFlow && - _conditionInfo.trueDemonstratesNonNullIntent != null) { - _connect(_conditionInfo.trueDemonstratesNonNullIntent, _graph.never, - NonNullAssertionOrigin(_source, node.offset), + var intentNode = _conditionInfo.trueDemonstratesNonNullIntent; + if (intentNode != null && _conditionInfo.postDominatingIntent) { + _graph.connect(_conditionInfo.trueDemonstratesNonNullIntent, + _graph.never, NonNullAssertionOrigin(_source, node.offset), hard: true); } } @@ -226,6 +230,7 @@ class EdgeBuilder extends GeneralizingAstVisitor // TODO(paulberry) _unimplemented(node, 'Assignment with operator ${node.operator.lexeme}'); } + _postDominatedLocals.removeReferenceFromAllScopes(node.leftHandSide); var leftType = node.leftHandSide.accept(this); var conditionalNode = _lastConditionalNode; _lastConditionalNode = null; @@ -263,6 +268,8 @@ class EdgeBuilder extends GeneralizingAstVisitor bool isPure = node.leftOperand is SimpleIdentifier; var conditionInfo = _ConditionInfo(node, isPure: isPure, + postDominatingIntent: + _postDominatedLocals.isReferenceInScope(node.leftOperand), trueGuard: leftType.node, falseDemonstratesNonNullIntent: leftType.node); _conditionInfo = operatorType == TokenType.EQ_EQ @@ -273,7 +280,8 @@ class EdgeBuilder extends GeneralizingAstVisitor } else if (operatorType == TokenType.AMPERSAND_AMPERSAND || operatorType == TokenType.BAR_BAR) { _handleAssignment(node.leftOperand, _notNullType); - _handleAssignment(node.rightOperand, _notNullType); + _postDominatedLocals.doScoped( + action: () => _handleAssignment(node.rightOperand, _notNullType)); return _nonNullableBoolType; } else if (operatorType == TokenType.QUESTION_QUESTION) { DecoratedType expressionType; @@ -317,6 +325,16 @@ class EdgeBuilder extends GeneralizingAstVisitor return DecoratedType(node.staticType, _graph.never); } + @override + DecoratedType visitBreakStatement(BreakStatement node) { + // Later statements no longer post-dominate the declarations because we + // exited (or, in parent scopes, conditionally exited). + // TODO(mfairhurst): don't clear post-dominators beyond the current loop. + _postDominatedLocals.clearEachScope(); + + return null; + } + @override DecoratedType visitCascadeExpression(CascadeExpression node) { var type = node.target.accept(this); @@ -365,9 +383,19 @@ class EdgeBuilder extends GeneralizingAstVisitor @override DecoratedType visitConditionalExpression(ConditionalExpression node) { _handleAssignment(node.condition, _notNullType); + + DecoratedType thenType; + DecoratedType elseType; + // TODO(paulberry): guard anything inside the true and false branches - var thenType = node.thenExpression.accept(this); - var elseType = node.elseExpression.accept(this); + + // Post-dominators diverge as we branch in the conditional. + // Note: we don't have to create a scope for each branch because they can't + // define variables. + _postDominatedLocals.doScoped(action: () { + thenType = node.thenExpression.accept(this); + elseType = node.elseExpression.accept(this); + }); var overallType = _decorateUpperOrLowerBound( node, node.staticType, thenType, elseType, true); @@ -397,6 +425,16 @@ class EdgeBuilder extends GeneralizingAstVisitor return null; } + @override + DecoratedType visitContinueStatement(ContinueStatement node) { + // Later statements no longer post-dominate the declarations because we + // exited (or, in parent scopes, conditionally exited). + // TODO(mfairhurst): don't clear post-dominators beyond the current loop. + _postDominatedLocals.clearEachScope(); + + return null; + } + @override DecoratedType visitDefaultFormalParameter(DefaultFormalParameter node) { node.parameter.accept(this); @@ -458,15 +496,54 @@ class EdgeBuilder extends GeneralizingAstVisitor return null; } + @override + DecoratedType visitForStatement(ForStatement node) { + // TODO do special condition handling + // TODO do create true/false guards? + // Create a scope of for new initializers etc, which includes previous + // post-dominators. + _postDominatedLocals.doScoped( + copyCurrent: true, + action: () { + final parts = node.forLoopParts; + if (parts is ForParts) { + if (parts is ForPartsWithDeclarations) { + parts.variables.accept(this); + } + if (parts is ForPartsWithExpression) { + parts.initialization.accept(this); + } + parts.condition.accept(this); + } + if (parts is ForEachParts) { + parts.iterable.accept(this); + } + + // The condition may fail/iterable may be empty, so the body does not + // post-dominate the parts, or the outer scope. + _postDominatedLocals.popScope(); + _postDominatedLocals.pushScope(); + + if (parts is ForParts) { + parts.updaters.accept(this); + } + + node.body.accept(this); + }); + return null; + } + @override DecoratedType visitFunctionDeclaration(FunctionDeclaration node) { node.functionExpression.parameters?.accept(this); assert(_currentFunctionType == null); _currentFunctionType = _variables.decoratedElementType(node.declaredElement); - _inConditionalControlFlow = false; + // Initialize a new postDominator scope that contains only the parameters. try { - node.functionExpression.body.accept(this); + _postDominatedLocals.doScoped( + elements: node.functionExpression.declaredElement.parameters, + action: () => node.functionExpression.body.accept(this)); } finally { _currentFunctionType = null; } @@ -476,6 +553,7 @@ class EdgeBuilder extends GeneralizingAstVisitor @override DecoratedType visitFunctionExpression(FunctionExpression node) { // TODO(brianwilkerson) + // TODO(mfairhurst): enable edge builder "_insideFunction" hard edge tests. _unimplemented(node, 'FunctionExpression'); } @@ -492,7 +570,6 @@ class EdgeBuilder extends GeneralizingAstVisitor // TODO(paulberry): should the use of a boolean in an if-statement be // treated like an implicit `assert(b != null)`? Probably. _handleAssignment(node.condition, _notNullType); - _inConditionalControlFlow = true; NullabilityNode trueGuard; NullabilityNode falseGuard; if (identical(_conditionInfo?.condition, node.condition)) { @@ -505,7 +582,9 @@ class EdgeBuilder extends GeneralizingAstVisitor _guards.add(trueGuard); } try { - node.thenStatement.accept(this); + // We branched, so create a new scope for post-dominators. + _postDominatedLocals.doScoped( + action: () => node.thenStatement.accept(this)); } finally { if (trueGuard != null) { _guards.removeLast(); @@ -515,7 +594,9 @@ class EdgeBuilder extends GeneralizingAstVisitor _guards.add(falseGuard); } try { - node.elseStatement?.accept(this); + // We branched, so create a new scope for post-dominators. + _postDominatedLocals.doScoped( + action: () => node.elseStatement?.accept(this)); } finally { if (falseGuard != null) { _guards.removeLast(); @@ -800,6 +881,12 @@ $stackTrace'''); } else { _handleAssignment(returnValue, returnType); } + + // Later statements no longer post-dominate the declarations because we + // exited (or, in parent scopes, conditionally exited). + // TODO(mfairhurst): don't clear post-dominators beyond the current function. + _postDominatedLocals.clearEachScope(); + return null; } @@ -962,6 +1049,17 @@ $stackTrace'''); } } } + _postDominatedLocals + .addAll(node.variables.map((variable) => variable.declaredElement)); + return null; + } + + @override + DecoratedType visitWhileStatement(WhileStatement node) { + // TODO do special condition handling + // TODO do create true/false guards? + _handleAssignment(node.condition, _notNullType); + _postDominatedLocals.doScoped(action: () => node.body.accept(this)); return null; } @@ -1112,8 +1210,7 @@ $stackTrace'''); _checkAssignment(expressionChecks, source: sourceType, destination: destinationType, - hard: _isVariableOrParameterReference(expression) && - !_inConditionalControlFlow); + hard: _postDominatedLocals.isReferenceInScope(expression)); return sourceType; } @@ -1147,7 +1244,8 @@ $stackTrace'''); returnType?.accept(this); parameters?.accept(this); _currentFunctionType = _variables.decoratedElementType(declaredElement); - _inConditionalControlFlow = false; + // Push a scope of post-dominated declarations on the stack. + _postDominatedLocals.pushScope(elements: declaredElement.parameters); try { initializers?.accept(this); body.accept(this); @@ -1231,6 +1329,7 @@ $stackTrace'''); } } finally { _currentFunctionType = null; + _postDominatedLocals.popScope(); } } @@ -1370,15 +1469,6 @@ $stackTrace'''); } else { return false; } - } - - bool _isVariableOrParameterReference(Expression expression) { - expression = expression.unParenthesized; - if (expression is SimpleIdentifier) { - var element = expression.staticElement; - if (element is LocalVariableElement) return true; - if (element is ParameterElement) return true; - } return false; } @@ -1572,6 +1662,9 @@ class _ConditionInfo { /// effect other than returning a boolean value. final bool isPure; + /// Indicates whether the intents postdominate the intent node declarations. + final bool postDominatingIntent; + /// If not `null`, the [NullabilityNode] that would need to be nullable in /// order for [condition] to evaluate to `true`. final NullabilityNode trueGuard; @@ -1581,7 +1674,7 @@ class _ConditionInfo { final NullabilityNode falseGuard; /// If not `null`, the [NullabilityNode] that should be asserted to have - // /// non-null intent if [condition] is asserted to be `true`. + /// non-null intent if [condition] is asserted to be `true`. final NullabilityNode trueDemonstratesNonNullIntent; /// If not `null`, the [NullabilityNode] that should be asserted to have @@ -1590,6 +1683,7 @@ class _ConditionInfo { _ConditionInfo(this.condition, {@required this.isPure, + this.postDominatingIntent, this.trueGuard, this.falseGuard, this.trueDemonstratesNonNullIntent, @@ -1598,8 +1692,31 @@ class _ConditionInfo { /// Returns a new [_ConditionInfo] describing the boolean "not" of `this`. _ConditionInfo not(Expression condition) => _ConditionInfo(condition, isPure: isPure, + postDominatingIntent: postDominatingIntent, trueGuard: falseGuard, falseGuard: trueGuard, trueDemonstratesNonNullIntent: falseDemonstratesNonNullIntent, falseDemonstratesNonNullIntent: trueDemonstratesNonNullIntent); } + +/// A [ScopedSet] specific to the [Element]s of locals/parameters. +/// +/// Contains helpers for dealing with expressions as if they were elements. +class _ScopedLocalSet extends ScopedSet { + bool isReferenceInScope(Expression expression) { + expression = expression.unParenthesized; + if (expression is SimpleIdentifier) { + var element = expression.staticElement; + return isInScope(element); + } + return false; + } + + void removeReferenceFromAllScopes(Expression expression) { + expression = expression.unParenthesized; + if (expression is SimpleIdentifier) { + var element = expression.staticElement; + removeFromAllScopes(element); + } + } +} diff --git a/pkg/nnbd_migration/lib/src/utilities/scoped_set.dart b/pkg/nnbd_migration/lib/src/utilities/scoped_set.dart new file mode 100644 index 000000000000..972f5900fe62 --- /dev/null +++ b/pkg/nnbd_migration/lib/src/utilities/scoped_set.dart @@ -0,0 +1,80 @@ +// Copyright (c) 2019, 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 'package:analyzer/dart/element/element.dart'; + +/// Tracks scopes of instances to be used by the [EdgeBuilder] for certain +/// analyses. +/// +/// For example, [EdgeBuilder] uses this to find post-dominating usages of +/// locals and parameters to decide where to insert hard edges. +/// +/// Currently does not implement Set because this has properties undesirable +/// of a Set, such as state that means different things at different times, and +/// some of the methods (such as `removeAll()`) may be ambiguous. However, it +/// may be reasonable to do so, carefully, in the future. +class ScopedSet { + /// The scope stack, where the last element is the current scope, and each + /// scope is a list of elements in that scope. + final _scopeStack = >[]; + + /// Get the current scope. Private so as not to expose to clients. + Set get _currentScope => _scopeStack.last; + + /// Add an element to the current scope (and not its parent scopes). + void add(T element) { + if (_scopeStack.isNotEmpty) { + _currentScope.add(element); + } + } + + /// Add many elements to the current scope (and not its parent scopes). + void addAll(Iterable elements) { + if (_scopeStack.isNotEmpty) { + _currentScope.addAll(elements); + } + } + + /// Clear each scope in the stack (the stack itself is not affected). + /// + /// This is useful in post-dominator analysis. Upon non-convergent branching, + /// all scopes of potentially post-dominated elements becomes empty. + void clearEachScope() => _scopeStack.forEach((scope) => scope.clear()); + + /// Create a scope like [pushScope], and use it to perform some [action] + /// before popping it. + void doScoped( + {List elements = const [], + bool copyCurrent: false, + void Function() action}) { + pushScope(elements: elements, copyCurrent: copyCurrent); + try { + action(); + } finally { + popScope(); + } + } + + /// Look up if the element is in the scope. + bool isInScope(T element) => + _scopeStack.isNotEmpty && _currentScope.contains(element); + + /// End the current scope. + void popScope() { + assert(_scopeStack.isNotEmpty); + _scopeStack.removeLast(); + } + + /// Begin a new scope, optionally with some known starting [elements], or + /// copying the current scope, as a starting state. + void pushScope({List elements = const [], bool copyCurrent: false}) => + _scopeStack.add({ + ...elements, + if (copyCurrent) ..._currentScope, + }); + + /// Remove element from the current scope and all containing scopes. + void removeFromAllScopes(T t) => + _scopeStack.forEach((scope) => scope.remove(t)); +} diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart index 96e6ff3b347e..44d2eac232b4 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -2225,6 +2225,72 @@ main() { await _checkSingleFileChanges(content, expected); } + test_unconditional_method_call_implies_non_null_intent_after_conditions() async { + var content = ''' +void g(bool b, int i1, int i2) { + int i3 = i1; + if (b) { + b; + } + i3.toDouble(); + int i4 = i2; + if (b) { + b; + return; + } + i4.toDouble(); +} +main() { + g(false, null, null); +} +'''; + var expected = ''' +void g(bool b, int i1, int? i2) { + int i3 = i1; + if (b) { + b; + } + i3.toDouble(); + int? i4 = i2; + if (b) { + b; + return; + } + i4!.toDouble(); +} +main() { + g(false, null!, null); +} +'''; + await _checkSingleFileChanges(content, expected); + } + + test_unconditional_method_call_implies_non_null_intent_in_condition() async { + var content = ''' +void g(bool b, int _i) { + if (b) { + int i = _i; + i.toDouble(); + } +} +main() { + g(false, null); +} +'''; + var expected = ''' +void g(bool b, int? _i) { + if (b) { + int i = _i!; + i.toDouble(); + } +} +main() { + g(false, null); +} +'''; + await _checkSingleFileChanges(content, expected); + } + test_unconditional_non_null_usage_implies_non_null_intent() async { var content = ''' void f(int i, int j) { diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart index 0da7fd08ad6d..cd1f159c892a 100644 --- a/pkg/nnbd_migration/test/edge_builder_test.dart +++ b/pkg/nnbd_migration/test/edge_builder_test.dart @@ -2053,6 +2053,24 @@ class C extends B { assertUnion(bReturnType.node, cReturnType.node); } + test_methodDeclaration_doesntAffect_unconditional_control_flow() async { + await analyze(''' +class C { + void f(bool b, int i, int j) { + assert(i != null); + if (b) {} + assert(j != null); + } + void g(int k) { + assert(k != null); + } +} +'''); + assertEdge(decoratedTypeAnnotation('int i').node, never, hard: true); + assertNoEdge(always, decoratedTypeAnnotation('int j').node); + assertEdge(decoratedTypeAnnotation('int k').node, never, hard: true); + } + test_methodDeclaration_resets_unconditional_control_flow() async { await analyze(''' class C { @@ -2468,6 +2486,445 @@ int f() { assertEdge(always, decoratedTypeAnnotation('int').node, hard: false)); } + test_postDominators_assert() async { + await analyze(''' +void test(bool b1, bool b2, bool b3, bool _b) { + assert(b1 != null); + if (_b) { + assert(b2 != null); + } + assert(b3 != null); +} +'''); + + assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true); + assertNoEdge(decoratedTypeAnnotation('bool b2').node, never); + assertEdge(decoratedTypeAnnotation('bool b3').node, never, hard: true); + } + + test_postDominators_break() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b1, C _c) { + while (b1/*check*/) { + bool b2 = b1; + C c = _c; + if (b2/*check*/) { + break; + } + c.m(); + } +} +'''); + + // TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('b1/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true)); + assertNullCheck(checkExpression('b2/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b2').node, never, hard: true)); + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: false)); + } + + test_postDominators_continue() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b1, C _c) { + while (b1/*check*/) { + bool b2 = b1; + C c = _c; + if (b2/*check*/) { + continue; + } + c.m(); + } +} +'''); + + // TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('b1/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true)); + assertNullCheck(checkExpression('b2/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b2').node, never, hard: true)); + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: false)); + } + + test_postDominators_doWhileStatement_conditional() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b, C c) { + do { + return; + } while(b/*check*/); + + c.m(); +} +'''); + + // TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('b/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: false)); + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: false)); + } + + test_postDominators_doWhileStatement_unconditional() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b, C c1, C c2) { + do { + C c3 = C(); + c1.m(); + c3.m(); + } while(b/*check*/); + + c2.m(); +} +'''); + + // TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('b/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true)); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: true)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: true)); + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: true)); + } + + test_postDominators_forInStatement_unconditional() async { + await analyze(''' +class C { + void m() {} +} +void test(List l, C c1, C c2) { + for (C c3 in l) { + c1.m(); + c3.m(); + } + + c2.m(); +} +'''); + + //TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('l/*check*/'), + // assertEdge(decoratedTypeAnnotation('List l').node, never, hard: true)); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: true)); + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: false)); + } + + test_postDominators_forStatement_unconditional() async { + await analyze(''' + +class C { + void m() {} +} +void test(bool b1, C c1, C c2, C c3) { + for (bool b2 = b1, b3 = b1; b1/*check*/ & b2/*check*/; c3.m()) { + c1.m(); + assert(b3 != null); + } + + c2.m(); +} +'''); + + //TODO(mfairhurst): enable this check + assertNullCheck(checkExpression('b1/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true)); + //assertNullCheck(checkExpression('b2/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b2').node, never, hard: true)); + //assertEdge(decoratedTypeAnnotation('b3 =').node, never, hard: false); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: true)); + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: false)); + } + + test_postDominators_ifStatement_conditional() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b, C c1, C c2) { + if (b/*check*/) { + C c3 = C(); + C c4 = C(); + c1.m(); + c3.m(); + + // Divergence breaks post-dominance. + return; + c4.m(); + + } + c2.m(); +} +'''); + + assertNullCheck(checkExpression('b/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true)); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: false)); + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: true)); + assertNullCheck(checkExpression('c4.m'), + assertEdge(decoratedTypeAnnotation('C c4').node, never, hard: false)); + } + + test_postDominators_ifStatement_unconditional() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b, C c1, C c2) { + if (b/*check*/) { + C c3 = C(); + C c4 = C(); + c1.m(); + c3.m(); + + // We ignore exceptions for post-dominance. + throw ''; + c4.m(); + + } + c2.m(); +} +'''); + + assertNullCheck(checkExpression('b/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true)); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: true)); + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: true)); + assertNullCheck(checkExpression('c4.m'), + assertEdge(decoratedTypeAnnotation('C c4').node, never, hard: true)); + } + + test_postDominators_inReturn_local() async { + await analyze(''' +class C { + int m() => 0; +} +int test(C c) { + return c.m(); +} +'''); + + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true)); + } + + test_postDominators_loopReturn() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b1, C _c) { + C c1 = _c; + while (b1/*check*/) { + bool b2 = b1; + C c2 = _c; + if (b2/*check*/) { + return; + } + c2.m(); + } + c1.m(); +} +'''); + + // TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('b1/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true)); + assertNullCheck(checkExpression('b2/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b2').node, never, hard: true)); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: false)); + } + + test_postDominators_reassign() async { + await analyze(''' +void test(bool b, int i1, int i2) { + i1 = null; + i1.toDouble(); + if (b) { + i2 = null; + } + i2.toDouble(); +} +'''); + + assertNullCheck(checkExpression('i1.toDouble'), + assertEdge(decoratedTypeAnnotation('int i1').node, never, hard: false)); + + assertNullCheck(checkExpression('i2.toDouble'), + assertEdge(decoratedTypeAnnotation('int i2').node, never, hard: false)); + } + + test_postDominators_shortCircuitOperators() async { + await analyze(''' +class C { + bool m() => true; +} +void test(C c1, C c2, C c3, C c4) { + c1.m() && c2.m(); + c3.m() || c4.m(); +} +'''); + + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: true)); + + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: true)); + + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: false)); + + assertNullCheck(checkExpression('c4.m'), + assertEdge(decoratedTypeAnnotation('C c4').node, never, hard: false)); + } + + @failingTest + test_postDominators_subFunction() async { + await analyze(''' +class C { + void m() {} +} +void test() { + (C c) { + c.m(); + }; +} +'''); + + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true)); + } + + @failingTest + test_postDominators_subFunction_ifStatement_conditional() async { + // Failing because function expressions aren't implemented + await analyze(''' +class C { + void m() {} +} +void test() { + (bool b, C c) { + if (b/*check*/) { + return; + } + c.m(); + }; +} +'''); + + assertNullCheck(checkExpression('b/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: false)); + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: false)); + } + + @failingTest + test_postDominators_subFunction_ifStatement_unconditional() async { + // Failing because function expressions aren't implemented + await analyze(''' +class C { + void m() {} +} +void test() { + (bool b, C c) { + if (b/*check*/) { + } + c.m(); + }; +} +'''); + + assertNullCheck(checkExpression('b/*check*/'), + assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true)); + assertNullCheck(checkExpression('c.m'), + assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true)); + } + + test_postDominators_ternaryOperator() async { + await analyze(''' +class C { + bool m() => true; +} +void test(C c1, C c2, C c3, C c4) { + c1.m() ? c2.m() : c3.m(); + + c4.m(); +} +'''); + + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: true)); + + assertNullCheck(checkExpression('c4.m'), + assertEdge(decoratedTypeAnnotation('C c4').node, never, hard: true)); + + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: false)); + + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: false)); + } + + test_postDominators_whileStatement_unconditional() async { + await analyze(''' +class C { + void m() {} +} +void test(bool b, C c1, C c2) { + while (b/*check*/) { + C c3 = C(); + c1.m(); + c3.m(); + } + + c2.m(); +} +'''); + + //TODO(mfairhurst): enable this check + //assertNullCheck(checkExpression('b/*check*/'), + // assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true)); + assertNullCheck(checkExpression('c1.m'), + assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false)); + assertNullCheck(checkExpression('c2.m'), + assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: true)); + assertNullCheck(checkExpression('c3.m'), + assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: true)); + } + test_postfixExpression_minusMinus() async { await analyze(''' int f(int i) { diff --git a/pkg/nnbd_migration/test/test_all.dart b/pkg/nnbd_migration/test/test_all.dart index e8b5c7280942..2272dea39aa9 100644 --- a/pkg/nnbd_migration/test/test_all.dart +++ b/pkg/nnbd_migration/test/test_all.dart @@ -10,6 +10,7 @@ import 'decorated_type_test.dart' as decorated_type_test; import 'edge_builder_test.dart' as edge_builder_test; import 'node_builder_test.dart' as node_builder_test; import 'nullability_node_test.dart' as nullability_node_test; +import 'utilities/test_all.dart' as utilities; main() { defineReflectiveSuite(() { @@ -19,5 +20,6 @@ main() { edge_builder_test.main(); node_builder_test.main(); nullability_node_test.main(); + utilities.main(); }); } diff --git a/pkg/nnbd_migration/test/utilities/scoped_set_test.dart b/pkg/nnbd_migration/test/utilities/scoped_set_test.dart new file mode 100644 index 000000000000..47a8661a1a01 --- /dev/null +++ b/pkg/nnbd_migration/test/utilities/scoped_set_test.dart @@ -0,0 +1,182 @@ +// Copyright (c) 2019, 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 'package:nnbd_migration/src/utilities/scoped_set.dart'; +import 'package:test/test.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +main() { + defineReflectiveSuite(() { + defineReflectiveTests(ScopedSetTest); + }); +} + +@reflectiveTest +class ScopedSetTest { + test_clearEachScope() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.pushScope(copyCurrent: true); + set.clearEachScope(); + expect(set.isInScope(0), false); + set.popScope(); + expect(set.isInScope(0), false); + } + + test_doScoped_actionPerformed() { + final set = ScopedSet(); + bool ran = false; + set.doScoped(action: () { + ran = true; + }); + expect(ran, true); + } + + test_doScoped_actionThrows() { + final set = ScopedSet(); + bool threw; + try { + set.doScoped(action: () { + set.add(0); + throw ''; + }); + } catch (_) { + threw = true; + } + + expect(threw, true); + expect(set.isInScope(0), false); + } + + test_doScoped_copyCurrent() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.doScoped( + copyCurrent: true, + action: () { + set.add(1); + expect(set.isInScope(0), true); + expect(set.isInScope(1), true); + }); + expect(set.isInScope(0), true); + expect(set.isInScope(1), false); + } + + test_doScoped_elements() { + final set = ScopedSet(); + set.pushScope(); + set.doScoped( + elements: [0, 1], + action: () { + expect(set.isInScope(0), true); + expect(set.isInScope(1), true); + }); + expect(set.isInScope(0), false); + expect(set.isInScope(1), false); + } + + test_doScoped_newScope() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.doScoped(action: () { + set.add(1); + expect(set.isInScope(0), false); + expect(set.isInScope(1), true); + }); + expect(set.isInScope(0), true); + expect(set.isInScope(1), false); + } + + test_initiallyEmpty() { + final set = ScopedSet(); + expect(set.isInScope(0), false); + expect(set.isInScope(1), false); + } + + test_popScope_copyCurrent() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.pushScope(copyCurrent: true); + set.popScope(); + expect(set.isInScope(0), true); + } + + test_popScope_element() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.pushScope(); + set.popScope(); + expect(set.isInScope(0), true); + } + + test_popScope_empty() { + final set = ScopedSet(); + set.pushScope(); + set.pushScope(); + set.add(0); + set.popScope(); + expect(set.isInScope(0), false); + } + + test_pushScope_add() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + expect(set.isInScope(0), true); + } + + test_pushScope_addAll() { + final set = ScopedSet(); + set.pushScope(); + set.addAll([0, 1]); + expect(set.isInScope(0), true); + expect(set.isInScope(1), true); + } + + test_pushScope_copyCurrent() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.pushScope(copyCurrent: true); + expect(set.isInScope(0), true); + } + + test_pushScope_empty() { + final set = ScopedSet(); + set.pushScope(); + expect(set.isInScope(0), false); + } + + test_pushScope_empty2() { + final set = ScopedSet(); + set.pushScope(); + set.add(0); + set.pushScope(); + expect(set.isInScope(0), false); + } + + test_pushScope_withElements() { + final set = ScopedSet(); + set.pushScope(elements: [0, 1]); + expect(set.isInScope(0), true); + expect(set.isInScope(1), true); + } + + test_removeFromAllScopes() { + final set = ScopedSet(); + set.pushScope(elements: [0, 1]); + set.pushScope(copyCurrent: true); + set.removeFromAllScopes(0); + expect(set.isInScope(0), false); + expect(set.isInScope(1), true); + set.popScope(); + expect(set.isInScope(0), false); + expect(set.isInScope(1), true); + } +} diff --git a/pkg/nnbd_migration/test/utilities/test_all.dart b/pkg/nnbd_migration/test/utilities/test_all.dart new file mode 100644 index 000000000000..7c14545f3f6e --- /dev/null +++ b/pkg/nnbd_migration/test/utilities/test_all.dart @@ -0,0 +1,13 @@ +// Copyright (c) 2019, 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 'package:test_reflective_loader/test_reflective_loader.dart'; + +import 'scoped_set_test.dart' as scoped_set_test; + +main() { + defineReflectiveSuite(() { + scoped_set_test.main(); + }); +}