[dart:js_interop/dart2wasm] Missing type parameter check in exported generic callback

[srujzs]

import 'dart:js_interop';

@JS('jsFunction')
external void callJsFunction(JSAny? val);

@JS()
external set jsFunction(JSFunction f);

void main() {
  void test<T extends JSAny?>() {
    jsFunction = ((T t) {
      return t;
    }).toJS;
    callJsFunction(null);
  }

  test<JSAny>();

  void test2<T extends JSAny?>() {
    final f = (T t) {};
    (f as Function)(null);
  }

  test2<JSNumber>();
}

test<JSAny> does not produce any errors, while test2<JSNumber> does the correct check on the parameter.

I had to disable some checks and remove some casts from the transformer to get a third test to work:

  void test3<T extends int?>() {
    jsFunction = ((T t) {
      return t;
    }).toJS;
    callJsFunction(null);
  }

  test3<int>();

but this also does not do a type-check, indicating that something about exporting the function is affecting the type-checking.

The callback should be called in the trampoline function, which is in Dart, so this shouldn't be an issue of the error escaping through JS.

[mkustermann]

/cc @osa1

[osa1]

It looks like interop closures (those converted to JSFunction) need some special treatment when they have generic arguments, but probably the non-interop closures don't.

The reason why test2 is working is because Function is the same as dynamic in that you can't call a Function without runtime type checks based on the function's runtime type. The code that does the type checking is not the function itself, it's the "forwarder". The closure itself ((T t) {}) doesn't have a type test, just like the JS function:

(func $main closure at file:///usr/local/google/home/omersa/dart/sdk_wasm/test/test.dart:20:15 (;684;) (param $var0 (ref struct)) (param $var1 (ref null $JSValue)) (result noneref)
  (local $var2 (ref $<context>))
  local.get $var0
  ref.cast $<context>
  local.set $var2
  ref.null none)

Now the question is, does (T t) {} (without toJS) need a type check? I think no, because it looks like we have proper variance checks when casting function types and I couldn't find a way to call a function with an incorrect type. For example, I can't do this:

void test<T extends A>() {
  final f = (T t) {};
  (f as Function(A))(A()); // static error
}

The only way I could find is by f as Function, which causes a runtime type test in the forwarder, not in the closure.

In the JS case, we can't have something like the forwarders we have in Dart, because in a declaration like

external void callJsFunction(JSAny? val);

The function being called may not be a Dart function with a runtime type that we can use to type check against the argument.

So I think the right place to introduce the type check is when we generate the Dart for the closure's toJS:

// The thing assigned to `jsFunction` should do the type check.
jsFunction = ((T t) {
  return t;
}).toJS;

[srujzs]

Ah, good to have context that these checks are done elsewhere in a "forwarder" and not in the methods themselves. That makes sense why non-exported functions are okay.

There are complications with adding this check in when we do toJS. Callbacks are wrapped in a JS function that then forwards the callback to a trampoline function, which then calls the callback. The ideal place to put this would be in that trampoline function, but the type parameter that we need to cast to is no longer in scope. We tried to do the cast before and that's where #54192 came from. And since we generate these trampolines and the wrapper statically, we don't know what the type parameter is declared as.

What is interesting is that this code:

import 'dart:js_interop';

@JS()
external JSExportedDartFunction get jsFunction;

@JS()
external set jsFunction(JSFunction f);

void main() {
  void test<T extends JSAny?>() {
    jsFunction = ((T t) {
      return t;
    }).toJS;
    final f = jsFunction.toDart;
    f(null);
  }

  test<JSAny>();
}

does throw. I'd expect the mechanisms to be very similar in the case when we call through callJsFunction, except in that case we're just going through a trampoline to do it. There might be something buggy there that is causing dart2wasm to be unable to do those type-checks, but I'd need to investigate more.

[osa1]

What is interesting is that this code:
...
does throw.

That code also throws for the same reason. Return type of toDart is Function, so you have to do a runtime type check to be able to call it.

---

Something else I noticed is we also don't check the "shape" of the function being called. For example, I can call a closure that takes no args, with args:

import 'dart:js_interop';

@JS('jsFunction')
external void callJsFunction(JSAny? v1, JSAny? v2);

@JS()
external set jsFunction(JSFunction f);

void main() {
  void test<T extends JSAny?>() {
    jsFunction = (() {
      return 1;
    }).toJS;
    callJsFunction(1.toJS, 2.toJS); // Dart closure takes no arguments, but this succeeds
  }

  test<JSAny>();
}

This program doesn't throw any exceptions.

---

Here's how it works today (mostly for my own benefit, first time working on this parts of the compiler):

The relevant JS imports:

_232: (x0,x1) => globalThis.jsFunction(x0,x1), // void callJsFunction(v1, v2)
_233: x0 => globalThis.jsFunction = x0, // set jsFunction

The JS export for calling the Dart closure from JS:

