[dart2wasm] Fix a number of minor JS interop issues.

This CL fixes a number of minor JS interop issues.
* `dartify` was moved to Dart, so we can control implicit coercion from JS
* All JS types are 'correctly' handled in `dartify` though a few are boxed
  as opaque types that could be exposed.
* The logic to convert a JS array is now driven from the Dart side.
* Function, Number, and Boolean can now make the roundtrip through Dart.
* The wrapper for Dart functions in JS is now a regular JS function.

Cq-Include-Trybots: luci.dart.try:dart2wasm-linux-x64-d8-try

Change-Id: Ifcd7a447419bca2adf78070e07750c1b3c5c6a18
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247925
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Commit-Queue: Joshua Litt <joshualitt@google.com>

pkg/_js_interop_checks/lib/src/transformations/js_util_wasm_optimizer.dart

@@ -49,7 +49,7 @@ class JsUtilWasmOptimizer extends Transformer {
   final Procedure _setPropertyTarget;
   final Procedure _jsifyRawTarget;
   final Procedure _newObjectTarget;
-  final Procedure _wrapDartCallbackTarget;
+  final Procedure _wrapDartFunctionTarget;
   final Procedure _allowInteropTarget;
   final Class _wasmAnyRefClass;
   final Class _objectClass;
@@ -57,7 +57,7 @@ class JsUtilWasmOptimizer extends Transformer {
   final Field _pragmaName;
   final Field _pragmaOptions;
   final Member _globalThisMember;
-  int _callbackTrampolineN = 1;
+  int _functionTrampolineN = 1;
 
   final CoreTypes _coreTypes;
   final StatefulStaticTypeContext _staticTypeContext;
@@ -77,8 +77,8 @@ class JsUtilWasmOptimizer extends Transformer {
             .getTopLevelProcedure('dart:js_util', 'setProperty'),
         _jsifyRawTarget = _coreTypes.index
             .getTopLevelProcedure('dart:_js_helper', 'jsifyRaw'),
-        _wrapDartCallbackTarget = _coreTypes.index
-            .getTopLevelProcedure('dart:_js_helper', '_wrapDartCallback'),
+        _wrapDartFunctionTarget = _coreTypes.index
+            .getTopLevelProcedure('dart:_js_helper', '_wrapDartFunction'),
         _newObjectTarget =
             _coreTypes.index.getTopLevelProcedure('dart:js_util', 'newObject'),
         _allowInteropTarget =
@@ -210,11 +210,11 @@ class JsUtilWasmOptimizer extends Transformer {
   /// trampoline expects a Dart callback as its first argument, followed by all
   /// of the arguments to the Dart callback as Dart objects. The trampoline will
   /// cast all incoming Dart objects to the appropriate types, dispatch, and
-  /// then `jsifyRaw` any returned value. [_createCallbackTrampoline] Returns a
+  /// then `jsifyRaw` any returned value. [_createFunctionTrampoline] Returns a
   /// [String] function name representing the name of the wrapping function.
   /// TODO(joshualitt): Share callback trampolines if the [FunctionType]
   /// matches.
-  String _createCallbackTrampoline(Procedure node, FunctionType function) {
+  String _createFunctionTrampoline(Procedure node, FunctionType function) {
     int fileOffset = node.fileOffset;
     Library library = node.enclosingLibrary;
 
@@ -245,11 +245,11 @@ class JsUtilWasmOptimizer extends Transformer {
     // a native JS value before being returned to JS.
     DartType nullableWasmAnyRefType =
         _wasmAnyRefClass.getThisType(_coreTypes, Nullability.nullable);
-    final callbackTrampolineName =
-        '|_callbackTrampoline${_callbackTrampolineN++}';
-    final callbackTrampolineImportName = '\$$callbackTrampolineName';
-    final callbackTrampoline = Procedure(
-        Name(callbackTrampolineName, library),
+    final functionTrampolineName =
+        '|_functionTrampoline${_functionTrampolineN++}';
+    final functionTrampolineImportName = '\$$functionTrampolineName';
+    final functionTrampoline = Procedure(
+        Name(functionTrampolineName, library),
         ProcedureKind.Method,
         FunctionNode(
             ReturnStatement(StaticInvocation(
@@ -268,24 +268,24 @@ class JsUtilWasmOptimizer extends Transformer {
         fileUri: node.fileUri)
       ..fileOffset = fileOffset
       ..isNonNullableByDefault = true;
-    callbackTrampoline.addAnnotation(
+    functionTrampoline.addAnnotation(
         ConstantExpression(InstanceConstant(_pragmaClass.reference, [], {
       _pragmaName.fieldReference: StringConstant('wasm:export'),
       _pragmaOptions.fieldReference:
-          StringConstant(callbackTrampolineImportName)
+          StringConstant(functionTrampolineImportName)
     })));
-    library.addProcedure(callbackTrampoline);
-    return callbackTrampolineImportName;
+    library.addProcedure(functionTrampoline);
+    return functionTrampolineImportName;
   }
 
   /// Lowers a [StaticInvocation] of `allowInterop` to
-  /// [_createCallbackTrampoline] followed by `_wrapDartCallback`.
+  /// [_createFunctionTrampoline] followed by `_wrapDartFunction`.
   StaticInvocation _allowInterop(
       Procedure node, FunctionType type, Expression argument) {
-    String callbackTrampolineName = _createCallbackTrampoline(node, type);
+    String functionTrampolineName = _createFunctionTrampoline(node, type);
     return StaticInvocation(
-        _wrapDartCallbackTarget,
-        Arguments([argument, StringLiteral(callbackTrampolineName)],
+        _wrapDartFunctionTarget,
+        Arguments([argument, StringLiteral(functionTrampolineName)],
             types: [type]));
   }
 

pkg/dart2wasm/bin/run_wasm.js

@@ -42,17 +42,6 @@ function stringToDartString(string) {
     }
 }
 
-// Converts a JS array to a Dart List, and also recursively converts the items
-// in the array.
-function arrayToDartList(array, allocator, adder) {
-    var length = array.length;
-    var dartList = dartInstance.exports.$listAllocate();
-    for (var i = 0; i < length; i++) {
-        dartInstance.exports.$listAdd(dartList, array[i]);
-    }
-    return dartList;
-}
-
 // Converts a Dart List to a JS array. Any Dart objects will be converted, but
 // this will be cheap for JSValues.
 function arrayFromDartList(list, reader) {
@@ -64,35 +53,8 @@ function arrayFromDartList(list, reader) {
     return array;
 }
 
-// TODO(joshualitt): Once we can properly return functions, then we can also try
-// returning a regular closure with some custom keys and a special symbol to
-// disambiguate it from other functions. I suspect this will also be necessary
-// for CSP.
-class WrappedDartCallback extends Function {
-    constructor(dartCallback, exportFunctionName) {
-        super('dartCallback', '...args',
-            `return dartInstance.exports['${exportFunctionName}'](
-                dartCallback, ...args.map(dartify));`);
-        this.bound = this.bind(this, dartCallback);
-        this.bound.dartCallback = dartCallback;
-        return this.bound;
-    }
-}
-
-// Recursively converts a JS object into a Dart object.
-function dartify(object) {
-    if (typeof object === "string") {
-        return stringToDartString(object);
-    } else if (object instanceof Array) {
-        return arrayToDartList(object);
-    } else if (object instanceof WrappedDartCallback) {
-        return object.dartCallback;
-    } else if (object instanceof Object) {
-        return dartInstance.exports.$boxJSValue(object);
-    } else {
-        return object;
-    }
-}
+// A special symbol attached to functions that wrap Dart functions.
+var jsWrappedDartFunctionSymbol = Symbol("JSWrappedDartFunction");
 
 // Imports for printing and event loop
 var dart2wasm = {
@@ -118,13 +80,64 @@ var dart2wasm = {
         return stringToDartString(userStackString);
     },
     arrayFromDartList: arrayFromDartList,
-    arrayToDartList: arrayToDartList,
     stringFromDartString: stringFromDartString,
     stringToDartString: stringToDartString,
-    wrapDartCallback: function(dartCallback, exportFunctionName) {
-        return new WrappedDartCallback(dartCallback, exportFunctionName);
+    wrapDartFunction: function(dartFunction, exportFunctionName) {
+        var wrapped = function (...args) {
+            return dartInstance.exports[`${exportFunctionName}`](
+                dartFunction, ...args.map(dartInstance.exports.$dartifyRaw));
+        }
+        wrapped.dartFunction = dartFunction;
+        wrapped[jsWrappedDartFunctionSymbol] = true;
+        return wrapped;
+    },
+    objectLength: function(o) {
+        return o.length;
+    },
+    objectReadIndex: function(o, i) {
+        return o[i];
+    },
+    unwrapJSWrappedDartFunction: function(o) {
+        return o.dartFunction;
+    },
+    isJSUndefined: function(o) {
+        return o === undefined;
+    },
+    isJSBoolean: function(o) {
+        return typeof o === "boolean";
+    },
+    isJSNumber: function(o) {
+        return typeof o === "number";
+    },
+    isJSBigInt: function(o) {
+        return typeof o === "bigint";
+    },
+    isJSString: function(o) {
+        return typeof o === "string";
+    },
+    isJSSymbol: function(o) {
+        return typeof o === "symbol";
+    },
+    isJSFunction: function(o) {
+        return typeof o === "function";
+    },
+    isJSArray: function(o) {
+        return o instanceof Array;
+    },
+    isJSWrappedDartFunction: function(o) {
+        return typeof o === "function" &&
+            o[jsWrappedDartFunctionSymbol] === true;
+    },
+    isJSObject: function(o) {
+        return o instanceof Object;
+    },
+    roundtrip: function (o) {
+      // This function exists as a hook for the native JS -> Wasm type
+      // conversion rules. The Dart runtime will overload variants of this
+      // function with the necessary return type to trigger the desired
+      // coercion.
+      return o;
     },
-    dartify: dartify,
     newObject: function() {
         return {};
     },

sdk/lib/_internal/wasm/lib/js_helper.dart

@@ -18,7 +18,7 @@ class JSValue {
 
   WasmAnyRef toAnyRef() => _ref;
   String toString() => jsStringToDartString(_ref);
-  List<Object?> toObjectList() => jsArrayToDartList(_ref);
+  List<Object?> toObjectList() => toDartList(_ref);
   Object toObject() => jsObjectToDartObject(_ref);
 }
 
@@ -34,24 +34,59 @@ extension ObjectToJS on Object {
   JSValue toJS() => JSValue(jsObjectFromDartObject(this));
 }
 
-Object? toDart(WasmAnyRef? ref) {
-  if (ref == null) {
-    return null;
-  }
-  return jsObjectToDartObject(dartifyRaw(ref)!);
-}
-
 Object jsObjectToDartObject(WasmAnyRef ref) => unsafeCastOpaque<Object>(ref);
 
 WasmAnyRef jsObjectFromDartObject(Object object) =>
     unsafeCastOpaque<WasmAnyRef>(object);
 
+@pragma("wasm:import", "dart2wasm.isJSUndefined")
+external bool isJSUndefined(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSBoolean")
+external bool isJSBoolean(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSNumber")
+external bool isJSNumber(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSBigInt")
+external bool isJSBigInt(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSString")
+external bool isJSString(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSSymbol")
+external bool isJSSymbol(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSFunction")
+external bool isJSFunction(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSArray")
+external bool isJSArray(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSWrappedDartFunction")
+external bool isJSWrappedDartFunction(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.isJSObject")
+external bool isJSObject(WasmAnyRef? o);
+
+@pragma("wasm:import", "dart2wasm.roundtrip")
+external double toDartNumber(WasmAnyRef ref);
+
+@pragma("wasm:import", "dart2wasm.roundtrip")
+external bool toDartBool(WasmAnyRef ref);
+
+@pragma("wasm:import", "dart2wasm.objectLength")
+external double objectLength(WasmAnyRef ref);
+
+@pragma("wasm:import", "dart2wasm.objectReadIndex")
+external WasmAnyRef? objectReadIndex(WasmAnyRef ref, int index);
+
+@pragma("wasm:import", "dart2wasm.unwrapJSWrappedDartFunction")
+external Object? unwrapJSWrappedDartFunction(WasmAnyRef f);
+
 @pragma("wasm:import", "dart2wasm.arrayFromDartList")
 external WasmAnyRef jsArrayFromDartList(List<Object?> list);
 
-@pragma("wasm:import", "dart2wasm.arrayToDartList")
-external List<Object?> jsArrayToDartList(WasmAnyRef list);
-
 @pragma("wasm:import", "dart2wasm.stringFromDartString")
 external WasmAnyRef jsStringFromDartString(String string);
 
@@ -61,9 +96,6 @@ external String jsStringToDartString(WasmAnyRef string);
 @pragma("wasm:import", "dart2wasm.eval")
 external void evalRaw(WasmAnyRef code);
 
-@pragma("wasm:import", "dart2wasm.dartify")
-external WasmAnyRef? dartifyRaw(WasmAnyRef? object);
-
 @pragma("wasm:import", "dart2wasm.newObject")
 external WasmAnyRef newObjectRaw();
 
@@ -113,15 +145,52 @@ WasmAnyRef? jsifyRaw(Object? object) {
   }
 }
 
-@pragma("wasm:import", "dart2wasm.wrapDartCallback")
-external WasmAnyRef _wrapDartCallbackRaw(
-    WasmAnyRef callback, WasmAnyRef trampolineName);
+/// TODO(joshualitt): We shouldn't need this, but otherwise we seem to get a
+/// cast error for certain oddball types(I think undefined, but need to dig
+/// deeper).
+@pragma("wasm:export", "\$dartifyRaw")
+Object? dartifyExported(WasmAnyRef? ref) => dartifyRaw(ref);
 
-F _wrapDartCallback<F extends Function>(F f, String trampolineName) {
+Object? dartifyRaw(WasmAnyRef? ref) {
+  if (ref == null) {
+    return null;
+  } else if (isJSUndefined(ref)) {
+    // TODO(joshualitt): Introduce a `JSUndefined` type.
+    return null;
+  } else if (isJSBoolean(ref)) {
+    return toDartBool(ref);
+  } else if (isJSNumber(ref)) {
+    return toDartNumber(ref);
+  } else if (isJSString(ref)) {
+    return jsStringToDartString(ref);
+  } else if (isJSArray(ref)) {
+    return toDartList(ref);
+  } else if (isJSWrappedDartFunction(ref)) {
+    return unwrapJSWrappedDartFunction(ref);
+  } else if (isJSObject(ref) ||
+      // TODO(joshualitt): We may want to create proxy types for some of these
+      // cases.
+      isJSBigInt(ref) ||
+      isJSSymbol(ref) ||
+      isJSFunction(ref)) {
+    return JSValue(ref);
+  } else {
+    return jsObjectToDartObject(ref);
+  }
+}
+
+List<Object?> toDartList(WasmAnyRef ref) => List<Object?>.generate(
+    objectLength(ref).round(), (int n) => dartifyRaw(objectReadIndex(ref, n)));
+
+@pragma("wasm:import", "dart2wasm.wrapDartFunction")
+external WasmAnyRef _wrapDartFunctionRaw(
+    WasmAnyRef dartFunction, WasmAnyRef trampolineName);
+
+F _wrapDartFunction<F extends Function>(F f, String trampolineName) {
   if (functionToJSWrapper.containsKey(f)) {
     return f;
   }
-  JSValue wrappedFunction = JSValue(_wrapDartCallbackRaw(
+  JSValue wrappedFunction = JSValue(_wrapDartFunctionRaw(
       f.toJS().toAnyRef(), trampolineName.toJS().toAnyRef()));
   functionToJSWrapper[f] = wrappedFunction;
   return f;
@@ -134,13 +203,3 @@ double _listLength(List list) => list.length.toDouble();
 @pragma("wasm:export", "\$listRead")
 WasmAnyRef? _listRead(List<Object?> list, double index) =>
     jsifyRaw(list[index.toInt()]);
-
-@pragma("wasm:export", "\$listAllocate")
-List<Object?> _listAllocate() => [];
-
-@pragma("wasm:export", "\$listAdd")
-void _listAdd(List<Object?> list, WasmAnyRef? item) =>
-    list.add(dartifyRaw(item));
-
-@pragma("wasm:export", "\$boxJSValue")
-JSValue _boxJSValue(WasmAnyRef ref) => JSValue(ref);

sdk/lib/_internal/wasm/lib/js_util_patch.dart

@@ -22,15 +22,15 @@ bool hasProperty(Object o, String name) =>
 
 @patch
 T getProperty<T>(Object o, String name) =>
-    toDart(getPropertyRaw(jsifyRaw(o)!, name.toJS().toAnyRef())) as T;
+    dartifyRaw(getPropertyRaw(jsifyRaw(o)!, name.toJS().toAnyRef())) as T;
 
 @patch
-T setProperty<T>(Object o, String name, T? value) => toDart(
+T setProperty<T>(Object o, String name, T? value) => dartifyRaw(
     setPropertyRaw(jsifyRaw(o)!, name.toJS().toAnyRef(), jsifyRaw(value))) as T;
 
 @patch
 T callMethod<T>(Object o, String method, List<Object?> args) =>
-    toDart(callMethodVarArgsRaw(
+    dartifyRaw(callMethodVarArgsRaw(
         jsifyRaw(o)!, method.toJS().toAnyRef(), args.toJS().toAnyRef())) as T;
 
 @patch
@@ -38,7 +38,7 @@ bool instanceof(Object? o, Object type) => throw 'unimplemented';
 
 @patch
 T callConstructor<T>(Object o, List<Object?> args) =>
-    toDart(callConstructorVarArgsRaw(jsifyRaw(o)!, args.toJS().toAnyRef()))!
+    dartifyRaw(callConstructorVarArgsRaw(jsifyRaw(o)!, args.toJS().toAnyRef()))!
         as T;
 
 @patch
@@ -99,7 +99,7 @@ List<Object?> objectKeys(Object? object) => throw 'unimplemented';
 @patch
 Object? dartify(Object? object) {
   if (object is JSValue) {
-    return jsObjectToDartObject(dartifyRaw(object.toAnyRef())!);
+    return dartifyRaw(object.toAnyRef())!;
   } else {
     return object;
   }

tests/web/wasm/js_util_test.dart

@@ -72,12 +72,21 @@ void deepConversionsTest() {
     globalThis.a = null;
     globalThis.b = 'foo';
     globalThis.c = ['a', 'b', 'c'];
+    globalThis.d = 2.5;
+    globalThis.e = true;
+    globalThis.f = function () { return 'hello world'; };
+    globalThis.invoke = function (f) { return f(); }
   ''');
   Object gt = globalThis;
-  Expect.isNull(dartify(getProperty(gt, 'a')));
-  Expect.equals('foo', dartify(getProperty(gt, 'b')));
-  _expectListEquals(
-      ['a', 'b', 'c'], dartify(getProperty(gt, 'c')) as List<Object?>);
+  Expect.isNull(getProperty(gt, 'a'));
+  Expect.equals('foo', getProperty(gt, 'b'));
+  _expectListEquals(['a', 'b', 'c'], getProperty<List<Object?>>(gt, 'c'));
+  Expect.equals(2.5, getProperty(gt, 'd'));
+  Expect.equals(true, getProperty(gt, 'e'));
+
+  // Confirm a function that takes a roundtrip remains a function.
+  Expect.equals('hello world',
+      callMethod(gt, 'invoke', <Object?>[dartify(getProperty(gt, 'f'))]));
 }
 
 void main() {