[dart2js] Fix valid/invalid refinement logic.

The issue here was that isInMask does not hold inversely. The key is that we want to verify which refinements to skip, therefore we want to calculate *in*valid refinements. We want to skip refines where the new type is a Union AND the old tpye is a supertype of the new type. after.isInMask(before) gives us that but !before.isInMask(after) does not.

I've renamed isValidRefinement to isInvalidRefinement to more accurately capture this distinction.

Change-Id: I0d99479357a140095a5d0dfb7e2f987556097891
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312160
Commit-Queue: Nate Biggs <[email protected]>
Reviewed-by: Mayank Patke <[email protected]>

Author: natebiggs

pkg/compiler/lib/src/inferrer/abstract_value_domain.dart

@@ -660,7 +660,7 @@ abstract class AbstractValueDomain {
   /// Serializes this [value] for this domain to [sink].
   void writeAbstractValueToDataSink(DataSinkWriter sink, AbstractValue? value);
 
-  bool isValidRefinement(AbstractValue before, AbstractValue after);
+  bool isInvalidRefinement(AbstractValue before, AbstractValue after);
 
   void finalizeMetrics() {}
 

pkg/compiler/lib/src/inferrer/computable.dart

@@ -629,9 +629,9 @@ class ComputableAbstractValueDomain with AbstractValueDomain {
   }
 
   @override
-  bool isValidRefinement(covariant ComputableAbstractValue before,
+  bool isInvalidRefinement(covariant ComputableAbstractValue before,
       covariant ComputableAbstractValue after) {
-    return _wrappedDomain.isValidRefinement(_unwrap(before), _unwrap(after));
+    return _wrappedDomain.isInvalidRefinement(_unwrap(before), _unwrap(after));
   }
 
   @override

pkg/compiler/lib/src/inferrer/engine.dart

@@ -2,6 +2,7 @@
 // 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:collection/collection.dart';
 import 'package:kernel/ast.dart' as ir;
 
 import '../../compiler_api.dart' as api;
@@ -480,60 +481,66 @@ class InferrerEngine {
     _processCalledTargets(shouldMarkCalled: true);
 
     if (debug.PRINT_SUMMARY) {
-      types.allocatedLists.values.forEach((_info) {
+      void printSorted(Iterable<String> toSort) {
+        print(toSort.sorted((a, b) => a.compareTo(b)).join('\n'));
+      }
+
+      printSorted(types.allocatedLists.values.map((_info) {
         ListTypeInformation info = _info;
-        print('${info.type} '
+        return '${info.type} '
             'for ${abstractValueDomain.getAllocationNode(info.originalType)} '
             'at ${abstractValueDomain.getAllocationElement(info.originalType)}'
-            'after ${info.refineCount}');
-      });
-      types.allocatedSets.values.forEach((_info) {
+            'after ${info.refineCount}';
+      }));
+      printSorted(types.allocatedSets.values.map((_info) {
         SetTypeInformation info = _info;
-        print('${info.type} '
+        return ('${info.type} '
             'for ${abstractValueDomain.getAllocationNode(info.originalType)} '
             'at ${abstractValueDomain.getAllocationElement(info.originalType)} '
             'after ${info.refineCount}');
-      });
-      types.allocatedMaps.values.forEach((_info) {
+      }));
+      printSorted(types.allocatedMaps.values.map((_info) {
         MapTypeInformation info = _info;
-        print('${info.type} '
+        return '${info.type} '
             'for ${abstractValueDomain.getAllocationNode(info.originalType)} '
             'at ${abstractValueDomain.getAllocationElement(info.originalType)}'
-            'after ${info.refineCount}');
-      });
-      types.allocatedClosures.forEach((TypeInformation info) {
+            'after ${info.refineCount}';
+      }));
+      printSorted(types.allocatedClosures.map((TypeInformation info) {
         if (info is ElementTypeInformation) {
-          print('${info.getInferredSignature(types)} for '
-              '${info.debugName}');
+          return '${info.getInferredSignature(types)} for '
+              '${info.debugName}';
         } else if (info is ClosureTypeInformation) {
-          print('${info.getInferredSignature(types)} for '
-              '${info.debugName}');
+          return '${info.getInferredSignature(types)} for '
+              '${info.debugName}';
         } else if (info is DynamicCallSiteTypeInformation) {
+          var str = '';
           if (info.hasClosureCallTargets) {
-            print('<Closure.call>');
+            str += '<Closure.call>';
           }
           info.forEachConcreteTarget(memberHierarchyBuilder, (member) {
             if (member is FunctionEntity) {
-              print('${types.getInferredSignatureOfMethod(member)} '
-                  'for ${member}');
+              str += '${types.getInferredSignatureOfMethod(member)} '
+                  'for ${member}';
             } else {
-              print('${types.getInferredTypeOfMember(member).type} '
-                  'for ${member}');
+              str += '${types.getInferredTypeOfMember(member).type} '
+                  'for ${member}';
             }
             return true;
           });
+          return str;
         } else if (info is StaticCallSiteTypeInformation) {
           final cls = info.calledElement.enclosingClass!;
           final callMethod = _lookupCallMethod(cls)!;
-          print('${types.getInferredSignatureOfMethod(callMethod)} for ${cls}');
+          return '${types.getInferredSignatureOfMethod(callMethod)} for ${cls}';
         } else {
-          print('${info.type} for some unknown kind of closure');
+          return '${info.type} for some unknown kind of closure';
         }
-      });
-      _analyzedElements.forEach((MemberEntity elem) {
+      }));
+      printSorted(_analyzedElements.map((MemberEntity elem) {
         TypeInformation type = types.getInferredTypeOfMember(elem);
-        print('${elem} :: ${type} from ${type.inputs} ');
-      });
+        return '${elem} :: ${type} from ${type.inputs} ';
+      }));
     }
     dump?.afterAnalysis();
 
@@ -779,7 +786,9 @@ class InferrerEngine {
       // Check that refinement has not accidentally changed the type.
       assert(oldType == info.type);
       if (info.abandonInferencing) info.doNotEnqueue = true;
-      if ((info.type = newType) != oldType) {
+      final invalidRefine =
+          abstractValueDomain.isInvalidRefinement(oldType, newType);
+      if (!invalidRefine && (info.type = newType) != oldType) {
         _overallRefineCount++;
         info.incrementRefineCount();
         if (info.refineCount > _MAX_CHANGE_COUNT) {

pkg/compiler/lib/src/inferrer/powersets/powersets.dart

@@ -848,9 +848,9 @@ class PowersetDomain with AbstractValueDomain {
           receiver.abstractValue, selector, memberHierarchyBuilder);
 
   @override
-  bool isValidRefinement(
+  bool isInvalidRefinement(
       covariant PowersetValue before, covariant PowersetValue after) {
-    return _abstractValueDomain.isValidRefinement(
+    return _abstractValueDomain.isInvalidRefinement(
         before._abstractValue, after._abstractValue);
   }
 }

pkg/compiler/lib/src/inferrer/trivial.dart

@@ -393,7 +393,7 @@ class TrivialAbstractValueDomain with AbstractValueDomain {
       const [];
 
   @override
-  bool isValidRefinement(AbstractValue before, AbstractValue after) => true;
+  bool isInvalidRefinement(AbstractValue before, AbstractValue after) => true;
 
   @override
   AbstractValue get asyncStarStreamType => const TrivialAbstractValue();

pkg/compiler/lib/src/inferrer/typemasks/masks.dart

@@ -993,7 +993,8 @@ class CommonMasks with AbstractValueDomain {
   }
 
   @override
-  bool isValidRefinement(covariant TypeMask before, covariant TypeMask after) {
+  bool isInvalidRefinement(
+      covariant TypeMask before, covariant TypeMask after) {
     // Consider a typegraph node which simply outputs the union of its inputs,
     // and suppose such a node has K inputs with types: A, B, C, and D. The
     // union would flatten to a common supertype, e.g. `Object`. However, a
@@ -1014,7 +1015,13 @@ class CommonMasks with AbstractValueDomain {
     // (i.e. omit UnionTypeMask type check). But since it is only the behavior
     // of UnionTypeMask that can lead to a narrowing, we save work by only doing
     // the subtype check when a UnionTypeMask is involved.
-    return after is! UnionTypeMask || before.isInMask(after, _closedWorld);
+    //
+    // Note: [UnionTypeMask.isInMask] can return false negatives. We have to
+    // ensure we take the conservative action when we get a false negative and
+    // treat those as valid refinements. Invoking `after.isInMask` instead of
+    // `!before.isInMask` ensures that the false negatives are handled
+    // correctly.
+    return after is UnionTypeMask && after.isInMask(before, _closedWorld);
   }
 
   @override

pkg/compiler/lib/src/inferrer/wrapped.dart

@@ -641,9 +641,9 @@ class WrappedAbstractValueDomain with AbstractValueDomain {
           receiver._abstractValue, selector, memberHierarchyBuilder);
 
   @override
-  bool isValidRefinement(covariant WrappedAbstractValue before,
+  bool isInvalidRefinement(covariant WrappedAbstractValue before,
       covariant WrappedAbstractValue after) {
-    return _abstractValueDomain.isValidRefinement(
+    return _abstractValueDomain.isInvalidRefinement(
         before._abstractValue, after._abstractValue);
   }
 }