[flow analysis] Do not promote to mutual subtypes.

Previously, flow analysis had the rule that type promotion only
occurred when the type being tested was a subtype of the previously
promoted type (or the declared type, if there was no previous
promotion). This led to counterintuitive behaviors when the type being
tested and the previously promoted type were mutual subtypes (see
dart-lang/language#4368).

With this change, the rule is updated so that type promotion only
occurs when the type being tested is a subtype of the previously
promoted type _and_ the previously promoted type is _not_ a subtype of
the type being tested. The user-visible difference is that promotion
to a mutual subtype no longer occurs.

This change makes flow analysis easier to reason about, and improves
its behavior in corner cases, but I believe it will have minimal
impact on real-world code. But to reduce the risk to existing code,
the change only takes effect when the `sound-flow-analysis` language
feature is enabled.

Fixes dart-lang/language#4368.

Bug: dart-lang/language#4368
Change-Id: I30dab017e043e75603d618df721c8a2683667cd5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429200
Reviewed-by: Johnni Winther <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Paul Berry <[email protected]>

Author: stereotype441

pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart

@@ -2669,7 +2669,7 @@ class FlowModel<Type extends Object> {
         // In all of these cases, the correct thing to do is to keep all
         // promotions that were done in both the `try` and `finally` blocks.
         newPromotedTypes = PromotionModel.rebasePromotedTypes(
-          helper.typeOperations,
+          helper,
           thisModel.promotedTypes,
           afterFinallyModel.promotedTypes,
         );
@@ -2950,7 +2950,7 @@ class FlowModel<Type extends Object> {
         // usual "promotion chain" invariant (each promoted type is a subtype of
         // the previous).
         newPromotedTypes = PromotionModel.rebasePromotedTypes(
-          helper.typeOperations,
+          helper,
           thisModel.promotedTypes,
           baseModel.promotedTypes,
         );
@@ -3028,13 +3028,15 @@ class FlowModel<Type extends Object> {
 
     Type previousType = reference._type;
     Type newType = helper.typeOperations.promoteToNonNull(previousType);
-    if (newType == previousType) {
+    if (!helper.isValidPromotionStep(
+      previousType: previousType,
+      newType: newType,
+    )) {
       return new ExpressionInfo<Type>.trivial(
         model: this,
         type: helper.boolType,
       );
     }
-    assert(helper.typeOperations.isSubtypeOf(newType, previousType));
 
     FlowModel<Type> ifTrue = _finishTypeTest(
       helper,
@@ -3075,14 +3077,14 @@ class FlowModel<Type extends Object> {
 
     Type previousType = reference._type;
     Type? newType = helper.typeOperations.tryPromoteToType(type, previousType);
-    if (newType == null || newType == previousType) {
+    if (newType == null ||
+        !helper.isValidPromotionStep(
+          previousType: previousType,
+          newType: newType,
+        )) {
       return this;
     }
 
-    assert(
-      helper.typeOperations.isSubtypeOf(newType, previousType),
-      "Expected $newType to be a subtype of $previousType.",
-    );
     return _finishTypeTest(helper, reference, info, type, newType);
   }
 
@@ -3118,11 +3120,11 @@ class FlowModel<Type extends Object> {
       type,
       previousType,
     );
-    if (typeIfSuccess != null && typeIfSuccess != previousType) {
-      assert(
-        helper.typeOperations.isSubtypeOf(typeIfSuccess, previousType),
-        "Expected $typeIfSuccess to be a subtype of $previousType.",
-      );
+    if (typeIfSuccess != null &&
+        helper.isValidPromotionStep(
+          previousType: previousType,
+          newType: typeIfSuccess,
+        )) {
       ifTrue = _finishTypeTest(helper, reference, info, type, typeIfSuccess);
     }
 
@@ -3132,8 +3134,11 @@ class FlowModel<Type extends Object> {
       // Promoting to `Never` would mark the code as unreachable.  But it might
       // be reachable due to mixed mode unsoundness.  So don't promote.
       typeIfFalse = null;
-    } else if (factoredType == previousType) {
-      // No change to the type, so don't promote.
+    } else if (!helper.isValidPromotionStep(
+      previousType: previousType,
+      newType: factoredType,
+    )) {
+      // Don't promote.
       typeIfFalse = null;
     } else {
       typeIfFalse = factoredType;
@@ -3198,19 +3203,18 @@ class FlowModel<Type extends Object> {
     NonPromotionReason? nonPromotionReason,
     int variableKey,
     Type writtenType,
-    SsaNode<Type> newSsaNode,
-    FlowAnalysisOperations<Variable, Type> operations, {
+    SsaNode<Type> newSsaNode, {
     bool promoteToTypeOfInterest = true,
     required Type unpromotedType,
   }) {
     FlowModel<Type>? newModel;
     PromotionModel<Type>? infoForVar = promotionInfo?.get(helper, variableKey);
     if (infoForVar != null) {
       PromotionModel<Type> newInfoForVar = infoForVar.write(
+        helper,
         nonPromotionReason,
         variableKey,
         writtenType,
-        operations,
         newSsaNode,
         promoteToTypeOfInterest: promoteToTypeOfInterest,
         unpromotedType: unpromotedType,
@@ -3386,6 +3390,15 @@ mixin FlowModelHelper<Type extends Object> {
   /// Whether the variable of [variableKey] was declared with the `final`
   /// modifier and the `inference-update-4` feature flag is enabled.
   bool isFinal(int variableKey);
+
+  /// Determines whether a promotion from type [previousType] to [newType] is
+  /// allowed to occur, given the current configuration of flow analysis.
+  ///
+  /// Caller is required to ensure that `newType <: previousType`.
+  bool isValidPromotionStep({
+    required Type previousType,
+    required Type newType,
+  });
 }
 
 /// Documentation links that might be presented to the user to accompany a "why
@@ -3717,10 +3730,10 @@ class PromotionModel<Type extends Object> {
   /// must pass in a non-null value for [nonPromotionReason] describing the
   /// reason for any potential demotion.
   PromotionModel<Type> write<Variable extends Object>(
+    FlowModelHelper<Type> helper,
     NonPromotionReason? nonPromotionReason,
     int variableKey,
     Type writtenType,
-    FlowAnalysisOperations<Variable, Type> operations,
     SsaNode<Type> newSsaNode, {
     required bool promoteToTypeOfInterest,
     required Type unpromotedType,
@@ -3737,14 +3750,14 @@ class PromotionModel<Type extends Object> {
 
     _DemotionResult<Type> demotionResult = _demoteViaAssignment(
       writtenType,
-      operations,
+      helper.typeOperations,
       nonPromotionReason,
     );
     List<Type>? newPromotedTypes = demotionResult.promotedTypes;
 
     if (promoteToTypeOfInterest) {
       newPromotedTypes = _tryPromoteToTypeOfInterest(
-        operations,
+        helper,
         unpromotedType,
         newPromotedTypes,
         writtenType,
@@ -3858,7 +3871,7 @@ class PromotionModel<Type extends Object> {
   /// Note that since promotion chains are considered immutable, if promotion
   /// is required, a new promotion chain will be created and returned.
   List<Type>? _tryPromoteToTypeOfInterest(
-    FlowAnalysisTypeOperations<Type> typeOperations,
+    FlowModelHelper<Type> helper,
     Type declaredType,
     List<Type>? promotedTypes,
     Type writtenType,
@@ -3875,15 +3888,18 @@ class PromotionModel<Type extends Object> {
 
     void handleTypeOfInterest(Type type) {
       // The written type must be a subtype of the type.
-      if (!typeOperations.isSubtypeOf(writtenType, type)) {
+      if (!helper.typeOperations.isSubtypeOf(writtenType, type)) {
         return;
       }
 
       // Must be more specific that the currently promoted type.
-      if (type == currentlyPromotedType) {
+      if (!helper.typeOperations.isSubtypeOf(type, currentlyPromotedType)) {
         return;
       }
-      if (!typeOperations.isSubtypeOf(type, currentlyPromotedType)) {
+      if (!helper.isValidPromotionStep(
+        previousType: currentlyPromotedType,
+        newType: type,
+      )) {
         return;
       }
 
@@ -3906,7 +3922,9 @@ class PromotionModel<Type extends Object> {
 
     // The declared type is always a type of interest, but we never promote
     // to the declared type. So, try NonNull of it.
-    Type declaredTypeNonNull = typeOperations.promoteToNonNull(declaredType);
+    Type declaredTypeNonNull = helper.typeOperations.promoteToNonNull(
+      declaredType,
+    );
     if (declaredTypeNonNull != declaredType) {
       handleTypeOfInterest(declaredTypeNonNull);
       if (result != null) {
@@ -3922,7 +3940,7 @@ class PromotionModel<Type extends Object> {
         return result!;
       }
 
-      Type typeNonNull = typeOperations.promoteToNonNull(type);
+      Type typeNonNull = helper.typeOperations.promoteToNonNull(type);
       if (typeNonNull != type) {
         handleTypeOfInterest(typeNonNull);
         if (result != null) {
@@ -3940,7 +3958,10 @@ class PromotionModel<Type extends Object> {
       for (int i = 0; i < candidates2.length; i++) {
         for (int j = 0; j < candidates2.length; j++) {
           if (j == i) continue;
-          if (!typeOperations.isSubtypeOf(candidates2[i], candidates2[j])) {
+          if (!helper.typeOperations.isSubtypeOf(
+            candidates2[i],
+            candidates2[j],
+          )) {
             // Not a subtype of all the others.
             continue outer;
           }
@@ -4127,7 +4148,7 @@ class PromotionModel<Type extends Object> {
   /// [thisPromotedTypes] or [basePromotedTypes] (to make it easier for the
   /// caller to detect when data structures may be re-used).
   static List<Type>? rebasePromotedTypes<Type extends Object>(
-    FlowAnalysisTypeOperations<Type> typeOperations,
+    FlowModelHelper<Type> helper,
     List<Type>? thisPromotedTypes,
     List<Type>? basePromotedTypes,
   ) {
@@ -4147,8 +4168,11 @@ class PromotionModel<Type extends Object> {
       Type otherPromotedType = basePromotedTypes.last;
       for (int i = 0; i < thisPromotedTypes.length; i++) {
         Type nextType = thisPromotedTypes[i];
-        if (typeOperations.isSubtypeOf(nextType, otherPromotedType) &&
-            nextType != otherPromotedType) {
+        if (helper.typeOperations.isSubtypeOf(nextType, otherPromotedType) &&
+            helper.isValidPromotionStep(
+              previousType: otherPromotedType,
+              newType: nextType,
+            )) {
           newPromotedTypes =
               basePromotedTypes.toList()..addAll(thisPromotedTypes.skip(i));
           break;
@@ -4582,7 +4606,7 @@ class SsaNode<Type extends Object> {
       }
       List<Type>? newPromotedTypes = newModel.promotedTypes;
       List<Type>? rebasedPromotedTypes = PromotionModel.rebasePromotedTypes(
-        helper.typeOperations,
+        helper,
         afterFinallyPromotedTypes,
         newPromotedTypes,
       );
@@ -5772,6 +5796,27 @@ class _FlowAnalysisImpl<
         true;
   }
 
+  @override
+  bool isValidPromotionStep({
+    required Type previousType,
+    required Type newType,
+  }) {
+    // Caller must ensure that `newType <: previousType`.
+    assert(
+      typeOperations.isSubtypeOf(newType, previousType),
+      "Expected $newType to be a subtype of $previousType.",
+    );
+    if (this.typeAnalyzerOptions.soundFlowAnalysisEnabled) {
+      // Promotion to a mutual subtype is not allowed. Since the caller has
+      // already ensured that `newType <: previousType`, it's only necessary to
+      // check whether `previousType <: newType`.
+      return !typeOperations.isSubtypeOf(previousType, newType);
+    } else {
+      // Repeated promotion to the same type is not allowed.
+      return newType != previousType;
+    }
+  }
+
   @override
   void labeledStatement_begin(Statement node) {
     _current = _current.split();
@@ -7126,7 +7171,6 @@ class _FlowAnalysisImpl<
       promotionKey,
       matchedType,
       newSsaNode,
-      operations,
       promoteToTypeOfInterest: !isImplicitlyTyped && !isFinal,
       unpromotedType: unpromotedType,
     );
@@ -7394,7 +7438,6 @@ class _FlowAnalysisImpl<
       variableKey,
       writtenType,
       newSsaNode,
-      operations,
       unpromotedType: unpromotedType,
     );
 

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_mini_ast.dart

@@ -48,6 +48,22 @@ class FlowAnalysisTestHarness extends Harness
     if (variable != null && operations.isFinal(variable)) return true;
     return false;
   }
+
+  @override
+  bool isValidPromotionStep({
+    required SharedTypeView previousType,
+    required SharedTypeView newType,
+  }) {
+    // Caller must ensure that `newType <: previousType`.
+    assert(
+      typeOperations.isSubtypeOf(newType, previousType),
+      "Expected $newType to be a subtype of $previousType.",
+    );
+    // Promotion to a mutual subtype is not allowed. Since the caller has
+    // already ensured that `newType <: previousType`, it's only necessary to
+    // check whether `previousType <: newType`.
+    return !typeOperations.isSubtypeOf(previousType, newType);
+  }
 }
 
 /// Helper class allowing tests to examine the values of variables' SSA nodes.