(func $_234 (;84;) (export "_234") (param $var0 anyref) (result externref)
  (local $var1 (ref $#Closure-0-0))
  local.get $var0
  ref.cast $#Closure-0-0
  local.tee $var1
  struct.get $#Closure-0-0 $field2
  local.get $var1
  struct.get $#Closure-0-0 $field3
  struct.get $#Vtable-0-0 $field1
  call_ref $type301
  call $jsifyRaw
  return
  ref.null noextern)

Note that this only takes a closure argument, because the Dart closure doesn't take any arguments.

Wasm for @JS functions:

(func $callJsFunction (;693;) (param $var0 (ref null $JSValue)) (param $var1 (ref null $JSValue)) (result (ref null $#Top))
  local.get $var0
  call $JSValue.unbox
  local.get $var1
  call $JSValue.unbox
  call $dart2wasm._232 (import)
  call $dartifyRaw
  drop
  ref.null none)

// The setter
(func $jsFunction (;691;) (param $var0 (ref $JSValue))
  local.get $var0
  call $JSValue.unbox
  call $dart2wasm._233 (import)
  call $dartifyRaw
  drop)

The setter is called here:

(func $main closure at file:///usr/local/google/home/omersa/dart/sdk_wasm/test/test.dart:10:30 (;687;) (param $var0 (ref struct)) (param $var1 (ref $_Type)) (result (ref null $#Top))
  ...
  struct.new $#Closure-0-0                      ;; allocate Dart closure
  call $jsObjectFromDartObject                  ;; calls extern.convert_any, converts `ref any` to `ref extern`
  call $dart2wasm._234 (import)                 ;; imports `f => finalizeWrapper(f,() => dartInstance.exports._234(f))`
  call $JSValue.box
  ref.as_non_null
  call $jsFunction                              ;; sets jsFunction JS global = `finalizeWrapper(f, () => ...)`
  global.get $global328
  call $NumToJSExtension|get#toJS               ;; first arg
  global.get $global40
  call $NumToJSExtension|get#toJS               ;; second arg
  call $callJsFunction                          ;; call JS global
  drop
  ref.null none)

finalizeWrapper:

function finalizeWrapper(dartFunction, wrapped) {
    wrapped.dartFunction = dartFunction;
    wrapped[jsWrappedDartFunctionSymbol] = true;
    return wrapped;
}

The reason why this program works without any errors is because the closure being from JS is this argument to finalizeWrapper: () => dartInstance.exports._234(f) this takes no argument, but in JS passing extra arguments doesn't cause problems.

It also works when I don't pass required arguments, because in JS land unpassed args become "undefined" 😃

---

Getting back to the question, in the call sequence:

1. globalThis.jsFunction(<args>) called by Dart callJSFunction(...)
2. which calls dartInstance.exports._234
3. which calls the Dart closure

If I understand correctly, the export _234 is specific to the closure (i.e. not per closure type, closures with same types get different exports), so I think we can do the shape (number of args) and type checks there. @srujzs is this function what you call "trampoline"? If it is then I think we're in agreement on where to add the checks.

Type checks could be done by the closure itself, but that would make calling it from Dart inefficient as we'd be redundantly checking types.

(I'm ignoring optional positional and named arguments, I'm assuming we either don't allow those in functions converted to JS, or the order of things is somehow handled correctly in (2))

[srujzs]

Sorry for the late response! This fell into the holiday void backlog. :)

That code also throws for the same reason. Return type of toDart is Function, so you have to do a runtime type check to be able to call it.

Ah, yeah, this relates to your comment above regarding the forwarder. I wonder if the solution is we should cast the callback as Function before calling it in the "trampoline" so the forwarder can do that check? I tried doing there here:

sdk/pkg/dart2wasm/lib/js/callback_specializer.dart

Line 51 in 3cc4876

VariableGet(callbackVariable), Arguments(callbackArguments),

, but no luck.

---

It also works when I don't pass required arguments, because in JS land unpassed args become "undefined" 😃

I think passing in too many args is actually okay and in some cases, desired by users e.g. #48186. :) We should be checking if we have too few args, though, but I think that code was only specialized for callbacks that take optional parameters.

In the case where we have optional parameters, the JS function that we pass to finalizeWrapper actually passes in arguments.length to the "trampoline" that then calls the callback. In this case, depending on the amount of arguments passed, we generate a different callback call for each possibility (callback(a), callback(a, b), etc.). I think this is to support the case where we have a callback with optional parameters and default values e.g.

(int a, [int b = 50]) {
  return b;
}.toJS;

At any rate, we can adopt the convention of passing arguments.length in the case where the callback doesn't have any optionals as well. We can use this to always emit a check that the user passed more or equal the number of arguments as there are required parameters. FYI, a lot of this logic is in callback_specializer.dart if you're curious.

@srujzs is this function what you call "trampoline"? If it is then I think we're in agreement on where to add the checks.

Yup! I've probably abused that term in various contexts, so I should be more clear here.

(I'm ignoring optional positional and named arguments, I'm assuming we either don't allow those in functions converted to JS, or the order of things is somehow handled correctly in (2))

We don't allow named arguments in callbacks (I don't think you can call them in JS even if you wanted to?), just in object literal constructors.

Also, out of curiosity, what are you using to view Wasm files? VS code + the Wasm extension keeps mangling the module that dart2wasm outputs.