Flow analysis: don't use == to compare types.

stereotype441 · commit-bot@chromium.org · commit c8587ce1999e · 2019-07-21T20:33:55.000Z
Includes an integration test illustrating where this would have caused incorrect behavior (due to the fact that the analyzer's `DartType.==` doesn't yet distinguish nullabilities - see #37587), as well as unit tests that will ensure we don't regress even after #37587 is fixed. Change-Id: Ief92cf5a052bc2b96e4e74f69f2268b0565d920f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109820 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
diff --git a/pkg/analyzer/lib/src/dart/resolver/flow_analysis.dart b/pkg/analyzer/lib/src/dart/resolver/flow_analysis.dart
@@ -655,6 +655,9 @@ abstract class TypeOperations<Variable, Type> {
   /// Return `true` if the [variable] is a local variable, not a parameter.
   bool isLocalVariable(Variable variable);
 
+  /// Returns `true` if [type1] and [type2] are the same type.
+  bool isSameType(Type type1, Type type2);
+
   /// Return `true` if the [leftType] is a subtype of the [rightType].
   bool isSubtypeOf(Type leftType, Type rightType);
 
@@ -733,7 +736,7 @@ class _State<Variable, Type> {
     previousType ??= typeOperations.variableType(variable);
 
     if (typeOperations.isSubtypeOf(type, previousType) &&
-        type != previousType) {
+        !typeOperations.isSameType(type, previousType)) {
       var newPromoted = <Variable, Type>{}..addAll(promoted);
       newPromoted[variable] = type;
       return _State<Variable, Type>(
diff --git a/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart b/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart
@@ -463,6 +463,16 @@ class _TypeSystemTypeOperations
     return element is LocalVariableElement;
   }
 
+  @override
+  bool isSameType(covariant TypeImpl type1, covariant TypeImpl type2) {
+    if (type1.nullabilitySuffix != type2.nullabilitySuffix) {
+      // TODO(paulberry): after DartType.operator== has been updated to compare
+      // nullabilities, this if test can be dropped.  See dartbug.com/37587.
+      return false;
+    }
+    return type1 == type2;
+  }
+
   @override
   bool isSubtypeOf(DartType leftType, DartType rightType) {
     return typeSystem.isSubtypeOf(leftType, rightType);
@@ -471,15 +481,7 @@ class _TypeSystemTypeOperations
   @override
   DartType tryPromoteToNonNull(covariant TypeImpl type) {
     TypeImpl promotedType = typeSystem.promoteToNonNull(type);
-    if (promotedType.nullabilitySuffix != type.nullabilitySuffix) {
-      // TODO(paulberry): after DartType.operator== has been updated to compare
-      // nullabilities, this if test can be dropped.  See dartbug.com/37587.
-      return promotedType;
-    }
-    if (promotedType != type) {
-      return promotedType;
-    }
-    return null;
+    return isSameType(type, promotedType) ? null : promotedType;
   }
 
   @override
diff --git a/pkg/analyzer/test/src/dart/resolution/flow_analysis_test.dart b/pkg/analyzer/test/src/dart/resolution/flow_analysis_test.dart
@@ -193,6 +193,18 @@ void f(int? x) {
 ''');
   }
 
+  test_is_promotes_nullability() async {
+    await trackCode(r'''
+void f(int? x) {
+  if (x is int) {
+    /*nonNullable*/ x;
+  } else {
+    x;
+  }
+}
+''');
+  }
+
   test_method_if_then_else() async {
     await trackCode(r'''
 class C {
diff --git a/pkg/analyzer/test/src/dart/resolution/flow_analysis_unit_test.dart b/pkg/analyzer/test/src/dart/resolution/flow_analysis_unit_test.dart
@@ -47,6 +47,37 @@ main() {
       expect(flow.promotedType(x).type, 'int');
       flow.verifyStackEmpty();
     });
+
+    void _checkIs(String declaredType, String tryPromoteType,
+        String expectedPromotedType) {
+      var flow = _Harness().flow;
+      var x = _Var('x', _Type(declaredType));
+      flow.add(x, assigned: true);
+      var expr = _Expression();
+      flow.isExpression_end(expr, x, false, _Type(tryPromoteType));
+      flow.ifStatement_thenBegin(expr);
+      if (expectedPromotedType == null) {
+        expect(flow.promotedType(x), isNull);
+      } else {
+        expect(flow.promotedType(x).type, 'int');
+      }
+      flow.ifStatement_elseBegin();
+      expect(flow.promotedType(x), isNull);
+      flow.ifStatement_end(true);
+      flow.verifyStackEmpty();
+    }
+
+    test('isExpression_end promotes to a subtype', () {
+      _checkIs('int?', 'int', 'int');
+    });
+
+    test('isExpression_end does not promote to a supertype', () {
+      _checkIs('int', 'int?', null);
+    });
+
+    test('isExpression_end does not promote to an unrelated type', () {
+      _checkIs('int', 'String', null);
+    });
   });
 
   group('join', () {
@@ -146,6 +177,11 @@ class _Harness
     throw UnimplementedError('TODO(paulberry)');
   }
 
+  @override
+  bool isSameType(_Type type1, _Type type2) {
+    return type1.type == type2.type;
+  }
+
   @override
   bool isSubtypeOf(_Type leftType, _Type rightType) {
     const Map<String, bool> _subtypes = const {
diff --git a/pkg/nnbd_migration/lib/src/decorated_type_operations.dart b/pkg/nnbd_migration/lib/src/decorated_type_operations.dart
@@ -21,6 +21,11 @@ class DecoratedTypeOperations
     return element is LocalVariableElement;
   }
 
+  @override
+  bool isSameType(DecoratedType type1, DecoratedType type2) {
+    throw new UnimplementedError('TODO(paulberry)');
+  }
+
   @override
   bool isSubtypeOf(DecoratedType leftType, DecoratedType rightType) {
     return _typeSystem.isSubtypeOf(leftType.type, rightType.type);
