Base support for coalescing & logical assignment

[RReverser]

Base PR for #20531.

[sbc100]

Nice!

I so think we might run into problems with out min node version.. can you confirm by adding other.test_es5_transpile to the list of tests in the test-node-compat job on circleci? My guess is that it will fail.. and we will need to bump out minimum supported (by the compiler itself, not its output) node version.

[sbc100]

Do you know what the first node version to support this syntax was?

[RReverser]

I actually tried some of the failed tests from the other PR - e.g. test_promise - locally and it fails even with this change. I don't know why yet.

[RReverser]

Do you know what the first node version to support this syntax was?

Node.js 16 so already covered by our MIN_NODE_VERSION.

[RReverser]

If we just do nullish operators ?? and ?. without logical assignment (??=, ||=, &&=) then Node.js 14 would suffice too. But it's a shame to lose those.

[RReverser]

I actually tried some of the failed tests from the other PR - e.g. test_promise - locally and it fails even with this change. I don't know why yet.

Ahh you're probably spot on with that question. I see it sets self.set_setting('MIN_NODE_VERSION', '150000') which likely causes issues.

and we will need to bump out minimum supported (by the compiler itself, not its output) node version.

what is our minimum right now?

[sbc100]

Do you know what the first node version to support this syntax was?

Node.js 16 so already covered by our MIN_NODE_VERSION.

MIN_NODE_VERSION relates to the min node supported by the generated code.

The min node version needed to the compiler itself is completely separate and is specified here:

emscripten/tools/shared.py

Lines 55 to 59 in 1dedd1b

	# Minimum node version required to run the emscripten compiler. This is distinct
	# from the minimum version required to execute the generated code. This is not an
	# exact requirement, but is the oldest version of node that we do any testing with.
	# This version aligns with the current Ubuuntu TLS 20.04 (Focal).
	MINIMUM_NODE_VERSION = (10, 19, 0)

(Currently 10.19.0 which is the version we test in test-node-compat).

[RReverser]

(Currently 10.19.0 which is the version we test in test-node-compat).

Ah that's pretty ancient and long dead even for security updates :( Why does it use different version than whatever emsdk ships? For scenarios of folks using Emscripten w/o emsdk?

But, assuming I revert changes to parseTools.js and such, why does it even matter?

[sbc100]

(Currently 10.19.0 which is the version we test in test-node-compat).

Ah that's pretty ancient and long dead even for security updates :( Why does it use different version than whatever emsdk ships? For scenarios of folks using Emscripten w/o emsdk?

But, assuming I revert changes to parseTools.js and such, why does it even matter?

For this change it won't matter since we don't currently run test_es5_transpile in test-node-compat. But as soon as we add any unsupported stuff to the core stdlib the compiler will no longer run on that old version of node.

[sbc100]

(Currently 10.19.0 which is the version we test in test-node-compat).

Ah that's pretty ancient and long dead even for security updates :( Why does it use different version than whatever emsdk ships? For scenarios of folks using Emscripten w/o emsdk?

Yes, for folks who just want use their OS/system version of node. Its probably reasonable to bump that version, but we should do that as a separate change.

[sbc100]

(Currently 10.19.0 which is the version we test in test-node-compat).

Ah that's pretty ancient and long dead even for security updates :( Why does it use different version than whatever emsdk ships? For scenarios of folks using Emscripten w/o emsdk?

Yes, for folks who just want use their OS/system version of node. Its probably reasonable to bump that version, but we should do that as a separate change.

It looks like debian stable (bookworm, which was recently released) has 18.3: https://packages.debian.org/bookworm/arm64/nodejs

The previous debian/stable was 12.22.12: https://packages.debian.org/bullseye/arm64/nodejs

The current ubuntu LTS (jammy) has 12.22.9 : https://packages.ubuntu.com/jammy/amd64/nodejs.

Would 12.22.9 have the features we need?

[RReverser]

Would 12.22.9 have the features we need?

