[dart:js_interop] Make isUndefined and isNull throw on dart2wasm

null and undefined cannot be distinguished on dart2wasm in its
current state, so these helpers should only work on the JS
compilers. Some comments are updated to reflect the current state
of this internalization. Also fixes a pending TODO in isNull and
isUndefined on the JS backends.

CoreLibraryReviewExempt: Backend-specific library.
Change-Id: Ic56e8aa346af99cb99d01fe3c7ac5e37e965db23
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326690
Commit-Queue: Srujan Gaddam <srujzs@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>

CHANGELOG.md

@@ -162,6 +162,11 @@ constraint][language version] lower bound to 3.2 or greater (`sdk: '^3.2.0'`).
   of allowed types. Namely, this include the primitive types like `String`, JS
   types from `dart:js_interop`, and other static interop types (either through
   `@staticInterop` or extension types).
+- **Breaking Change on `dart:js_interop` `isNull` and `isUndefined`**:
+  `null` and `undefined` can only be discerned in the JS backends. dart2wasm
+  conflates the two values and treats them both as Dart null. Therefore, these
+  two helper methods should not be used on dart2wasm and will throw to avoid
+  potentially erroneous code.
 
 ### Tools
 

sdk/lib/_internal/js_shared/lib/js_interop_patch.dart

@@ -15,18 +15,15 @@ import 'dart:typed_data';
 JSObject get globalContext => staticInteropGlobalContext as JSObject;
 
 /// Helper for working with the [JSAny?] top type in a backend agnostic way.
