Migrator: Fix if-null promotion with GLB

Fixes https://github.com/dart-lang/sdk/issues/42375

EdgeBuilder wishes to do its own GLB type-joining, as does FlowAnalysis.
When FlowAnalysis pushes a promotion in ifNullExpression_rightBegin,
and then in write, and then pops those two promotions to join them in
ifNullExpression_end, EdgeBuilder is not given a chance to make its
own GLB node. Instead promotion never happens: the DecoratedType which
is the variable's type is promoted to non-nullable, for the left side.
The DecoratedType for the right side expression is not considered
non-nullable, so the FlowAnalysis.join will always fail to promote.

TypeOperations.adjustPromotedTypes solves this. It is a no-op in all
implementations except in EdgeBuilder.

Additionally FlowAnalysis.write's promotion fails for the same reason:
the only type of interest is the declared type made non-nullable, and
no matter what, the writtenType is not a subtype of this.

TypeOperations.forcePromotion solves this. It is a no-op in all
implementations except EdgeBuilder.

Change-Id: Iba6b9a69583af8fe1aa23124c7d66047acfff495
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/152104
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>

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

@@ -1662,6 +1662,16 @@ abstract class TypeOperations<Variable, Type> {
   /// consideration by an instance check.
   Type factor(Type from, Type what);
 
+  /// Whether the possible promotion from [from] to [to] should be forced, given
+  /// the current [promotedTypes], and [newPromotedTypes] resulting from
+  /// possible demotion.
+  ///
+  /// It is not expected that any implementation would override this except for
+  /// the migration engine.
+  bool forcePromotion(Type to, Type from, List<Type> promotedTypes,
+          List<Type> newPromotedTypes) =>
+      false;
+
   /// Return `true` if the [variable] is a local variable (not a formal
   /// parameter), and it has no declared type (no explicit type, and not
   /// initializer).
@@ -1679,6 +1689,15 @@ abstract class TypeOperations<Variable, Type> {
   /// `FutureOr<int?>`), so [type] may be returned even if it is nullable.
   Type /*!*/ promoteToNonNull(Type type);
 
+  /// Performs refinements on the [promotedTypes] chain which resulted in
+  /// intersecting [chain1] and [chain2].
+  ///
+  /// It is not expected that any implementation would override this except for
+  /// the migration engine.
+  List<Type> refinePromotedTypes(
+          List<Type> chain1, List<Type> chain2, List<Type> promotedTypes) =>
+      promotedTypes;
+
   /// Tries to promote to the first type from the second type, and returns the
   /// promoted type if it succeeds, otherwise null.
   Type tryPromoteToType(Type to, Type from);
@@ -1936,7 +1955,7 @@ class VariableModel<Variable, Type> {
   /// returns an updated promotion chain; otherwise returns [promotedTypes]
   /// unchanged.
   ///
-  /// Note that since promotions chains are considered immutable, if promotion
+  /// Note that since promotion chains are considered immutable, if promotion
   /// is required, a new promotion chain will be created and returned.
   List<Type> _tryPromoteToTypeOfInterest(
       TypeOperations<Variable, Type> typeOperations,
@@ -1945,6 +1964,11 @@ class VariableModel<Variable, Type> {
       Type writtenType) {
     assert(!writeCaptured);
 
+    if (typeOperations.forcePromotion(
+        writtenType, declaredType, this.promotedTypes, promotedTypes)) {
+      return _addToPromotedTypes(promotedTypes, writtenType);
+    }
+
     // Figure out if we have any promotion candidates (types that are a
     // supertype of writtenType and a proper subtype of the currently-promoted
     // type).  If at any point we find an exact match, we take it immediately.
@@ -2048,6 +2072,8 @@ class VariableModel<Variable, Type> {
       VariableModel<Variable, Type> second) {
     List<Type> newPromotedTypes = joinPromotedTypes(
         first.promotedTypes, second.promotedTypes, typeOperations);
+    newPromotedTypes = typeOperations.refinePromotedTypes(
+        first.promotedTypes, second.promotedTypes, newPromotedTypes);
     bool newAssigned = first.assigned && second.assigned;
     bool newUnassigned = first.unassigned && second.unassigned;
     bool newWriteCaptured = first.writeCaptured || second.writeCaptured;

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

@@ -3158,7 +3158,7 @@ class _Expression {
   String toString() => 'E$_id';
 }
 
-class _Harness implements TypeOperations<_Var, _Type> {
+class _Harness extends TypeOperations<_Var, _Type> {
   static const Map<String, bool> _coreSubtypes = const {
     'double <: Object': true,
     'double <: num': true,

pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart

@@ -345,7 +345,7 @@ class FlowAnalysisHelperForMigration extends FlowAnalysisHelper {
 }
 
 class TypeSystemTypeOperations
-    implements TypeOperations<PromotableElement, DartType> {
+    extends TypeOperations<PromotableElement, DartType> {
   final TypeSystemImpl typeSystem;
 
   TypeSystemTypeOperations(this.typeSystem);

pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart

@@ -258,8 +258,7 @@ class FlowAnalysisResult {
 }
 
 /// CFE-specific implementation of [TypeOperations].
-class TypeOperationsCfe
-    implements TypeOperations<VariableDeclaration, DartType> {
+class TypeOperationsCfe extends TypeOperations<VariableDeclaration, DartType> {
   final TypeEnvironment typeEnvironment;
 
   TypeOperationsCfe(this.typeEnvironment);

pkg/front_end/test/spell_checking_list_common.txt

@@ -1555,6 +1555,7 @@ interpreted
 interpreter
 interpreting
 intersect
+intersecting
 intersection
 intertwined
 into
@@ -2396,6 +2397,7 @@ referred
 refers
 refine
 refinement
+refinements
 reflecting
 regardless
 region

pkg/nnbd_migration/lib/src/decorated_type_operations.dart

@@ -6,6 +6,7 @@ import 'package:_fe_analyzer_shared/src/flow_analysis/flow_analysis.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type_system.dart';
 import 'package:nnbd_migration/src/decorated_type.dart';
+import 'package:nnbd_migration/src/edge_origin.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
 import 'package:nnbd_migration/src/variables.dart';
 
@@ -25,6 +26,29 @@ class DecoratedTypeOperations
     return from;
   }
 
+  @override
+  bool forcePromotion(DecoratedType to, DecoratedType from,
+      List<DecoratedType> promotedTypes, List<DecoratedType> newPromotedTypes) {
+    assert(to != null);
+    assert(from != null);
+    // Do not force promotion if it appears that the element's type was just
+    // demoted.
+    if (promotedTypes != null &&
+        (newPromotedTypes == null ||
+            newPromotedTypes.length < promotedTypes.length)) {
+      return false;
+    }
+    if (!isSubtypeOf(to, from)) {
+      return false;
+    }
+    var fromSources = from.node.upstreamEdges;
+    // Do not force promotion if [to] already points to [from].
+    if (fromSources.length == 1 && fromSources.single.sourceNode == to.node) {
+      return false;
+    }
+    return true;
+  }
+
   @override
   bool isLocalVariableWithoutDeclaredType(PromotableElement variable) {
     return variable is LocalVariableElement &&
@@ -58,6 +82,53 @@ class DecoratedTypeOperations
     return type.withNode(_graph.never);
   }
 
+  @override
+  // This function walks [chain1] and [chain2], creating an intersection similar
+  // to that of [VariableModel.joinPromotedTypes].
+  List<DecoratedType> refinePromotedTypes(List<DecoratedType> chain1,
+      List<DecoratedType> chain2, List<DecoratedType> promotedTypes) {
+    if (chain1 == null || chain2 == null) return promotedTypes;
+
+    // This method can only handle very simple joins.
+    if (chain1.length != chain2.length) return promotedTypes;
+
+    // The promotion chains were intersected without any promotions being
+    // dropped. There is nothing to do here.
+    if (promotedTypes != null && promotedTypes.length == chain1.length) {
+      return promotedTypes;
+    }
+
+    var result = <DecoratedType>[];
+    int index = 0;
+    while (index < chain1.length) {
+      if (!isSameType(chain1[index], chain2[index])) {
+        break;
+      }
+      result.add(chain1[index]);
+      index++;
+    }
+
+    if (index != chain1.length - 1) {
+      // This method can only handle the situation in which the promotion chains
+      // are identical up to the last node. If we are not in such situation,
+      // return the previous result.
+      return promotedTypes;
+    }
+
+    DecoratedType firstType = chain1[index];
+    DecoratedType secondType = chain2[index];
+
+    var node = NullabilityNode.forGLB();
+    var origin =
+        // TODO(srawlins): How to get the source or astNode from within here...
+        GreatestLowerBoundOrigin(null /* source */, null /* astNode */);
+    _graph.connect(firstType.node, node, origin, guards: [secondType.node]);
+    _graph.connect(node, firstType.node, origin);
+    _graph.connect(node, secondType.node, origin);
+
+    return result..add(firstType.withNode(node));
+  }
+
   @override
   DecoratedType tryPromoteToType(DecoratedType to, DecoratedType from) {
     // TODO(paulberry): implement appropriate logic for type variable promotion.

pkg/nnbd_migration/lib/src/edge_builder.dart

@@ -2177,6 +2177,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
         }
       }
       if (destinationLocalVariable != null) {
+        _dispatch(destinationExpression);
         destinationType = getOrComputeElementType(destinationLocalVariable);
       } else {
         destinationType = _dispatch(destinationExpression);

pkg/nnbd_migration/test/api_test.dart

@@ -5025,6 +5025,90 @@ main() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_promotion_conditional_variableRead() async {
+    var content = '''
+f({int i}) {
+  i = i == null ? 0 : i;
+  g(i);
+}
+
+g(int j) {}
+''';
+    var expected = '''
+f({int? i}) {
+  i = i == null ? 0 : i;
+  g(i);
+}
+
+g(int j) {}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  Future<void> test_promotion_ifNull_variableRead() async {
+    var content = '''
+f({int i}) {
+  i ??= 3;
+  g(i);
+}
+
+g(int j) {}
+''';
+    var expected = '''
+f({int? i}) {
+  i ??= 3;
+  g(i);
+}
+
+g(int j) {}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  Future<void> test_promotion_ifNull_variableRead_alreadyPromoted() async {
+    var content = '''
+f({num i}) {
+  if (i is int /*?*/) {
+    i ??= 3;
+    g(i);
+  }
+}
+
+g(int j) {}
+''';
+    var expected = '''
+f({num? i}) {
+  if (i is int?) {
+    i ??= 3;
+    g(i);
+  }
+}
+
+g(int j) {}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  Future<void> test_promotion_ifNull_variableRead_subType() async {
+    var content = '''
+f({num i}) {
+  i ??= 3;
+  g(i);
+}
+
+g(int j) {}
+''';
+    var expected = '''
+f({num? i}) {
+  i ??= 3;
+  g(i as int);
+}
+
+g(int j) {}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_promotion_preserves_complex_types() async {
     var content = '''
 int/*!*/ f(List<int/*?*/>/*?*/ x) {