[ddc, dart2js] Add results cache to isSubtype

Optimize repetitive calls to isSubtype with a caches to store pairwise
results.

There are currently two caches for sound and unsound results but in the
future that can be combined into a single cache once the library is
aware of error reporting. That single cache could stores "pass", "fail", 
or "fails when sound mode but passes in unsound null safety".

Issue: #48585
Change-Id: I49e5794703fd58f1b2bba50e426e25146800fbb8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323707
Reviewed-by: Mayank Patke <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>

Author: nshahan

pkg/compiler/lib/src/ssa/builder.dart

@@ -398,6 +398,9 @@ class KernelSsaGraphBuilder extends ir.Visitor<void> with ir.VisitorVoidMixin {
         return options.enableVariance;
       case 'LEGACY':
         return options.useLegacySubtyping;
+      case 'EXTRA_NULL_SAFETY_CHECKS':
+        // TODO(fishythefish): Handle this flag as needed.
+        return false;
       case 'PRINT_LEGACY_STARS':
         return options.printLegacyStars;
       default:

pkg/dev_compiler/lib/src/kernel/compiler.dart

@@ -6323,6 +6323,14 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
                   // checks. This allows DDC to produce optional warnings or
                   // errors when tests pass but would fail in sound null safety.
                   : runtimeCall('legacyTypeChecks');
+            case 'EXTRA_NULL_SAFETY_CHECKS':
+              return _options.soundNullSafety
+                  ? js.boolean(false)
+                  // When running the new runtime type system with weak null
+                  // safety this flag gets toggled when performing `is` and `as`
+                  // checks. This allows DDC to produce optional warnings or
+                  // errors when tests pass but would fail in sound null safety.
+                  : runtimeCall('extraNullSafetyChecks');
             case 'MINIFIED':
               return js.boolean(false);
             case 'NEW_RUNTIME_TYPES':

sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart

@@ -661,6 +661,56 @@ dindex(obj, index) => callMethod(obj, '_get', null, [index], null, '[]');
 dsetindex(obj, index, value) =>
     callMethod(obj, '_set', null, [index, value], null, '[]=');
 
+// TODO(nshahan): Cleanup the following `is`, `as`, and isSubtype
+// implementations.
+//
+// The logic is currently too convoluted but is temporary while the
+// implementation gets added directly to the dart:rti library and moved out of
+// the DDC only runtime library.
+//
+// These methods are dead code when running with sound null safety.
+//
+// Ideally each runtime should be able to set the compile time flag
+// `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')` and these helpers will no longer be
+// needed. The dart:rti library will know how to perform the checks and call a
+// method for the backend specific warning/error/logging.
+//
+// As currently written flow goes as follows:
+//   1) A type check (`is`, `as`, or isSubtype) is dispatched to the
+//      corresponding helper.
+//   2) The legacy stars are stripped out of the test type. This is expected to
+//      prove less impactful over time. Once all code has been migrated to ^2.12
+//      getting a legacy type on the RHS of a type test is much harder. Possibly
+//      only achievable through type checks involving type variables
+//      instantiated in constants.
+//   3) Flags are set manually to tell the dart:rti library how to perform the
+//      upcoming type check:
+//      - `extraNullSafetyChecks`: This is currently used to signal that if the type
+//        check reaches the full `isSubtype()` implementation, the legacy stars
+//        should be erased from the rti that is extracted from the value being
+//        tested.
+//      - `legacyTypeChecks`: This is currently used to signal that the test
+//        should be performed with sound semantics.
+//   4) Perform the sound type check using the legacy erased test type. This may
+//      fall into a fast path optimization for simple checks like
+//      `val is String?`. If the test involves a more complicated check it might
+//      trigger the extraction of the rti from the value being tested and passed
+//      to the full `isSubtype()` implementation. This uncertainty requires the
+//      two flags described above.
+//      - Note: `isSubtype()` uses two caches (sound and unsound) to speedup
+//        repeated checks if the results have already been determined. This
+//        could be reduced to a single cache when a reporting method is called
+//        from the dart:rti library directly. The single cache could store one
+//        of three values: "pass", "fail", and "disagree" (passes when unsound
+//        but would fail if sound).
+//   5) Reset the flags back to the default state.
+//   6) If the check passes then it will also pass in weak mode so simply return
+//      the result.
+//   7) Otherwise, perform the same type check in weak mode without erasing
+//      any legacy types.
+//   8) If the result disagrees with the sound mode check issue a warning.
+//   9) Return the result.
+
 /// General implementation of the Dart `is` operator.
 ///
 /// Some basic cases are handled directly by the `.is` methods that are attached
@@ -670,22 +720,33 @@ dsetindex(obj, index, value) =>
 @JSExportName('is')
 bool instanceOf(obj, type) {
   if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) {
-    var testRti = JS<rti.Rti>('!', '#', type);
     // When running without sound null safety is type tests are dispatched here
     // to issue optional warnings/errors when the result is true but would be
     // false with sound null safety.
-    var legacyErasedRecipe =
-        rti.Rti.getLegacyErasedRecipe(JS<rti.Rti>('!', '#', testRti));
-    var legacyErasedType = rti.findType(legacyErasedRecipe);
+    var testRti = JS<rti.Rti>('!', '#', type);
+    // TODO(nshahan): Move to isSubtype in dart:rti once all fast path checks
+    // have been updated to be aware of
+    // `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`.
+    rti.Rti legacyErasedType;
+    if (JS_GET_FLAG('PRINT_LEGACY_STARS')) {
+      // When preserving the legacy stars in the runtime type, avoid caching
+      // the version with erased types on the Rti.
+      var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(testRti);
+      legacyErasedType = rti.findType(legacyErasedRecipe);
+    } else {
+      legacyErasedType = rti.getLegacyErasedRti(testRti);
+    }
+    extraNullSafetyChecks = true;
     legacyTypeChecks = false;
     var result = JS<bool>('bool', '#.#(#)', legacyErasedType,
         JS_GET_NAME(JsGetName.RTI_FIELD_IS), obj);
+    extraNullSafetyChecks = false;
     legacyTypeChecks = true;
     if (result) return true;
     result =
         JS('bool', '#.#(#)', testRti, JS_GET_NAME(JsGetName.RTI_FIELD_IS), obj);
     if (result) {
-      // Type test passes put would fail with sound null safety.
+      // Type test returned true but would be false with sound null safety.
       var t1 = runtimeType(obj);
       var t2 = rti.createRuntimeType(testRti);
       _nullWarn('$t1 is not a subtype of $t2.');
@@ -713,18 +774,33 @@ cast(obj, type) {
     // optional warnings/errors when casts pass but would fail with sound null
     // safety.
     var testRti = JS<rti.Rti>('!', '#', type);
-    var objRti = JS<rti.Rti>('!', '#', getReifiedType(obj));
-    var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(testRti);
-    var legacyErasedType = rti.findType(legacyErasedRecipe);
+    if (obj == null && !rti.isNullable(testRti)) {
+      // Allow cast to pass but warn that it would fail in sound null safety.
+      _nullWarnOnType(type);
+      return obj;
+    }
+    // TODO(nshahan): Move to isSubtype in dart:rti once all fast path checks
+    // have been updated to be aware of
+    // `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`.
+    rti.Rti legacyErasedType;
+    if (JS_GET_FLAG('PRINT_LEGACY_STARS')) {
+      // When preserving the legacy stars in the runtime type, avoid caching
+      // the version with erased types on the Rti.
+      var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(testRti);
+      legacyErasedType = rti.findType(legacyErasedRecipe);
+    } else {
+      legacyErasedType = rti.getLegacyErasedRti(testRti);
+    }
+    extraNullSafetyChecks = true;
     legacyTypeChecks = false;
-    // Call `isSubtype()` to avoid throwing an error if it fails.
-    var result = rti.isSubtype(
-        JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), objRti, legacyErasedType);
+    var result = JS<bool>('!', '#.#(#)', legacyErasedType,
+        JS_GET_NAME(JsGetName.RTI_FIELD_IS), obj);
+    extraNullSafetyChecks = false;
     legacyTypeChecks = true;
     if (result) return obj;
     // Perform the actual cast and allow the error to be thrown if it fails.
     JS('', '#.#(#)', testRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), obj);
-    // Subtype check passes put would fail with sound null safety.
+    // Cast succeeds but would fail with sound null safety.
     var t1 = runtimeType(obj);
     var t2 = rti.createRuntimeType(testRti);
     _nullWarn('$t1 is not a subtype of $t2.');
@@ -756,17 +832,29 @@ bool _isSubtypeWithWarning(@notNull t1, @notNull t2) {
   if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) {
     var t1Rti = JS<rti.Rti>('!', '#', t1);
     var t2Rti = JS<rti.Rti>('!', '#', t2);
-    var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(t2Rti);
-    var legacyErasedType = rti.findType(legacyErasedRecipe);
+    // TODO(nshahan): Move to isSubtype in dart:rti once all fast path checks
+    // have been updated to be aware of
+    // `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`.
+    rti.Rti legacyErasedType;
+    if (JS_GET_FLAG('PRINT_LEGACY_STARS')) {
+      // When preserving the legacy stars in the runtime type, avoid caching
+      // the version with erased types on the Rti.
+      var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(t2Rti);
+      legacyErasedType = rti.findType(legacyErasedRecipe);
+    } else {
+      legacyErasedType = rti.getLegacyErasedRti(t2Rti);
+    }
+    extraNullSafetyChecks = true;
     legacyTypeChecks = false;
     var validSubtype = rti.isSubtype(
         JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), t1Rti, legacyErasedType);
+    extraNullSafetyChecks = false;
     legacyTypeChecks = true;
     if (validSubtype) return true;
     validSubtype =
         rti.isSubtype(JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), t1Rti, t2Rti);
     if (validSubtype) {
-      // Subtype check passes put would fail with sound null safety.
+      // Subtype check passes but would fail with sound null safety.
       _nullWarn('${rti.createRuntimeType(t1Rti)} '
           'is not a subtype of '
           '${rti.createRuntimeType(t2Rti)}.');

sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/runtime.dart

@@ -56,27 +56,24 @@ import 'dart:_js_shared_embedded_names';
 import 'dart:_rti' as rti
     show
         bindingRtiFromList,
-        createRuntimeType,
         constructorRtiCachePropertyName,
+        createRuntimeType,
         findType,
         getFunctionParametersForDynamicChecks,
         getGenericFunctionBounds,
+        getLegacyErasedRti,
+        getRecordTypeElementTypes,
+        getRecordTypeShapeKey,
         instanceType,
         instantiatedGenericFunctionType,
         interfaceTypeRecipePropertyName,
         isGenericFunctionType,
+        isNullable,
+        isRecordType,
         isSubtype,
         Rti,
-        _Type,
-        substitute,
         rtiToString,
-        isFunctionType,
-        isObjectType,
-        isRecordInterfaceType,
-        isRecordType,
-        getRecordTypeElementTypes,
-        getRecordTypeShapeKey,
-        getLibraryUri;
+        substitute;
 
 export 'dart:_debugger' show getDynamicStats, clearDynamicStats, trackCall;
 

sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart

@@ -25,6 +25,17 @@ _throwInvalidFlagError(String message) =>
 @notNull
 bool legacyTypeChecks = !compileTimeFlag("soundNullSafety");
 
+/// Signals if the next type check should be considered to to be sound when
+/// running without sound null safety.
+///
+/// The provides a way for this library to communicate that intent to the
+/// dart:rti library.
+///
+/// This flag gets inlined by the compiler in the place of
+/// `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`.
+@notNull
+bool extraNullSafetyChecks = false;
+
 @notNull
 bool _weakNullSafetyWarnings = false;
 

sdk/lib/_internal/js_shared/lib/rti.dart

@@ -150,8 +150,26 @@ class Rti {
     return future;
   }
 
-  Object? _precomputed2;
-  Object? _precomputed3;
+  // TODO(fishythefish): Replace with a single cache that stores one of three
+  // possible values.
+  Object? _isSubtypeCache;
+  Object? _unsoundIsSubtypeCache;
+
+  static Object _getIsSubtypeCache(Rti rti) {
+    if (JS_GET_FLAG('LEGACY')) {
+      // Read/write unsound cache field.
+      var probe = rti._unsoundIsSubtypeCache;
+      if (probe != null) return probe;
+      Object cache = JS('', 'new Map()');
+      rti._unsoundIsSubtypeCache = cache;
+      return cache;
+    }
+    var probe = rti._isSubtypeCache;
+    if (probe != null) return probe;
+    Object cache = JS('', 'new Map()');
+    rti._isSubtypeCache = cache;
+    return cache;
+  }
 
   /// If kind == kindFunction, stores an object used for checking function
   /// parameters in dynamic calls after the first use.
@@ -390,6 +408,17 @@ class Rti {
   }
 }
 
+// TODO(nshahan): Make private and change the argument type to rti once this
+// method is no longer called from outside the library.
+Rti getLegacyErasedRti(Object? rti) {
+  // When preserving the legacy stars in the runtime type no legacy erasure
+  // happens so the cached version cannot be used.
+  assert(!JS_GET_FLAG('PRINT_LEGACY_STARS'));
+  var originalType = _Utils.asRti(rti);
+  return Rti._getCachedRuntimeType(originalType)?._rti ??
+      _createAndCacheRuntimeType(originalType)._rti;
+}
+
 @pragma('dart2js:types:trust')
 @pragma('dart2js:index-bounds:trust')
 bool pairwiseIsTest(JSArray fieldRtis, JSArray values) {
@@ -3148,7 +3177,23 @@ class Variance {
 
 // Future entry point from compiled code.
 bool isSubtype(Object? universe, Rti s, Rti t) {
-  return _isSubtype(universe, s, null, t, null);
+  var sType = s;
+  // Erase any legacy types that may appear in the Object rti since those mask
+  // potential sound mode errors.
+  if (JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')) {
+    if (JS_GET_FLAG('PRINT_LEGACY_STARS')) {
+      var sLegacyErasedRecipe = Rti.getLegacyErasedRecipe(s);
+      sType = findType(sLegacyErasedRecipe);
+    } else {
+      sType = getLegacyErasedRti(s);
+    }
+  }
+  var sCache = Rti._getIsSubtypeCache(sType);
+  var probe = _Utils.mapGet(sCache, t);
+  if (probe != null) return _Utils.asBool(probe);
+  var result = _isSubtype(universe, sType, null, t, null);
+  _Utils.mapSet(sCache, t, result);
+  return result;
 }
 
 /// Based on

tests/dartdevc/weak_null_safety_errors_test.dart

@@ -8,10 +8,42 @@
 
 import 'package:expect/expect.dart';
 
+/// Code that runs without error when running with unsound null safety but
+/// should throw in sound mode or when running DDC with
+/// `--weak-null-safety-errors`.
+
+void fn(StringBuffer arg) {}
+void testArg<T>(T t) => t is T;
+void testReturn<T>(T Function() f) => throw 'do not call';
+
+const c = C<Duration>();
+
+class C<T> {
+  covariantCheck(List<T> t) {}
+  const C();
+}
+
 void main() {
   Expect.throwsTypeError(() => null as int);
   dynamic dynamicNull = null;
   Expect.throwsTypeError(() => fn(dynamicNull));
-}
 
-void fn(StringBuffer arg) {}
+  var l = [Duration(days: 1), null];
+  Expect.throwsTypeError(() => l as List<Duration>);
+
+  Expect.throwsTypeError(() => (testReturn<Duration?>) as Duration Function());
+
+  // Constants get legacy types introduced in their type arguments.
+  C<Duration?> c2 = c;
+  Expect.throwsTypeError(() => c2.covariantCheck([Duration(days: 1), null]));
+
+  // Tearoff instantiations are "potentially constant" and are treated as a
+  // constant by the CFE.
+  // When compiling for unsound null safety the resulting type signature
+  // attached to the tearoff is `void Function(Duration*)` which is a valid
+  // subtype of `void Function(Duration?)`. In sound null safety the signature
+  // is `void Function(Duration)` which should fail in the cast.
+  Expect.throwsTypeError(() => (testArg<Duration>) as void Function(Duration?));
+  Expect.throwsTypeError(
+      () => (testReturn<Duration>) as void Function(Duration? Function()));
+}