-/// TODO(joshualitt): Remove conflation of null and undefined after migration.
 @patch
 extension NullableUndefineableJSAnyExtension on JSAny? {
   @patch
   @pragma('dart2js:prefer-inline')
-  bool get isUndefined =>
-      this == null || js_util.typeofEquals(this, 'undefined');
+  bool get isUndefined => js_util.typeofEquals(this, 'undefined');
 
   @patch
   @pragma('dart2js:prefer-inline')
-  bool get isNull =>
-      this == null || foreign_helper.JS('bool', '# === null', this);
+  bool get isNull => foreign_helper.JS('bool', '# === null', this);
 
   @patch
   @pragma('dart2js:prefer-inline')

sdk/lib/_internal/wasm/lib/js_interop_patch.dart

@@ -29,10 +29,16 @@ extension NullableUndefineableJSAnyExtension on JSAny? {
   // reified `JSUndefined` and `JSNull`, we have to handle the case where
   // `this == null`. However, after migration we can remove these checks.
   @patch
-  bool get isUndefined => this == null || isJSUndefined(this?.toExternRef);
+  bool get isUndefined =>
+      throw UnimplementedError("JS 'null' and 'undefined' are internalized as "
+          "Dart null in dart2wasm. As such, they can not be differentiated and "
+          "this API should not be used when compiling to Wasm.");
 
   @patch
-  bool get isNull => this == null || this!.toExternRef.isNull;
+  bool get isNull =>
+      throw UnimplementedError("JS 'null' and 'undefined' are internalized as "
+          "Dart null in dart2wasm. As such, they can not be differentiated and "
+          "this API should not be used when compiling to Wasm.");
 
   @patch
   JSBoolean typeofEquals(JSString type) =>

sdk/lib/js_interop/js_interop.dart

@@ -147,19 +147,36 @@ typedef JSBigInt = js_types.JSBigInt;
 /// lowering.
 external JSObject get globalContext;
 
-/// `JSUndefined` and `JSNull` are actual reified types on some backends, but
-/// not others. Instead, users should use nullable types for any type that could
-/// contain `JSUndefined` or `JSNull`. However, instead of trying to determine
-/// the nullability of a JS type in Dart, i.e. using `?`, `!`, `!= null` or `==
-/// null`, users should use the provided helpers below to determine if it is
-/// safe to downcast a potentially `JSNullable` or `JSUndefineable` object to a
-/// defined and non-null JS type.
-// TODO(joshualitt): Investigate whether or not it will be possible to reify
-// `JSUndefined` and `JSNull` on all backends.
+/// JS `undefined` and JS `null` are internalized differently based on the
+/// backends. In the JS backends, Dart `null` can actually be JS `undefined` or
+/// JS `null`. In dart2wasm, that's not the case: there's only one Wasm value
+/// `null` can be. Therefore, when we get back JS `null` or JS `undefined`, we
+/// internalize both as Dart `null` in dart2wasm, and when we pass Dart `null`
+/// to an interop API, we pass JS `null`. In the JS backends, Dart `null`
+/// retains its original value when passed back to an interop API. Be wary of
+/// writing code where this distinction between `null` and `undefined` matters.
+// TODO(srujzs): Investigate what it takes to allow users to distinguish between
+// the two "nullish" values. An annotation-based model where users annotate
+// interop APIs to internalize `undefined` differently seems promising, but does
+// not handle some cases like converting a `JSArray` with `undefined`s in it to
+// `List<JSAny?>`. In this case, the implementation of the list wrapper needs to
+// make the decision, not the user.
 extension NullableUndefineableJSAnyExtension on JSAny? {
+  /// Determine if this value corresponds to JS `undefined`.
+  ///
+  /// **WARNING**: Currently, there isn't a way to distinguish between JS
+  /// `undefined` and JS `null` in dart2wasm. As such, this should only be used
+  /// for code that compiles to JS and will throw on dart2wasm.
   external bool get isUndefined;
+
+  /// Determine if this value corresponds to JS `null`.
+  ///
+  /// **WARNING**: Currently, there isn't a way to distinguish between JS
+  /// `undefined` and JS `null` in dart2wasm. As such, this should only be used
+  /// for code that compiles to JS and will throw on dart2wasm.
   external bool get isNull;
-  bool get isUndefinedOrNull => isUndefined || isNull;
+
+  bool get isUndefinedOrNull => this == null;
   bool get isDefinedAndNotNull => !isUndefinedOrNull;
   external JSBoolean typeofEquals(JSString typeString);
 

tests/lib/js/static_interop_test/js_types_test.dart

@@ -12,6 +12,8 @@ import 'dart:typed_data';
 import 'package:expect/expect.dart';
 import 'package:expect/minitest.dart';
 
+const isJSBackend = const bool.fromEnvironment('dart.library.html');
+
 @JS()
 external void eval(String code);
 
@@ -298,21 +300,30 @@ void syncTests() {
   expect(bigInt.toStringExternal(), '9876543210000000000000123456789');
 
   // null and undefined can flow into `JSAny?`.
-  // TODO(joshualitt): Fix tests when `JSNull` and `JSUndefined` are no longer
-  // conflated.
-  expect(nullAny.isNull, true);
-  //expect(nullAny.isUndefined, false);
-  expect(nullAny.isUndefined, true);
+  // TODO(srujzs): Remove the `isJSBackend` checks when `JSNull` and
+  // `JSUndefined` can be distinguished on dart2wasm.
+  if (isJSBackend) {
+    expect(nullAny.isNull, true);
+    expect(nullAny.isUndefined, false);
+  }
+  expect(nullAny, null);
+  expect(nullAny.isUndefinedOrNull, true);
   expect(nullAny.isDefinedAndNotNull, false);
   expect(typeofEquals(nullAny, 'object'), true);
-  //expect(undefinedAny.isNull, false);
-  expect(undefinedAny.isNull, true);
-  expect(undefinedAny.isUndefined, true);
+  if (isJSBackend) {
+    expect(undefinedAny.isNull, false);
+    expect(undefinedAny.isUndefined, true);
+  }
+  expect(undefinedAny.isUndefinedOrNull, true);
   expect(undefinedAny.isDefinedAndNotNull, false);
-  //expect(typeofEquals(undefinedAny, 'undefined'), true);
-  //expect(typeofEquals(undefinedAny, 'object'), true);
-  expect(definedNonNullAny.isNull, false);
-  expect(definedNonNullAny.isUndefined, false);
+  if (isJSBackend) {
+    expect(typeofEquals(undefinedAny, 'undefined'), true);
+    expect(definedNonNullAny.isNull, false);
+    expect(definedNonNullAny.isUndefined, false);
+  } else {
+    expect(typeofEquals(undefinedAny, 'object'), true);
+  }
+  expect(definedNonNullAny.isUndefinedOrNull, false);
   expect(definedNonNullAny.isDefinedAndNotNull, true);
   expect(typeofEquals(definedNonNullAny, 'object'), true);
 }