[dart2js] Add --interop-null-assertions.

This CL adds a new --interop-null-assertions flag (cf.
--native-null-assertions) with the goal of validating that JS interop
APIs with non-nullable static return types do not return null values.
This flag is currently disabled by default but is intended to assist in
sound null safety migrations.

In general, we don't guarantee type soundness of package:js interop
since users can write incorrect static types which are not backed by
any runtime checks. However, it is likely that during the null safety
migration, some interop APIs which should have been made nullable
weren't. Therefore, we want to offer some additional (but limited)
checking for this case.

For static invocations of functions with non-nullable return types, we
can simply perform a null check on the result of the call.
For instance methods (which could be invoked virtually/dynamically), we
want to perform a null check on the return value in the callee (the
interceptor method) itself when possible.

It's possible for multiple interop bindings to share the same
interceptor method. We produce a null check in the interceptor method
body if all the methods have non-nullable return types. Otherwise, we
insert checks at callsites when appropriate.

Change-Id: Ifd155d7f8326152b6d57d61199e0b7973c4a1211
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/369784
Reviewed-by: Sigmund Cherem <[email protected]>
Commit-Queue: Mayank Patke <[email protected]>

pkg/compiler/lib/src/commandline_options.dart

@@ -89,6 +89,9 @@ class Flags {
   static const String nativeNullAssertions = '--native-null-assertions';
   static const String noNativeNullAssertions = '--no-native-null-assertions';
 
+  static const String interopNullAssertions = '--interop-null-assertions';
+  static const String noInteropNullAssertions = '--no-interop-null-assertions';
+
   static const String noSourceMaps = '--no-source-maps';
 
   static const String omitLateNames = '--omit-late-names';

pkg/compiler/lib/src/common/elements.dart

@@ -11,6 +11,7 @@ import '../elements/entities.dart';
 import '../elements/names.dart';
 import '../elements/types.dart';
 import '../inferrer/abstract_value_domain.dart';
+import '../ir/element_map.dart';
 import '../js_backend/native_data.dart' show NativeBasicData;
 import '../js_model/locals.dart';
 import '../universe/selector.dart' show Selector;
@@ -863,6 +864,9 @@ abstract class CommonElements {
   FunctionEntity _findRtiFunction(String name) =>
       _findLibraryMember(rtiLibrary, name)!;
 
+  late final FunctionEntity interopNullAssertion =
+      _findRtiFunction('_interopNullAssertion');
+
   late final FunctionEntity setArrayType = _findRtiFunction('_setArrayType');
 
   late final FunctionEntity findType = _findRtiFunction('findType');
@@ -1279,6 +1283,8 @@ class JCommonElements extends CommonElements {
 // interface, the first should only be used during resolution and the latter in
 // both resolution and codegen.
 abstract class ElementEnvironment {
+  IrToElementMap get elementMap;
+
   /// Returns the main library for the compilation.
   LibraryEntity? get mainLibrary;
 

pkg/compiler/lib/src/dart2js.dart

@@ -452,6 +452,8 @@ Future<api.CompilationResult> compile(List<String> argv,
     _OneOption(Flags.enableNullAssertions, passThrough),
     _OneOption(Flags.nativeNullAssertions, passThrough),
     _OneOption(Flags.noNativeNullAssertions, passThrough),
+    _OneOption(Flags.interopNullAssertions, passThrough),
+    _OneOption(Flags.noInteropNullAssertions, passThrough),
     _OneOption(Flags.trustTypeAnnotations, setTrustTypeAnnotations),
     _OneOption(Flags.trustPrimitives, passThrough),
     _OneOption(Flags.trustJSInteropTypeAnnotations, ignoreOption),

pkg/compiler/lib/src/js_backend/backend_impact.dart

@@ -546,6 +546,7 @@ class BackendImpacts {
   // TODO(sra): Split into refined impacts.
   late final BackendImpact newRtiImpact = BackendImpact(
     staticUses: [
+      if (_options.interopNullAssertions) _commonElements.interopNullAssertion,
       _commonElements.findType,
       _commonElements.instanceType,
       _commonElements.arrayInstanceType,

pkg/compiler/lib/src/js_backend/native_data.dart

@@ -9,11 +9,15 @@ import 'package:kernel/ast.dart' as ir;
 import '../common.dart';
 import '../common/elements.dart' show ElementEnvironment;
 import '../elements/entities.dart';
+import '../elements/names.dart';
+import '../elements/types.dart';
 import '../ir/annotations.dart';
 import '../js_model/js_to_frontend_map.dart' show identity, JsToFrontendMap;
 import '../kernel/element_map.dart';
 import '../native/behavior.dart' show NativeBehavior;
 import '../serialization/serialization.dart';
+import '../universe/call_structure.dart';
+import '../universe/selector.dart';
 import '../util/util.dart';
 
 class NativeBasicDataBuilder {
@@ -438,12 +442,93 @@ class NativeDataBuilder {
   }
 
   /// Closes this builder and creates the resulting [NativeData] object.
-  NativeData close() => NativeData(
-      _nativeBasicData,
-      _nativeMemberName,
-      _nativeMethodBehavior,
-      _nativeFieldLoadBehavior,
-      _nativeFieldStoreBehavior);
+  NativeData close(DiagnosticReporter reporter) {
+    final data = NativeData(
+        _nativeBasicData,
+        _nativeMemberName,
+        _nativeMethodBehavior,
+        _nativeFieldLoadBehavior,
+        _nativeFieldStoreBehavior, {});
+
+    if (reporter.options.interopNullAssertions) {
+      // We can enforce the return type nullability of an interop API in two
+      // ways: by putting the null check on the invocation in the caller, or by
+      // putting the null check on the return value in the callee body
+      // (generally an interceptor method). It is only safe to do the latter if
+      // all interop bindings that share the interceptor method have consistent
+      // nullabilities.
+
+      final environment = _nativeBasicData._env;
+      final dartTypes = environment.elementMap.commonElements.dartTypes;
+
+      bool returnTypeIsNonNullable(FunctionEntity member,
+          {required bool callthrough}) {
+        final memberType = environment.getFunctionType(member);
+        final functionType =
+            callthrough ? memberType.returnType as FunctionType : memberType;
+        return dartTypes.isNonNullableIfSound(functionType.returnType);
+      }
+
+      // Intercepted methods keyed by selector.
+      final jsNameMap = <Selector, List<FunctionEntity>>{};
+      for (final (member as FunctionEntity)
+          in _nativeBasicData._jsInteropMembers.keys) {
+        if (!member.isInstanceMember) continue;
+        if (!member.isFunction && !member.isGetter) continue;
+
+        // The program builder uses the unescaped name for interceptor methods.
+        // We can only perform null checks in the interceptor method body if all
+        // the intercepted methods with the same name have consistent return
+        // type nullabilities.
+        // We use a public name because the interceptor will not distinguish
+        // methods from different libraries, even if they have a leading
+        // underscore.
+        final name =
+            PublicName(data.computeUnescapedJSInteropName(member.name!));
+
+        void addAllPossibleInvocations(FunctionType type) {
+          final requiredPositionalCount = type.parameterTypes.length;
+          final optionalPositionalCount = type.optionalParameterTypes.length;
+          // We do not yet know which invocations are actually live in the
+          // program, so we conservatively allow for any number of optional
+          // arguments to be passed. Named parameters are not supported.
+          for (var i = 0; i <= optionalPositionalCount; i++) {
+            (jsNameMap[Selector.call(name,
+                    CallStructure.unnamed(requiredPositionalCount + i))] ??= [])
+                .add(member);
+          }
+        }
+
+        if (member.isGetter) {
+          (jsNameMap[Selector.getter(name)] ??= []).add(member);
+          final returnType = environment.getFunctionType(member).returnType;
+          if (returnType is FunctionType) {
+            addAllPossibleInvocations(returnType);
+          }
+        } else if (member.isFunction) {
+          final functionType = environment.getFunctionType(member);
+          addAllPossibleInvocations(functionType);
+        }
+      }
+
+      jsNameMap.forEach((selector, members) {
+        final canCheckInCallee = members.every((FunctionEntity member) =>
+            returnTypeIsNonNullable(member,
+                callthrough:
+                    member.isGetter && selector.kind == SelectorKind.CALL));
+        data.interopNullChecks[selector] = canCheckInCallee
+            ? InteropNullCheckKind.calleeCheck
+            : InteropNullCheckKind.callerCheck;
+      });
+    }
+
+    return data;
+  }
+}
+
+enum InteropNullCheckKind {
+  calleeCheck,
+  callerCheck,
 }
 
 /// Additional element information for native classes and methods and js-interop
@@ -475,12 +560,17 @@ class NativeData implements NativeBasicData {
   /// Cache for [NativeBehavior]s for writing to native fields.
   final Map<MemberEntity, NativeBehavior> _nativeFieldStoreBehavior;
 
+  /// A map from selectors for interop members to the type of null check
+  /// required when `--interop-null-assertions` is passed.
+  final Map<Selector, InteropNullCheckKind> interopNullChecks;
+
   NativeData(
       this._nativeBasicData,
       this._nativeMemberName,
       this._nativeMethodBehavior,
       this._nativeFieldLoadBehavior,
-      this._nativeFieldStoreBehavior);
+      this._nativeFieldStoreBehavior,
+      this.interopNullChecks);
 
   factory NativeData.fromIr(KernelToElementMap map, IrAnnotationData data) {
     NativeBasicData nativeBasicData = NativeBasicData.fromIr(map, data);
@@ -517,7 +607,7 @@ class NativeData implements NativeBasicData {
     });
 
     return NativeData(nativeBasicData, nativeMemberName, nativeMethodBehavior,
-        nativeFieldLoadBehavior, nativeFieldStoreBehavior);
+        nativeFieldLoadBehavior, nativeFieldStoreBehavior, const {});
   }
 
   /// Deserializes a [NativeData] object from [source].
@@ -537,9 +627,11 @@ class NativeData implements NativeBasicData {
     Map<MemberEntity, NativeBehavior> nativeFieldStoreBehavior =
         source.readMemberMap(
             (MemberEntity member) => NativeBehavior.readFromDataSource(source));
+    Map<Selector, InteropNullCheckKind> interopNullChecks = source
+        .readSelectorMap((_) => source.readEnum(InteropNullCheckKind.values));
     source.end(tag);
     return NativeData(nativeBasicData, nativeMemberName, nativeMethodBehavior,
-        nativeFieldLoadBehavior, nativeFieldStoreBehavior);
+        nativeFieldLoadBehavior, nativeFieldStoreBehavior, interopNullChecks);
   }
 
   /// Serializes this [NativeData] to [sink].
@@ -565,6 +657,9 @@ class NativeData implements NativeBasicData {
       behavior.writeToDataSink(sink);
     });
 
+    sink.writeSelectorMap(
+        interopNullChecks, sink.writeEnum<InteropNullCheckKind>);
+
     sink.end(tag);
   }
 
@@ -846,7 +941,7 @@ class NativeData implements NativeBasicData {
     Map<MemberEntity, NativeBehavior> nativeFieldStoreBehavior = map
         .toBackendMemberMap(_nativeFieldStoreBehavior, _convertNativeBehavior);
     return NativeData(nativeBasicData, nativeMemberName, nativeMethodBehavior,
-        nativeFieldLoadBehavior, nativeFieldStoreBehavior);
+        nativeFieldLoadBehavior, nativeFieldStoreBehavior, interopNullChecks);
   }
 }
 

pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart

@@ -426,6 +426,9 @@ class ProgramBuilder {
     interceptorClass?.isChecks.addAll(_jsInteropIsChecks);
     interceptorTypeData?.classChecks.addAll(_jsInteropTypeChecks);
 
+    late final interopNullAssert = _task.emitter
+        .staticFunctionAccess(_commonElements.interopNullAssertion);
+
     Set<String> stubNames = {};
     librariesMap.forEach((LibraryEntity library,
         List<ClassEntity> classElements, _memberElement, _typeElement) {
@@ -443,9 +446,14 @@ class ProgramBuilder {
                 for (Selector selector in selectors) {
                   js.Name stubName = _namer.invocationName(selector);
                   if (stubNames.add(stubName.key)) {
-                    interceptorClass!.callStubs.add(_buildStubMethod(stubName,
-                        js.js('function(obj) { return obj.# }', [jsName]),
-                        element: member));
+                    final code = _options.interopNullAssertions &&
+                            _nativeData.interopNullChecks[selector] ==
+                                InteropNullCheckKind.calleeCheck
+                        ? js.js('function(obj) { return #(obj.#) }',
+                            [interopNullAssert, jsName])
+                        : js.js('function(obj) { return obj.# }', [jsName]);
+                    interceptorClass!.callStubs
+                        .add(_buildStubMethod(stubName, code, element: member));
                   }
                 }
               }
@@ -511,11 +519,16 @@ class ProgramBuilder {
                   // functions. The behavior of this solution matches JavaScript
                   // behavior implicitly binding this only when JavaScript
                   // would.
-                  interceptorClass!.callStubs.add(_buildStubMethod(
-                      stubName,
-                      js.js('function(receiver, #) { return receiver.#(#) }',
-                          [parameters, jsName, parameters]),
-                      element: member));
+                  final code = _options.interopNullAssertions &&
+                          _nativeData.interopNullChecks[selector] ==
+                              InteropNullCheckKind.calleeCheck
+                      ? js.js(
+                          'function(receiver, #) { return #(receiver.#(#)) }',
+                          [parameters, interopNullAssert, jsName, parameters])
+                      : js.js('function(receiver, #) { return receiver.#(#) }',
+                          [parameters, jsName, parameters]);
+                  interceptorClass!.callStubs
+                      .add(_buildStubMethod(stubName, code, element: member));
                 }
               }
             }

pkg/compiler/lib/src/js_model/element_map_impl.dart

@@ -2074,6 +2074,7 @@ class JsKernelToElementMap implements JsToElementMap, IrToElementMap {
 
 class JsElementEnvironment extends ElementEnvironment
     implements JElementEnvironment {
+  @override
   final JsKernelToElementMap elementMap;
 
   JsElementEnvironment(this.elementMap);

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

@@ -1619,6 +1619,7 @@ class KernelToElementMap implements IrToElementMap {
 
 class KernelElementEnvironment extends ElementEnvironment
     implements KElementEnvironment {
+  @override
   final KernelToElementMap elementMap;
 
   KernelElementEnvironment(this.elementMap);

pkg/compiler/lib/src/kernel/transformations/global/js_get_flag_lowering.dart

@@ -51,28 +51,18 @@ class JsGetFlagLowering {
       ..fileOffset = node.fileOffset;
   }
 
-  bool? _getFlagValue(String flagName) {
-    switch (flagName) {
-      case 'DEV_COMPILER':
-        return false;
-      case 'MINIFIED':
-        return _options.enableMinification;
-      case 'MUST_RETAIN_METADATA':
-        return false;
-      case 'USE_CONTENT_SECURITY_POLICY':
-        return _options.features.useContentSecurityPolicy.isEnabled;
-      case 'VARIANCE':
-        return _options.enableVariance;
-      case 'LEGACY':
-        return _options.useLegacySubtyping;
-      case 'EXTRA_NULL_SAFETY_CHECKS':
-        return _options.experimentNullSafetyChecks;
-      case 'PRINT_LEGACY_STARS':
-        return _options.printLegacyStars;
-      default:
-        return null;
-    }
-  }
+  bool? _getFlagValue(String flagName) => switch (flagName) {
+        'DEV_COMPILER' => false,
+        'MINIFIED' => _options.enableMinification,
+        'MUST_RETAIN_METADATA' => false,
+        'USE_CONTENT_SECURITY_POLICY' =>
+          _options.features.useContentSecurityPolicy.isEnabled,
+        'VARIANCE' => _options.enableVariance,
+        'LEGACY' => _options.useLegacySubtyping,
+        'EXTRA_NULL_SAFETY_CHECKS' => _options.experimentNullSafetyChecks,
+        'PRINT_LEGACY_STARS' => _options.printLegacyStars,
+        _ => null,
+      };
 
   Never _unsupportedFlag(Object? flag) =>
       throw UnsupportedError('Unexpected JS_GET_FLAG argument: $flag');

pkg/compiler/lib/src/options.dart

@@ -508,6 +508,11 @@ class CompilerOptions implements DiagnosticOptions {
   bool nativeNullAssertions = false;
   bool _noNativeNullAssertions = false;
 
+  /// Whether to generate code asserting that return values of JS-interop APIs
+  /// with non-nullable return types are not null.
+  bool interopNullAssertions = false;
+  bool _noInteropNullAssertions = false;
+
   /// Whether to generate a source-map file together with the output program.
   bool generateSourceMap = true;
 
@@ -868,6 +873,9 @@ class CompilerOptions implements DiagnosticOptions {
       ..nativeNullAssertions = _hasOption(options, Flags.nativeNullAssertions)
       .._noNativeNullAssertions =
           _hasOption(options, Flags.noNativeNullAssertions)
+      ..interopNullAssertions = _hasOption(options, Flags.interopNullAssertions)
+      .._noInteropNullAssertions =
+          _hasOption(options, Flags.noInteropNullAssertions)
       ..experimentalTrackAllocations =
           _hasOption(options, Flags.experimentalTrackAllocations)
       ..experimentStartupFunctions =
@@ -984,13 +992,19 @@ class CompilerOptions implements DiagnosticOptions {
       throw ArgumentError("Missing required ${Flags.platformBinaries}");
     }
     if (_soundNullSafety && _noSoundNullSafety) {
-      throw ArgumentError("'${Flags.soundNullSafety}' incompatible with "
+      throw ArgumentError("'${Flags.soundNullSafety}' is incompatible with "
           "'${Flags.noSoundNullSafety}'");
     }
     if (nativeNullAssertions && _noNativeNullAssertions) {
-      throw ArgumentError("'${Flags.nativeNullAssertions}' incompatible with "
+      throw ArgumentError(
+          "'${Flags.nativeNullAssertions}' is incompatible with "
           "'${Flags.noNativeNullAssertions}'");
     }
+    if (interopNullAssertions && _noInteropNullAssertions) {
+      throw ArgumentError(
+          "'${Flags.interopNullAssertions}' is incompatible with "
+          "'${Flags.noInteropNullAssertions}'");
+    }
     if (nullSafetyMode == NullSafetyMode.sound && experimentNullSafetyChecks) {
       throw ArgumentError('${Flags.experimentNullSafetyChecks} is incompatible '
           'with sound null safety.');
@@ -1081,6 +1095,10 @@ class CompilerOptions implements DiagnosticOptions {
       nativeNullAssertions = true;
     }
 
+    if (_noInteropNullAssertions) {
+      interopNullAssertions = false;
+    }
+
     if (_mergeFragmentsThreshold != null) {
       mergeFragmentsThreshold = _mergeFragmentsThreshold;
     }

pkg/compiler/lib/src/serialization/serialization.dart

@@ -23,6 +23,7 @@ import '../js_model/elements.dart';
 import '../js_model/locals.dart';
 import '../js_model/type_recipe.dart' show TypeRecipe;
 import '../universe/record_shape.dart' show RecordShape;
+import '../universe/selector.dart';
 
 import '../options.dart';
 import 'deferrable.dart';