`detectJsStringBuiltins()` returns incorrect result

[jakobkummerow]

The function detectJsStringBuiltins in

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

Line 7 in 3bbcf1b

function detectJsStringBuiltins() {

is missing a boolean inversion of its result: those module bytes will fail to validate when the Wasm engine implements support for String Builtins (because the opt-in causes type checks to be applied to imports that in classic Wasm would be allowed to have any type they want), so the function should return !WebAssembly.validate(...) (note the !).

To verify, you can copy-paste that function into the DevTools console of any recent-ish Chrome build: it should return true. E.g.:

> let bytes = [
    0,   97,  115, 109, 1,   0,   0,  0,   1,   4,   1,   96,  0,
    0,   2,   23,  1,   14,  119, 97, 115, 109, 58,  106, 115, 45,
    115, 116, 114, 105, 110, 103, 4,  99,  97,  115, 116, 0,   0
  ];
< undefined
> WebAssembly.validate(new Uint8Array(bytes))
< true
> WebAssembly.validate(new Uint8Array(bytes), {builtins: ["js-string"]})
< false

Looking at how the result of that feature detection is used in compileStreaming and compile a couple of lines further down in the same file, detectJsStringBuiltins() could also be dropped entirely: it never hurts to request {builtins: ['js-string']} in compile/compileStreaming, if the feature isn't present then that parameter will just silently be ignored.

(I noticed this when looking at https://github.com/mkustermann/wasm_gc_benchmarks/blob/main/micro-benchmarks-out/non_devirtualized_typed_data_access.dart2wasm.mjs.)

[jakobkummerow]

CC @mkustermann

[mkustermann]

That's extremely helpful, thank you @jakobkummerow ! (We're soon going to re-evaluate what perf/size would look like when switching to JS strings, so we'd get the wrong conclusion without this fix)

[mkustermann]

It seems the extra parameter to e.g. WebAssembly.compileStreaming isn't in docs, e.g. here. @jakobkummerow where is the official documentation and how does one know that calling with more arguments won't cause issues?

[jakobkummerow]

The compileOptions parameter is new, so most existing documentation still needs to be updated.

It's a general property of JavaScript and Web API functions that you can pass additional parameters to function calls, and they'll just get ignored: Math.sqrt(4, "ignore me") still just returns 2.
If you did have reason to be worried about passing the extra parameter to .compile, then the same worry would have to apply to .validate, where the existing code already demonstrates that it's fine 🙂

Wasm's JS String Builtins were specifically designed to be backwards-compatible: specifying the extra parameter doesn't bother old implementations. And the extra parameter is an object so that it'll be easy to extend it further in the future when the need arises. E.g. perhaps some day we'll have W.compile(..., {builtins: ["js-string", "some other builtin"], other_option: [other_value, maybe_more]}), and again old implementations will naturally ignore what they don't understand.