From 71cffed55cfb088e202da1927bb50ec769d8d1eb Mon Sep 17 00:00:00 2001 From: Joshua Litt Date: Fri, 17 Feb 2023 18:20:59 +0000 Subject: [PATCH] [js_types] Add Helpers to work with `JSUndefined` and `JSNull`. Before we can reify `JSUndefined` and `JSNull` we need to give users a way to detect these values. Once existing users have migrated to these new helpers, then we can start boxing `JSUndefined` and `JSNull` on Dart2Wasm. There are a few steps here: 1) Land these helpers, but for now they will just preserve the existing semantics. 2) Deploy JS types to Flutter and `package:test`, i.e. everywhere Dart2Wasm's JS interop is currently being used, and use these helpers instead of `null` checks. 3) Switch the semantics of the helpers to stop conflating on all Web backends, while simultaneously boxing `JSNull` and `JSUndefined` on Dart2Wasm. CoreLibraryReviewExempt: Refactoring web only libraries + some changes to Wasm's internal libraries. Change-Id: Idb50b28b3087438751557ffd28505c7b536bf78b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/282481 Reviewed-by: Srujan Gaddam Commit-Queue: Joshua Litt --- pkg/dart2wasm/lib/js_runtime_generator.dart | 21 ++++++------ .../js_shared/lib/js_interop_patch.dart | 13 +++++++- sdk/lib/_internal/wasm/lib/js_helper.dart | 33 ++++++++++--------- .../_internal/wasm/lib/js_interop_patch.dart | 17 ++++++++-- sdk/lib/_internal/wasm/lib/js_util_patch.dart | 13 +++++--- sdk/lib/_internal/wasm/lib/regexp_helper.dart | 20 +++++------ sdk/lib/js_interop/js_interop.dart | 16 ++++++++- .../js/static_interop_test/js_types_test.dart | 31 +++++++++++++++++ 8 files changed, 120 insertions(+), 44 deletions(-) diff --git a/pkg/dart2wasm/lib/js_runtime_generator.dart b/pkg/dart2wasm/lib/js_runtime_generator.dart index 91bcce66d1d..e377a152ddf 100644 --- a/pkg/dart2wasm/lib/js_runtime_generator.dart +++ b/pkg/dart2wasm/lib/js_runtime_generator.dart @@ -433,7 +433,7 @@ class _JSLowerer extends Transformer { Expression expression; VariableGet v = VariableGet(positionalParameters[i]); if (_isStaticInteropType(callbackParameterType) && boxExternRef) { - expression = _invokeOneArg(_jsValueBoxTarget, v); + expression = _createJSValue(v); } else { expression = AsExpression( _invokeOneArg(_dartifyRawTarget, v), callbackParameterType); @@ -630,6 +630,9 @@ class _JSLowerer extends Transformer { type)); } + Expression _createJSValue(Expression value) => + ConstructorInvocation(_jsValueConstructor, Arguments([value])); + /// Lowers an invocation of `.toJS` to: /// /// JSValue(jsWrapperFunction()) @@ -639,16 +642,11 @@ class _JSLowerer extends Transformer { _createFunctionTrampoline(node, type, boxExternRef: true); Procedure jsWrapperFunction = _getJSWrapperFunction(type, functionTrampolineName, node.fileUri); - return ConstructorInvocation( - _jsValueConstructor, + return _createJSValue(StaticInvocation( + jsWrapperFunction, Arguments([ - StaticInvocation( - jsWrapperFunction, - Arguments([ - StaticInvocation( - _jsObjectFromDartObjectTarget, Arguments([argument])) - ])) - ])); + StaticInvocation(_jsObjectFromDartObjectTarget, Arguments([argument])) + ]))); } InstanceInvocation _invokeMethod( @@ -711,6 +709,9 @@ class _JSLowerer extends Transformer { } else { Expression expression; if (_isStaticInteropType(returnType)) { + // TODO(joshualitt): Expose boxed `JSNull` and `JSUndefined` to Dart + // code after migrating existing users of js interop on Dart2Wasm. + // expression = _createJSValue(invocation); expression = _invokeOneArg(_jsValueBoxTarget, invocation); } else { expression = AsExpression( diff --git a/sdk/lib/_internal/js_shared/lib/js_interop_patch.dart b/sdk/lib/_internal/js_shared/lib/js_interop_patch.dart index 87f8dd7d640..47e72407add 100644 --- a/sdk/lib/_internal/js_shared/lib/js_interop_patch.dart +++ b/sdk/lib/_internal/js_shared/lib/js_interop_patch.dart @@ -2,12 +2,23 @@ // 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 'dart:_foreign_helper' show JS; import 'dart:_internal' show patch; import 'dart:_js_types'; -import 'dart:js'; +import 'dart:js_util'; import 'dart:js_util'; import 'dart:typed_data'; +/// Helper for working with the [JSAny?] top type in a backend agnostic way. +/// TODO(joshualitt): Remove conflation of null and undefined after migration. +extension NullableUndefineableJSAnyExtension on JSAny? { + @patch + bool get isUndefined => this == null || typeofEquals(this, 'undefined'); + + @patch + bool get isNull => this == null || JS('bool', '# === null', this); +} + /// [JSExportedDartFunction] <-> [Function] extension JSExportedDartFunctionToFunction on JSExportedDartFunction { @patch diff --git a/sdk/lib/_internal/wasm/lib/js_helper.dart b/sdk/lib/_internal/wasm/lib/js_helper.dart index 3684e96ce73..295904cc7e5 100644 --- a/sdk/lib/_internal/wasm/lib/js_helper.dart +++ b/sdk/lib/_internal/wasm/lib/js_helper.dart @@ -20,21 +20,24 @@ part 'regexp_helper.dart'; // TODO(joshualitt): In many places we use `WasmExternRef?` when the ref can't // be null, we should use `WasmExternRef` in those cases. -/// [JSValue] is just a box [WasmExternRef]. For now, it is the single box for +/// [JSValue] is just a box [WasmExternRef?]. For now, it is the single box for /// all JS types, but in time we may want to make each JS type a unique box /// type. class JSValue { - final WasmExternRef _ref; + final WasmExternRef? _ref; JSValue(this._ref); - // Currently we always explicitly box JS ref's in [JSValue] objects. In the - // future, we will want to leave these values unboxed when possible, even when - // they are nullable. + // This is currently only used in js_util. + // TODO(joshualitt): Investigate migrating `js_util` to js types. It should be + // non-breaking for js backends, and a tractable migration for wasm backends. static JSValue? box(WasmExternRef? ref) => isDartNull(ref) ? null : JSValue(ref!); - static WasmExternRef? unbox(JSValue? v) => v == null ? null : v._ref; + // We need to handle the case of a nullable [JSValue] to match the semantics + // of the JS backends. + static WasmExternRef? unbox(JSValue? v) => + v == null ? WasmExternRef.nullRef : v._ref; @override bool operator ==(Object that) => @@ -56,7 +59,7 @@ class JSValue { String toString() => stringify(_ref); // Overrides to avoid using [ObjectToJS]. - WasmExternRef toExternRef() => _ref; + WasmExternRef? toExternRef() => _ref; JSValue toJS() => this; } @@ -79,7 +82,8 @@ extension ObjectToJS on Object { WasmExternRef toExternRef() => jsObjectFromDartObject(this); } -// For now both `null` and `undefined` in JS map to `null` in Dart. +// For `dartify` and `jsify`, we match the conflation of `JSUndefined`, `JSNull` +// and `null`. bool isDartNull(WasmExternRef? ref) => ref.isNull || isJSUndefined(ref); // Extensions for [JSArray] and [JSObject]. @@ -373,10 +377,7 @@ WasmExternRef? jsifyRaw(Object? object) { bool isWasmGCStruct(WasmExternRef? ref) => ref.internalize()?.isObject ?? false; Object? dartifyRaw(WasmExternRef? ref) { - if (ref.isNull) { - return null; - } else if (isJSUndefined(ref)) { - // TODO(joshualitt): Introduce a `JSUndefined` type. + if (ref.isNull || isJSUndefined(ref)) { return null; } else if (isJSBoolean(ref)) { return toDartBool(ref); @@ -413,7 +414,7 @@ Object? dartifyRaw(WasmExternRef? ref) { } else if (isWasmGCStruct(ref)) { return jsObjectToDartObject(ref); } else { - return JSValue.box(ref); + return JSValue(ref); } } @@ -455,7 +456,7 @@ Float64List toDartFloat64List(WasmExternRef? ref) => ByteBuffer toDartByteBuffer(WasmExternRef? ref) => toDartByteData(callConstructorVarArgsRaw( - getConstructorString('DataView'), [JSValue.box(ref)].toExternRef())) + getConstructorString('DataView'), [JSValue(ref)].toExternRef())) .buffer; ByteData toDartByteData(WasmExternRef? ref) { @@ -503,7 +504,7 @@ JSArray toJSArray(List list) { List toDartListJSAny(WasmExternRef? ref) => List.generate( objectLength(ref).round(), - (int n) => JSValue.box(objectReadIndex(ref, n.toDouble())) as JSAny?); + (int n) => JSValue(objectReadIndex(ref, n.toDouble())) as JSAny?); List toDartList(WasmExternRef? ref) => List.generate( objectLength(ref).round(), @@ -524,7 +525,7 @@ WasmExternRef? getConstructorRaw(String name) => getPropertyRaw(globalThisRaw(), name.toExternRef()); /// Equivalent to `Object.keys(object)`. -JSArray objectKeys(JSObject object) => JSValue.box(callMethodVarArgsRaw( +JSArray objectKeys(JSObject object) => JSValue(callMethodVarArgsRaw( getConstructorRaw('Object'), 'keys'.toExternRef(), [object].toExternRef())!) as JSArray; diff --git a/sdk/lib/_internal/wasm/lib/js_interop_patch.dart b/sdk/lib/_internal/wasm/lib/js_interop_patch.dart index 56731fcd021..50383602c72 100644 --- a/sdk/lib/_internal/wasm/lib/js_interop_patch.dart +++ b/sdk/lib/_internal/wasm/lib/js_interop_patch.dart @@ -5,6 +5,7 @@ import 'dart:_internal' show patch; import 'dart:_js_helper'; import 'dart:async' show Completer; +import 'dart:js_interop'; import 'dart:js_util' show NullRejectionException; import 'dart:typed_data'; import 'dart:wasm'; @@ -12,8 +13,20 @@ import 'dart:wasm'; /// Some helpers for working with JS types internally. If we implement the JS /// types as inline classes then these should go away. /// TODO(joshualitt): Find a way to get rid of the explicit casts. -WasmExternRef _ref(T o) => (o as JSValue).toExternRef(); -T _box(WasmExternRef? ref) => JSValue.box(ref) as T; +WasmExternRef? _ref(T o) => (o as JSValue).toExternRef(); +T _box(WasmExternRef? ref) => JSValue(ref) as T; + +/// Helper for working with the [JSAny?] top type in a backend agnostic way. +extension NullableUndefineableJSAnyExtension on JSAny? { + // TODO(joshualitt): To support incremental migration of existing users to + // 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(_ref(this!)); + + @patch + bool get isNull => this == null || _ref(this!).isNull; +} /// [JSExportedDartFunction] <-> [Function] extension JSExportedDartFunctionToFunction on JSExportedDartFunction { diff --git a/sdk/lib/_internal/wasm/lib/js_util_patch.dart b/sdk/lib/_internal/wasm/lib/js_util_patch.dart index df4010a8111..5761e1c2787 100644 --- a/sdk/lib/_internal/wasm/lib/js_util_patch.dart +++ b/sdk/lib/_internal/wasm/lib/js_util_patch.dart @@ -39,7 +39,7 @@ dynamic jsify(Object? object) { o is Float64List || o is ByteBuffer || o is ByteData) { - return JSValue.box(jsifyRaw(o)); + return JSValue(jsifyRaw(o)); } if (o is Map) { @@ -60,7 +60,7 @@ dynamic jsify(Object? object) { return convertedIterable; } else { // None of the objects left will require recursive conversions. - return JSValue.box(jsifyRaw(o)); + return JSValue(jsifyRaw(o)); } } @@ -143,6 +143,10 @@ bool lessThan(Object? first, Object? second) => throw 'unimplemented'; @patch bool lessThanOrEqual(Object? first, Object? second) => throw 'unimplemented'; +@patch +bool typeofEquals(Object? o, String type) => + JS('(o, t) => typeof o === t', jsifyRaw(o), jsifyRaw(type)); + typedef _PromiseSuccessFunc = void Function(Object? value); typedef _PromiseFailureFunc = void Function(Object? error); @@ -205,8 +209,9 @@ Object? dartify(Object? object) { return o; } - WasmExternRef ref = o.toExternRef(); - if (isJSBoolean(ref) || + WasmExternRef? ref = o.toExternRef(); + if (ref.isNull || + isJSBoolean(ref) || isJSNumber(ref) || isJSString(ref) || isJSUndefined(ref) || diff --git a/sdk/lib/_internal/wasm/lib/regexp_helper.dart b/sdk/lib/_internal/wasm/lib/regexp_helper.dart index 43326fbda94..45085e41c72 100644 --- a/sdk/lib/_internal/wasm/lib/regexp_helper.dart +++ b/sdk/lib/_internal/wasm/lib/regexp_helper.dart @@ -114,8 +114,8 @@ class JSSyntaxRegExp implements RegExp { RegExpMatch? firstMatch(String string) { JSNativeMatch? m = _nativeRegExp.exec(string); - if (m == null) return null; - return new _MatchImplementation(this, m); + if (m.isUndefinedOrNull) return null; + return new _MatchImplementation(this, m!); } bool hasMatch(String string) { @@ -139,18 +139,18 @@ class JSSyntaxRegExp implements RegExp { JSNativeRegExp regexp = _nativeGlobalVersion; regexp.lastIndex = start; JSNativeMatch? match = regexp.exec(string); - if (match == null) return null; - return new _MatchImplementation(this, match); + if (match.isUndefinedOrNull) return null; + return new _MatchImplementation(this, match!); } RegExpMatch? _execAnchored(String string, int start) { JSNativeRegExp regexp = _nativeAnchoredVersion; regexp.lastIndex = start; JSNativeMatch? match = regexp.exec(string); - if (match == null) return null; + if (match.isUndefinedOrNull) return null; // If the last capture group participated, the original regexp did not // match at the start position. - if (match.pop() != null) return null; + if (match!.pop() != null) return null; return new _MatchImplementation(this, match); } @@ -200,8 +200,8 @@ class _MatchImplementation implements RegExpMatch { String? namedGroup(String name) { JSObject? groups = _match.groups; - if (groups != null) { - Object? result = groups[name]; + if (groups.isDefinedAndNotNull) { + Object? result = groups![name]; if (result != null || hasPropertyRaw( (groups as JSValue).toExternRef(), name.toExternRef())) { @@ -213,8 +213,8 @@ class _MatchImplementation implements RegExpMatch { Iterable get groupNames { JSObject? groups = _match.groups; - if (groups != null) { - return JSArrayIterableAdapter(objectKeys(groups)); + if (groups.isDefinedAndNotNull) { + return JSArrayIterableAdapter(objectKeys(groups!)); } return Iterable.empty(); } diff --git a/sdk/lib/js_interop/js_interop.dart b/sdk/lib/js_interop/js_interop.dart index a85b15fc00c..264334d9e79 100644 --- a/sdk/lib/js_interop/js_interop.dart +++ b/sdk/lib/js_interop/js_interop.dart @@ -93,7 +93,21 @@ typedef JSBoolean = js_types.JSBoolean; /// The type of JS strings, [JSString] <: [JSAny]. typedef JSString = js_types.JSString; -/// TODO(joshualitt): Figure out how we want to handle JSUndefined and JSNull. +/// `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. +extension NullableUndefineableJSAnyExtension on JSAny? { + external bool get isUndefined; + external bool get isNull; + bool get isUndefinedOrNull => isUndefined || isNull; + bool get isDefinedAndNotNull => !isUndefinedOrNull; +} /// The type of `JSUndefined` when returned from functions. Unlike pure JS, /// no actual object will be returned. diff --git a/tests/lib/js/static_interop_test/js_types_test.dart b/tests/lib/js/static_interop_test/js_types_test.dart index cb77cc9394a..9c27359f863 100644 --- a/tests/lib/js/static_interop_test/js_types_test.dart +++ b/tests/lib/js/static_interop_test/js_types_test.dart @@ -88,6 +88,15 @@ external JSBoolean boo; @JS() external JSString str; +@JS() +external JSAny? nullAny; + +@JS() +external JSAny? undefinedAny; + +@JS() +external JSAny? definedNonNullAny; + class DartObject { String get foo => 'bar'; } @@ -100,6 +109,9 @@ void syncTests() { globalThis.fun = function(a, b) { return globalThis.edf(a, b); } + globalThis.nullAny = null; + globalThis.undefinedAny = undefined; + globalThis.definedNonNullAny = {}; '''); // [JSObject] @@ -218,6 +230,25 @@ void syncTests() { expect(str is JSString, true); String dartStr = str.toDart; expect(dartStr, 'foo'); + + // 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); + expect(nullAny.isDefinedAndNotNull, false); + expect(typeofEquals(nullAny, 'object'), true); + //expect(undefinedAny.isNull, false); + expect(undefinedAny.isNull, true); + expect(undefinedAny.isUndefined, true); + expect(undefinedAny.isDefinedAndNotNull, false); + //expect(typeofEquals(undefinedAny, 'undefined'), true); + //expect(typeofEquals(undefinedAny, 'object'), true); + expect(definedNonNullAny.isNull, false); + expect(definedNonNullAny.isUndefined, false); + expect(definedNonNullAny.isDefinedAndNotNull, true); + expect(typeofEquals(definedNonNullAny, 'object'), true); } @JS()