Repository navigation
Conversation
|
Reproduced on a release build of main 7ca0042 (the parser is the same on 2596ac5): This PR is a draft and cannot land first. Four graphs that Node and main run fail on this branch, because Bun runs a CommonJS file that an ES module imports when its text loads and not where Node evaluates it. |
3e0aceb to
c5cae2c
Compare
…e written
Outside the bundler, the parser moved a top-level class declaration and an
`export default` statement to the top of the module when `can_be_moved()`
was true. The move erased the temporal dead zone of the class binding and
of the default export: code that read them above their statement got a
value, where Node throws ReferenceError. It also put the statement above a
directive such as "use client", and above the hoisted `vi.mock()` and
`jest.mock()` calls of a test file, so a hoisted factory read the class in
its first state.
Delete both arms of the parts loop, so these statements stay where they
are written.
TypeScript `export = value` is now the last statement of the module, as in
tsc and esbuild. The loop had no arm for it, and the class move hid that:
`export = Foo; class Foo {}` ran only because the class moved up. The arm
is the one of #41978.
Delete what only the two arms used: the `bool` that `append_part` returned,
and the `AutoAccessor` clause of `Class::can_be_moved`. Every other caller
sees a class after its accessors are lowered to a static block.
The runtime transpiler cache version goes from 51 to 52, because a cached
module holds the moved text.
…r directive The part loop no longer moves TypeScript `export =` to the end. It runs where it is written, as in what tsc prints with `module: preserve`. The CommonJS wrapper adds "use strict" unless the first statement is that directive. It took any directive, such as "use client", for it.
…that has it The wrapper and the REPL transform skipped the directive when the first statement was any directive. The parser has already removed "use strict" there, so that statement was another directive, such as "use client". Tests: a static accessor in a CommonJS file, which the moved class broke, and four graphs that Node runs and that need CommonJS in import order.
3df73cb to
dce0303
Compare
…ification mock-hoist.test.ts has six more cases. Compiled code still runs a class in front of the other statements of its file, so the six differ there.
The file of the issue has `new X()` above the class. Which binding is read first in its dead zone depends on where a top-level class runs (#46795): 'foo' while Bun runs the class first, 'X' when the class runs where it is written. The test is about the name in the message, so it takes both. The file with the class first still expects 'foo'.
Related to #25569
Problem
classorexport defaultmoves to the top of the module:typeof Cis"function"aboveclass C {}. Node throwsReferenceError.bun test, a hoistedvi.mock()factory reads the moved class in its first state. Vitest throws.before.push(parts.pop())arms insrc/js_parser/parse/parse_entry.rs:1233.Fix
export defaultrun where written."use strict"behind another first directive. The moved class hid that.append_partbool andAutoAccessorclause. Cache version 52.Background
Partis one top-level statement.beforeparts print first.classstatement or a default export is in the temporal dead zone (TDZ) and throws.--aot(built, not pushed) andexport =last (js_parser: emit TypeScriptexport =last, keepmodule.exportsreads after a reassignment #41978).Downsides
test.todorows): three throw, one bindsundefinedwith no error. They need CommonJS in Node's order, which no open PR gives.--aotcopies the move, so itsbun runparity tests fail here.export = Fooabove a class that main could move throws (js_parser: emit TypeScriptexport =last, keepmodule.exportsreads after a reassignment #41978).Notes
State of this PR. The branch holds the parser half: the deletion, the
"use strict"repair that goes with it, and the tests. It is a draft and it is not complete. Two things must come before it (see "Decisions this PR waits for"): a maintainer's decision that the move goes, and a loader change that runs a CommonJS file where Node runs it. The half forbun build --compile --aotis written and measured on the tree before #47942 and is not pushed (see "The ahead-of-time compiler").Repro (main 2596ac5, node v26.3.0)
A program can also get a wrong result with no error. This prints
undefinedon main. Node throws, and so does thebun build --target=bunoutput of the same file.#25569 is a user report of the result. For
new X()aboveclass X { name = foo }Node printsCannot access 'X' before initialization. A release build of main runs the constructor and printsCannot access '' before initialization.This branch prints Node's message. (The empty name is a second bug and is not part of this PR.)test262 (revision 3655e7464d,
language/module-code, 592 module tests that a small harness can run): 13 tests fail on main and pass here, 0 the other way. They areinstn-iee-bndng-cls.js,instn-named-bndng-cls.js,instn-named-bndng-dflt-cls.js,-dflt-expr.js,-dflt-named.js,-dflt-star.jsand sevennamespace/internals/*-uninit.jsfiles.What moved.
class C {},export class C {},export default class N {},export default class {},export default 42,export default () => aandexport default function f() {}. A class withextends, a decorator, a static block, a computed key or a static initializer that is not a constant did not move, so two classes in one file could differ.Where it applied, and where this PR ends. Every parse with
tree_shakingon andbundleoff:bun run(ES modules and CommonJS),bun test,Bun.pluginonLoadtext, macro and plugin modules that a build runs,bun build --no-bundle --target=bun, andBun.TranspilerwithtreeShakingorexports. This PR covers those. It does not change the bundler: the linker prints acan_be_moved()class above an__esmwrapper (generateCodeForFileInChunkJS.rs), and the dev server's HMR lowering places a default export inhmr.exports(lower_esm_exports_hmr.rs). A wrapped bundle module cannot throw on an early read in any case, because module-scopeletandconstbecomevarthere.Hoisted mock factories. Since #44833,
bun testmovesvi.mock()andjest.mock()above the imports of the file.docs/test/mocks.mdxsays: "A hoisted factory can't use the file's top-level variables, because they aren't initialized yet." A class is the exception on main, only because of the move:Main prints
VALUE firstand the file passes. Vitest 4.1.9 on Node fails the file withReferenceError: Cannot access 'Fake' before initialization. With this PR a class behaves as aconstalready does on main:bun testreports thatReferenceError, with the note aboutvi.hoisted(). Three layouts keep the import of the mocked module static (the file exports the import again, the import has attributes, or the mock is in a helper module). There the factory waits for the next import of the module (mockModuleinsrc/jsc/bindings/BunPlugin.cpp), and the file printsVALUE real, asconst Fake = class {}does on main.mock-hoist.test.tshas rows forclass,export classandexport default class, each forvi.mockandjest.mock.History. eec1a07 and c3dc64d (April 2023) added the move for kysely-org/kysely#412 and #1961 (luxon). The author's words on the kysely issue: "This is an ESM <> CJS module load order bug in Bun (not a bug in Kysely)". Those two packages load today without the move (kysely 0.24.2 with kysely-planetscale 1.3.0 and luxon 3.2.1, kysely 0.27.3 with luxon 3.4.4), because #3104 made CommonJS native. #14419 deleted the function hoist that was in the same loop and kept the class arm. #40887 and #46021 changed the arms and kept them. #45631 copied the rule into the AoT compiler, and #47942 documents it for compiled code. This PR starts from the first revision of #35648 (4585d34) and replaces #35648 and #34933. Both were closed by the stale sweep without a review.
Programs that Node and main run and this branch breaks. Runs that print what Node prints, transpiler cache off.
--aotcolumns are executables built by main and by the unpushed AoT half.--aot--aota.mjsimportsc.cjsandb.mjs,c.cjsrequiresb.mjs,b.mjsimportsa.mjsexports, as esbuild and SWC print itundefined, exit 0)const A = class {}import()calls, first module 84 bytesimport()calls, first module 457 KBcommonJSModuleSyntheticSourceCode(src/jsc/bindings/JSCommonJSModule.cpp:1813) callsevaluateCommonJSModuleOnce, so a CommonJS file that an ES module imports runs when its fetch settles, not at its place in the evaluation order. A function that the half-run file already exported then reads a class below therequire(). With the move the class exists. Row 6 shows that main already fails when the class is aconst.c.cjsstill runs, drops theReferenceError, and the import staysundefined.test.todorows inruntime-transpiler.test.ts. They pass on main.BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1is not a workaround. With it graphs 7 and 8 pass 10 of 10 runs here, and graphs 1 to 5 pass 0 of 10.collect_prefixinsrc/aot/modules/plan.rs), so a loader change needs the same change there.Code written only for Bun. These run on main and throw
ReferenceErrorhere:main(); class Foo {}withfunction main()below, andtest.each(cases())wherecases()builds a class that is declared below. Node throws for the first.emitDecoratorMetadata. tsc and Node throw for@d dto: Dtotoo. For@d later: Later | nulltsc printsObjectand Node runs it: Bun printstypeof Later === "undefined" ? Object : Later(emitDecoratorMetadata ignores strictNullChecks:X | undefined/X | nullserialized asXinstead ofObject#44120, emitDecoratorMetadata can cause "Cannot access uninitialized variable." at runtime #7591). typescript: match tsc for experimentalDecorators and emitDecoratorMetadata #46553 changes that. It is atest.todohere.require()or top-levelawait import()loads a cycle partner that reads its class. Node does not run these graphs.require-esm-evaluating-cycle.test.tsalready pins that a laterconstis in its TDZ there. A class now is too.The ahead-of-time compiler.
bun build --compile --aottakesbun runas its oracle, and its front end has a copy of the move:bun_sema::hir::is_moved_to_file_start(src/sema/hir/runtime_syntax.rs:313), read by the lowering (src/aot/lower/statement.rs:42and:60,src/aot/lower/module.rs:105), the scope walk (src/aot/lower/scope/walk/walker.rs:387), the layouts (src/aot/lower/layouts/containers.rs:688) and the dead-zone analysis (src/aot/compile/removable/dead_zone.rs:177). Since #47942,docs/compiler/compatibility.mdxhas the section "A class used above its declaration", which says thatbun runmoves such a class and that an executable does the same. No release has shipped that yet.The same deletion was built on the tree before #47942 (7ca0042) and is not pushed, because #47942 replaced that tree and #48120 moves it again. What that build showed:
test/aot/run-parallel.tsover 7,186 programs: 7,172 pass, 12 are Todo, 1 is red on main too, 1 timed out under load and passes alone. All 8transpiler-parityfiles pass.test/aot/servers, bun, node, hono and koa have 0 functions of another size.CheckInitializedper server: bun 1197, node 1919, hono 1353, koa 2680, express 2667 and elysia 5854 are equal, fastify goes from 6049 to 5888.test/aot/perf/compare-builds.ts, 2% threshold): 0 of 24 rows of the bun, node and hono servers move in one pair of runs. In another pair the threeparamsrows read +4.1% to +7.3%. The exact counters for aparamsrequest are equal on both builds: tests in compiled code 663, 18 and 347, run-time lookups 18, 3 and 22. callgrind was not run: valgrind is not installed here and cannot be installed.constgets better code: the constructor captures the value and loses aCheckInitialized.Until the AoT copy goes in the same merge, the AoT tests that compare a compiled program with
bun runfail on this branch for every program that reads a class above its statement.test/aot/mocks-are-hoisted-above-the-imports.test.tslists the six class rows ofmock-hoist.test.tsas cases that differ in compiled code. They leave that list when the copy goes.export =. An earlier push of this PR also ran TypeScriptexport = valuelast, as esbuild does. That arm is gone. Compiled code runsexport =where it is written, several AoT analyses take it at its text position, and no AoT test hasexport =above what it names. tsc printsmodule.exports = Fooin place undermodule: preserve, whichbun initwrites, and last undercommonjsandnodenext. So on this branchexport = Fooaboveclass Foo {}throwsReferenceErrorinbun run(main prints the class, because the class moved). 0 of 9,510 local TypeScript sources haveexport =above a class: 3 have a top-levelexport =, all as the last statement. #41978 decides the order ofexport =. It needs an AoT half. This PR does not pin the throw with a test.Decisions this PR waits for.
--aotand documents it there.bun test: is a class one of the "top-level variables" that a hoisted mock factory cannot read? If mock hoisting ships before this PR can land, a check of the mock plan in the two arms covers that case alone.export =run last (js_parser: emit TypeScriptexport =last, keepmodule.exportsreads after a reassignment #41978)? Then this PR lands after it.The svelte crash of #35648. With both arms deleted, CI build 80714 had
svelte.test.tsfail withSegmentation fault at address 0x0on linux aarch64. The test loads 2,000 copies of one component. In source order, the module texts ofhello.svelte?r563andhello.svelte?i978have the same length (173 bytes) and the same 24-bit string hash (11767515). JSC's CodeCache then gave the second module the code of the first (oven-sh/WebKit#346). The pair, loaded throughnode:vm, gives the same segfault on bun 1.4.2 and runs on 1.4.3, on main and on this PR.Measurements (release builds of main 7ca0042 and of this branch's parser changes on that base. #47942 did not change the parser.)
transformSync, instructions per transform (median of 5,instructions:ufromperf_event_open, JIT and concurrent GC off): 2,000conststatements 9,414,879 to 9,396,653, 2,000 classes 10,095,196 to 9,835,605 (-130 per class),importplusexport default function71,452 to 71,154, 200 statements withexport =last 986,231 to 984,447.perfis not installed on this machine.export =last 1 to 1.text, parser and AoT halves together: -4,352 bytestest/node_modules, one require chain per package, so it cannot reach graphs 5, 7 and 8). Duplicate-export error anchored on the later statement, as in Node: 29 of 29 cells (main: 13 of 29)."use client"line in--target=bunoutput: plain module 1 (main: 4), automatic-JSX module 2 (main: 7).Other changes you can see
class Box { static accessor count = 1 }throwsTypeError: __privateAdd_… is not a functionon main (since bun test: Vitest compatibility #44833), because the moved class runs above therequireof the helper. Here it prints1, and a test row pins it. Bind parser-generated names with require() in CommonJS-wrapped modules, ahead of hoisted classes #38012 repairs the same crash in another way."use strict"; "use client";is strict on main only if a class moved above the directives. Without a class it is sloppy under--inspectand underbun test --coverage, and the REPL has the same fault. Here the three are strict. js_parser: keep directives as statements and carry the file's prologue on the AST #46730 replaces that code. Whichever lands second drops the hunk.bun run:C; class C {}drops the read, as forlet.Left for later
can_be_moved()class above an__esmwrapper (generateCodeForFileInChunkJS.rs, bundler: keep a top-levelvarstore of a lazily loaded ES module at its statement #45529). A lazily loaded module in a bundle keeps the early value.lower_esm_exports_hmr.rs).usingprintsexport classabove the generatedtry(LowerUsingDeclarationsContext::finalize, js_parser: relocate the module-level declarations of a lowered top-levelusingout of its try #38284).bun rundoes not lowerusing.bun builddoesn't hoist"use strict","use client","use server", etc. directives #6854, js_parser: keep directives as statements and carry the file's prologue on the AST #46730).Self-review. 37 concerns. 28 are addressed in the code, the tests or this text. 2 say that nobody with merge rights has decided in writing to remove the move. 4 ask for the loader change (decision 2). 1 says that a parked draft is closed after 90 days. 2 need no change.
Suites run. Debug build of this branch on 2596ac5: the seven changed test files,
test/cli/test/coverage.test.tsandtest/cli/run/run-cjs.test.ts. 0 tests fail. Inmock-hoist.test.tsone--isolate=modulestest takes 3 to 5 seconds on a debug build and reaches the 5 second limit in some runs. A debug build of main does the same.no test proof · iteration 5 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/repl/repl.test.ts