No, none of those :(

I believe most folks install newer Node.js from https://github.com/nodesource/distributions anyway, but I might be mistaken.

[sbc100]

I guess we need to update our min supported version to 16.0 then? That doesn't seem unreasonable at this point.

[RReverser]

If you think that's an option, that would be great, yeah. Alternatively, as I said, at least Node 14 would get us halfway (some of the newer operators but not all).

[RReverser]

Something I still don't understand is how a change like this can lead to Wasmer bulk-memory validation issues (see failing standalone tests, it's as if standalone modules are suddenly compiled with bulk memory support but weren't before):

JS subprocess failed (/root/vms/wasmer run /tmp/emtest_xye538o6/emscripten_test_core0_mmqlmle0/test_posixtime.wasm): 1. Output:
Error: Can't compile module: ValidationError { msg: "bulk memory support is not enabled" }

How is that connected to either ECMAScript version update or even Node.js version?

[sbc100]

Something I still don't understand is how a change like this can lead to Wasmer bulk-memory validation issues (see failing standalone tests, it's as if standalone modules are suddenly compiled with bulk memory support but weren't before):

JS subprocess failed (/root/vms/wasmer run /tmp/emtest_xye538o6/emscripten_test_core0_mmqlmle0/test_posixtime.wasm): 1. Output:
Error: Can't compile module: ValidationError { msg: "bulk memory support is not enabled" }

How is that connected to either ECMAScript version update or even Node.js version?

We use the feature matrix code to opt into certain features based on the MIN_XXX_VERSIONS specified for the different targets:

emscripten/emcc.py

Lines 2514 to 2519 in 1dedd1b

	# TODO(sbc): Find make a generic way to expose the feature matrix to JS
	# compiler rather then adding them all ad-hoc as internal settings
	settings.SUPPORTS_GLOBALTHIS = feature_matrix.caniuse(feature_matrix.Feature.GLOBALTHIS)
	settings.SUPPORTS_PROMISE_ANY = feature_matrix.caniuse(feature_matrix.Feature.PROMISE_ANY)
	if not settings.BULK_MEMORY:
	settings.BULK_MEMORY = feature_matrix.caniuse(feature_matrix.Feature.BULK_MEMORY)

This is basically saying: enable bulk memory if all our current targets support it.

[RReverser]

This is basically saying: enable bulk memory if all our current targets support it.

Hm that seems like a bug that feature matrix of browser/JS engines affects standalone target?

[sbc100]

This is basically saying: enable bulk memory if all our current targets support it.

Hm that seems like a bug that feature matrix of browser/JS engines affects standalone target?

The standalone builds can still run in browser and engines and JS engines though. Are you suggesting that we simply ignore MIN_XX_VERIONS settings in standalone mode?

The fix here is probably to set -sBULK_MEMORY=0 since we don't have a MIN_WASMER_VERSION setting.

[RReverser]

Are you suggesting that we simply ignore MIN_XX_VERIONS settings in standalone mode?

That seems like the safest bet, but not ideal. I guess a proper alternative would be to have MIN_XX_VERSION for different standalone engines too (e.g. Wasmer + Wasmtime) that are taken into account in standalone mode.

That said, it looks like the actual problem is that CI is using very old prerelease version of Wasmer for testing. As per https://webassembly.org/roadmap/, it supported bulk memory since 1.0.

[sbc100]

That said, it looks like the actual problem is that CI is using very old prerelease version of Wasmer for testing. As per https://webassembly.org/roadmap/, it supported bulk memory since 1.0.

Yup, we can/should update that.

[RReverser]

The remaining failures are interesting and probably not going to be addressed by #20551. They all have the same cause:

+RuntimeError: Aborted(CompileError: WebAssembly.instantiate(): Compiling function #15:"emscripten_memset_bulkmem" failed: memory.fill[2] expected type i32, found local.get of type i64 @+1859)
+    at abort (/tmp/emtest_fsexcy95/emscripten_test_other_8gw_9wy3/a.out.js:666:11)
+    at /tmp/emtest_fsexcy95/emscripten_test_other_8gw_9wy3/a.out.js:780:5
+Thrown at:
+    at abort (/tmp/emtest_fsexcy95/emscripten_test_other_8gw_9wy3/a.out.js:666:11)
+    at /tmp/emtest_fsexcy95/emscripten_test_other_8gw_9wy3/a.out.js:780:5

It appears that bumping browser versions here makes BULK_MEMORY enabled by default, as after this bump it's now supported by all engines.

That, in turn, breaks wasm64 tests because bulk memory seems incompatible with wasm64 at the moment, or rather emscripten_memset_bulkmem is due to wrong signature.

I'm going to raise a separate issue for it for someone else to look into.

[RReverser]

All tests here finally pass but I've split out bulk memory + wasm64 fix into #20549 and will wait for it to be merged first.

[RReverser]

All tests here finally pass

...not counting the ever-flaking test_pthread_proxying_refcount. Restarted.

[sbc100]

Now that #20551 has landed I think this should be good to go.

[RReverser]

This is also waiting for #20562. Which I have already restarted but looks like I'll have to again (unless it's a legitimate failure).