Fix Closure await import mangling regression#27173
Conversation
Move `await import` mangling from `mangleUnsupportedSyntax` back to `preprocess` for Closure builds. This fixes a regression introduced in 1cee75d where parenthesized imports like: ```js isPthread = (await import('node:worker_threads')).workerData === 'em-pthread'; ``` were transformed into: ```js isPthread=await import("node:worker_threads").workerData==="em-pthread"; ``` breaking expression precedence and causing incorrect behavior (i.e. `isPthread` always evaluating to false under `-sEXPORT_ES6 -pthread --closure=1`).
| test_file('hello_world.c'), '--closure=1']) | ||
| src = read_file('hello_world.mjs') | ||
| self.assertContained('new URL("hello_world.wasm",import.meta.url)', src) | ||
| self.assertContained('(await import("node:worker_threads")).workerData==="em-pthread"', src) |
There was a problem hiding this comment.
Are these assertions needed? i.e. won't the program fail if we the get the mangling wrong?
| // Replace `await import` with a placeholder during preprocessing, and restore | ||
| // it after other transforms. This must be done in `preprocess` (rather than | ||
| // `mangleUnsupportedSyntax`) to avoid accidentally rewriting `await import` | ||
| // inside `nodePthreadDetection()` and `nodeWWDetection()` macro expansions. |
There was a problem hiding this comment.
I think we need even more explanation here. Why we not what to rewrite those, and how do moving the re-writing prevent that? Are they not run through preprocess?
There was a problem hiding this comment.
I found a better fix for this, PTAL (the PR description has been updated accordingly).
| self.assertContained("new Worker(new URL('hello_world.mjs', import.meta.url), {", src) | ||
| self.assertContained('Hello, world!', self.run_js('hello_world.mjs')) | ||
|
|
||
| def test_esm_worker_closure(self): |
There was a problem hiding this comment.
This is an extend of test_esm_worker, just like test_esm_worker_single_file.
This fixes a regression introduced in 1cee75d where parentheses around an `await import` were incorrectly stripped by Closure. For example: ```js isPthread = (await import('node:worker_threads')).workerData === 'em-pthread'; `` Closure treated the old placeholder as a standard identifier. Since identifiers and function calls have higher operator precedence than member access (`.`), Closure removed the parentheses as redundant. When restored to `await` (which has a lower precedence), the execution order broke: ```js isPthread = await import('node:worker_threads').workerData === 'em-pthread'; ``` This evaluated as `await (import().workerData)`, causing `isPthread` to always be false under `-sEXPORT_ES6 -pthread --closure=1`. Using a low-precedence operator pattern (`void 0 ||`) forces Closure to keep the defensive parentheses intact.
| # https://github.com/google/closure-compiler/blob/v20260401/src/com/google/javascript/jscomp/ConvertChunksToESModules.java#L111-L113 | ||
| src = src \ | ||
| .replace('EMSCRIPTEN$AWAIT$IMPORT', 'await import') \ | ||
| .replace('void 0||await import', 'await import') \ |
There was a problem hiding this comment.
Why do this in two steps like this?
There was a problem hiding this comment.
Without this, it still emits:
isPthread=(void 0||await import("node:worker_threads")).workerData==="em-pthread";The reason this is done in two steps is that Closure optimizes away the other void 0 || patterns.
There was a problem hiding this comment.
Maybe wroth a comment here. I'm still a little confused by this.
|
Test failures looks real, hmm. building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!
building:ERROR: /tmp/emtest_iqyej93f/emscripten_temp_s0l_4of4/out.js:86:28: WARNING - [JSC_SUSPICIOUS_LEFT_OPERAND_OF_LOGICAL_OPERATOR] Left operand of || operator is always falsy.
86| const { createRequire } = void 0||EMSCRIPTEN$AWAIT$IMPORT('node:module');
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
0 error(s), 1 warning(s), 79.1% typedI'll have a look at it tomorrow. |
This fixes a regression introduced in 1cee75d where parentheses around an `await import` were incorrectly stripped by Closure. For example: ```js isPthread = (await import('node:worker_threads')).workerData === 'em-pthread'; `` Closure treated the old placeholder as a standard identifier. Since identifiers and function calls have higher operator precedence than member access (`.`), Closure removed the parentheses as redundant. When restored to `await` (which has a lower precedence), the execution order broke: ```js isPthread = await import('node:worker_threads').workerData === 'em-pthread'; ``` This evaluated as `await (import().workerData)`, causing `isPthread` to always be false under `-sEXPORT_ES6 -pthread --closure=1`. Using a low-precedence logical OR pattern (`EMSCRIPTEN$AWAIT|| `) prevents Closure from stripping the necessary grouping parentheses from any `await` expressions.
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (6) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_minimal_esm.json: 2423 => 2429 [+6 bytes / +0.25%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm.json: 13186 => 13174 [-12 bytes / -0.09%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm2js.json: 18559 => 18547 [-12 bytes / -0.06%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm_singlefile.json: 15040 => 15026 [-14 bytes / -0.09%] codesize/test_minimal_runtime_code_size_hello_webgl_wasm.json: 12724 => 12712 [-12 bytes / -0.09%] codesize/test_minimal_runtime_code_size_hello_webgl_wasm2js.json: 18085 => 18073 [-12 bytes / -0.07%] Average change: -0.03% (-0.09% - +0.25%) ```
|
I found a way to fix the CI failures without the two previous
building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!
building:ERROR: /tmp/emtest_1hhddipc/emscripten_temp_lp4xzu76/hello_wasm_worker.jso1.js:651:0: WARNING - [JSC_USELESS_CODE] Suspicious code. This code lacks side-effects. Is there a bug?
651| EMSCRIPTEN$AWAIT || instantiatePromise;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/tmp/emtest_1hhddipc/emscripten_temp_lp4xzu76/hello_wasm_worker.jso1.js:651:20: WARNING - [JSC_USELESS_CODE] Suspicious code. This code lacks side-effects. Is there a bug?
651| EMSCRIPTEN$AWAIT || instantiatePromise;
^^^^^^^^^^^^^^^^^^
0 error(s), 2 warning(s), 67.0% typedEdit: fixed with commit 2e08062 instead. |
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (8) test expectation files were updated by running the tests with `--rebaseline`: ``` test/codesize/test_codesize_minimal_O0.expected.js updated codesize/test_codesize_minimal_esm.json: 2429 => 2423 [-6 bytes / -0.25%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm.json: 13174 => 13186 [+12 bytes / +0.09%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm2js.json: 18547 => 18559 [+12 bytes / +0.06%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm_singlefile.json: 15026 => 15040 [+14 bytes / +0.09%] codesize/test_minimal_runtime_code_size_hello_webgl_wasm.json: 12712 => 12724 [+12 bytes / +0.09%] codesize/test_minimal_runtime_code_size_hello_webgl_wasm2js.json: 18073 => 18085 [+12 bytes / +0.07%] codesize/test_unoptimized_code_size.json: 177374 => 177390 [+16 bytes / +0.01%] Average change: +0.02% (-0.25% - +0.09%) ```
This fixes a regression introduced in 1cee75d where parentheses
around an
await importwere incorrectly stripped by Closure.For example:
Closure treated the old placeholder as a standard identifier. Since
identifiers and function calls have higher operator precedence than
member access (
.), Closure removed the parentheses as redundant.When restored to
await(which has a lower precedence), theexecution order broke:
This evaluated as
await (import().workerData), causingisPthreadto always be false under
-sEXPORT_ES6 -pthread --closure=1.Using a low-precedence logical OR pattern (
EMSCRIPTEN$AWAIT||)prevents Closure from stripping the necessary grouping parentheses
from any
await importexpressions.