Skip to content

js_parser: fold enum members nested in an earlier namespace into enum initializers - #42118

Open
robobun wants to merge 4 commits into
robobun/59da2d13/enum-const-variable-refsfrom
robobun/0b77c516/enum-values-in-namespaces
Open

robobun wants to merge 4 commits into
robobun/59da2d13/enum-const-variable-refsfrom
robobun/0b77c516/enum-values-in-namespaces

Conversation

@robobun

@robobun robobun commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #42004: the base branch is that PR's branch, and this reuses its eval_ts_constant_expression.

Problem

  • namespace M { export enum Z { Q = 3 } } enum D { A = M.Z.Q, B } emits D[D["B"] = undefined] = "B". tsc emits A = 3, B = 4. esbuild 0.28.2 has the same gap.
  • Cause: the enum pre-visit in visit_stmts (src/js_parser/visit/mod.rs, twin in parse_entry.rs) visits the enums of a statement list first, but not enums nested in a namespace of that list. D is visited before M, while M.Z.Q is still EnumProperty.

Fix

  • When either pre-visit loop passes a namespace, compute_enum_values_inside_namespace evaluates the constant members of the enums inside it with eval_ts_constant_expression and stores them in the member maps, where the visit looks them up. Names resolve from the enum's own scope.
  • The evaluator reaches a member of another block of the same enum through the block's closure argument (ts_member_data_of_symbol), as handle_identifier does.
  • No statement is visited out of order. The later visit of the nested enums stores the same values again.
  • Verified: test/bundler/transpiler/transpiler.test.js (13 new cases) and test/bundler/esbuild/ts.test.ts (1 new case), both fail on the base branch. Self-reviewed: restructured onto js_parser: fold references to const variables in enum initializers #42004 as asked, 4 more concerns addressed (Notes).

Background

  • A member without an initializer continues from the previous member only when that value is a known number (next_numeric_value in s_enum).
  • Each enum and namespace has a TSNamespaceMemberMap. A member is EnumProperty after parsing and becomes EnumNumber or EnumString when s_enum folds its initializer. M.Z.Q is inlined by walking these maps.
  • eval_ts_constant_expression (js_parser: fold references to const variables in enum initializers #42004) evaluates an unvisited expression by tsc's rules and binds names with find_symbol from current_scope. The walk sets current_scope to the enum's scope first.
Notes
  • Review concerns addressed: one evaluator instead of a second one (stacked on js_parser: fold references to const variables in enum initializers #42004); the const to nested enum to later enum case works and has a test; string members computed ahead of the visit are flat (the evaluator never builds ropes) and a test reads one through two template literals; the values do not depend on whether an unrelated enum follows the namespace, because every namespace the loop passes is walked, not only those before an enum.
  • A consequence of the last point: statements before the namespace in the same list see the values too. function f() { return M.Z.Q } namespace M { export enum Z { Q = 1 } } now returns 1 /* Q */. Top-level enums already behave this way (ts/EnumUseBeforeDeclare). A test pins it.
  • A reference to a namespace that comes after the enum is left alone (tsc reports TS2651 there). A test pins that.
  • consts declared inside the namespace body are not known to this early walk (they are recorded when the namespace body is visited, with its scope current). The nested enum's own output still folds them. Only an enum outside the namespace that reads such a member keeps the property access.
  • Not in this change: a bare reference to an exported enum of a sibling block of the same namespace (namespace M { export enum Z { Q = 3 } } namespace M { export enum Y { P = Z.Q, R } }). bun does not register the namespace closure argument in ref_to_ts_namespace_member (esbuild does), and the Namespace arm of the alias path in handle_identifier does not record the usage of the argument as esbuild does, so enabling it is its own change.
  • RuntimeTranspilerCache version goes to 31 on top of js_parser: fold references to const variables in enum initializers #42004's 30. main took 30 in js_parser: store string enum members flat so folds over them stay linear and cannot corrupt them #41973 since, so both numbers move up by one when js_parser: fold references to const variables in enum initializers #42004 rebases.
  • Cost: one statement-kind scan of each namespace body per enclosing list, plus one evaluation of each nested enum initializer, only in files that contain an enum. A 200 KB file of 300 namespaces with 30-member enums takes the same time within noise on the debug build.
  • Also ran: the rest of transpiler.test.js and esbuild/ts.test.ts, bundler_edgecase, bundler_decorator_metadata, decorator-metadata, ts-enum-redecl-panic, regression/issue/10887. cargo clippy -p bun_js_parser is clean.

[human-review] gate passed · iteration 3 · 6 files touched

fails on main (without fix)
ASAN without fix: 13 failed, 39 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/esbuild/ts.test.ts test/bundler/transpiler/transpiler.test.js
bun test v1.4.3 (4ff919377)

test/bundler/transpiler/transpiler.test.js:
(pass) Bun.Transpiler > handles errors when parsing macros [5.80ms]
(pass) Bun.Transpiler > normalizes \r\n [8.14ms]
1
(pass) Bun.Transpiler > doesn't hang indefinitely #2746 [4.63ms]
(pass) Bun.Transpiler > property access inlining > bails out with spread [8.91ms]
(pass) Bun.Transpiler > property access inlining > bails out with multiple items [2.86ms]
(pass) Bun.Transpiler > property access inlining > works [3.42ms]
(pass) Bun.Transpiler > property access inlining > works nested [2.93ms]
(pass) Bun.Transpiler > property access inlining > bails out when the array item is an optional chain [58.71ms]
(pass) Bun.Transpiler > property access inlining > bails out or strips `this` when the index is a call/assignment target [24.39ms]
(pass) Bun.Transpiler > property access inlining > preserves runtime semantics when inlining from a literal index [307.05ms]
(pass) Bun.Transpiler > property access i
... (truncated)

release without fix: 23 failed, 39 skipped
bun test v1.4.3-canary.1 (4ff919377)

test/bundler/transpiler/transpiler.test.js:
(pass) Bun.Transpiler > handles errors when parsing macros [0.11ms]
(pass) Bun.Transpiler > normalizes \r\n [0.16ms]
1
(pass) Bun.Transpiler > doesn't hang indefinitely #2746 [0.08ms]
(pass) Bun.Transpiler > property access inlining > bails out with spread [0.11ms]
(pass) Bun.Transpiler > property access inlining > bails out with multiple items [0.04ms]
(pass) Bun.Transpiler > property access inlining > works [0.03ms]
(pass) Bun.Transpiler > property access inlining > works nested [0.02ms]
(pass) Bun.Transpiler > property access inlining > bails out when the array item is an optional chain [0.77ms]
(pass) Bun.Transpiler > property access inlining > bails out or strips `this` when the index is a call/assignment target [0.29ms]
(pass) Bun.Transpiler > property access inlining > preserves runtime semantics when inlining from a literal index [8.33ms]
(pass) Bun.Transpiler > property access inlining > bails out on optional-chain index into enum [0.42ms]
(pass) Bun.Transpiler > TypeScript > import Foo = Baz.Bar [0.06ms]
(pass) Bun.Transpiler > TypeScript > ternary should parse correctly when
... (truncated)
passes on PR (with fix)
ASAN with fix: 39 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/esbuild/ts.test.ts test/bundler/transpiler/transpiler.test.js
bun test v1.4.3 (4ff919377)

test/bundler/transpiler/transpiler.test.js:
(pass) Bun.Transpiler > handles errors when parsing macros [5.49ms]
(pass) Bun.Transpiler > normalizes \r\n [6.18ms]
1
(pass) Bun.Transpiler > doesn't hang indefinitely #2746 [4.38ms]
(pass) Bun.Transpiler > property access inlining > bails out with spread [9.28ms]
(pass) Bun.Transpiler > property access inlining > bails out with multiple items [2.93ms]
(pass) Bun.Transpiler > property access inlining > works [4.40ms]
(pass) Bun.Transpiler > property access inlining > works nested [5.51ms]
(pass) Bun.Transpiler > property access inlining > bails out when the array item is an optional chain [69.06ms]
(pass) Bun.Transpiler > property access inlining > bails out or strips `this` when the index is a call/assignment target [28.73ms]
(pass) Bun.Transpiler > property access inlining > preserves runtime semantics when inlining from a literal index [393.08ms]
(pass) Bun.Transpiler > property access i
... (truncated)

release with fix: 39 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 701ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/5] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 242 extern-C blocks audited
[1/5] cargo bun_runtime → libbun_runtime.a
�[1m�[92m   Compiling�[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser)
�[1m�[92m   Compiling�[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver)
�[1m�[92m   Compiling�[0m bun_ini v0.0.0 (/workspace/bun/src/ini)
�[1m�[92m   Compiling�[0m bun_bundler v0.0.0 (/workspace/bun/src/bundler)
�[1m�[92m   Compiling�[0m bun_router v0.0.0 (/workspace/bun/src/router)
�[1m�[92m   Compiling�[0m bun_standalone_graph v0.0.0 (/workspace/bun/src/standalone_graph)
�[1m�[92m   Compiling�[0m bun_transpiler v0.0.0 (/workspace/bun/src/transpiler)
�[1m�[92m   Compiling�[0m bun_bunfig v0.0.0 (/workspace/bun/src/bunfig)
�[1m�[92m   Compiling�[0m bun_install v0.0.0 (/workspace/bun/src/install)
�[1m�[92m   Compiling�[0m bun_jsc v0.0.0 (/workspace/bun/src/jsc)
�[1m�[92m   Compiling�[0m bun_js_parser_jsc v0.0.0 (/workspace/bun/src/js_parser_js
... (truncated)
diff hotspot
src/js_parser/parse/parse_entry.rs         |   4 ++
 src/js_parser/visit/mod.rs                 |   4 ++
 src/js_parser/visit/visit_stmt.rs          |  91 +++++++++++++++++++++++++-
 src/jsc/RuntimeTranspilerCache.rs          |   3 +-
 test/bundler/esbuild/ts.test.ts            |  44 +++++++++++++
 test/bundler/transpiler/transpiler.test.js | 101 +++++++++++++++++++++++++++++
 6 files changed, 244 insertions(+), 3 deletions(-)

gate history · 3 passed · 0 rejected · iteration 3

evidence per changed file
file                                        reads  edits  tests
src/js_parser/parse/parse_entry.rs              2      2     26
src/js_parser/visit/mod.rs                      2      2     26
src/js_parser/visit/visit_stmt.rs               8     13     26
src/jsc/RuntimeTranspilerCache.rs               2      2     26
test/bundler/esbuild/ts.test.ts                 1      2     11
test/bundler/transpiler/transpiler.test.js      3      3     21

… initializers

The enums of a statement list are visited before its other statements
so that their member values are known everywhere in the list. Enums
nested in a namespace of that list are only visited with the namespace.
An enum that follows the namespace and references one of their members
(`enum E { A = NS.Inner.X, B }`) was visited first, so the reference
stayed a property access and the members after it became `undefined`.

When the pre-visit loop passes a namespace, compute the constant member
values of the enums inside it from the unvisited initializers, with
eval_ts_constant_expression, and store them where the visit looks them
up. A member of another block of the same enum now resolves through the
block's closure argument there too, as in handle_identifier.
@github-actions github-actions Bot added the claude label Sep 9, 2026
@robobun

robobun commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:14 AM PT - Sep 11th, 2026

❌ @robobun, your commit ecd1db0 has 1 failures in Build #114163 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 42118

That installs a local version of the PR into your bun-42118 executable, so you can run:

bun-42118 --bun

@robobun

robobun commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

Reproduced on 1.4.3 (f42e980) with:

// mzq.ts
namespace M { export enum Z { Q = 3 } }
enum Direct { A = M.Z.Q, B }
console.log(JSON.stringify(Direct));
  • before: {"3":"A","A":3,"undefined":"B"} (bun mzq.ts, bun build, and Bun.Transpiler all emit Direct[Direct["B"] = undefined] = "B")
  • after: {"3":"A","4":"B","A":3,"B":4}, the same as tsc --target es2022

This PR is stacked on #42004 and targets its branch. It becomes mergeable into main after #42004 lands. Comment here at that point and I rebase it onto main.

Blocker (checked 2026-09-23): #42004 conflicts with main in one line, the EXPECTED_VERSION bump in src/jsc/RuntimeTranspilerCache.rs (main is at 33). It needs a rebase before either PR can land. The rest of #42004 merges into main cleanly.

CI: the diff is green. The one red test on this branch, test/js/node/test/parallel/test-crypto-dh-leak.js on the x64-asan lane, is the failure that #41965 fixed on main. The base branch predates #41965, so it fails here and on #42004 alike and goes away with the rebase onto main. The other reported tests passed on retry.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/js_parser/visit/visit_stmt.rs Outdated
Comment thread src/js_parser/visit/visit_stmt.rs
…ber refs to the walk

A second block of an enum may give a member of the first block a new
value, so do not reuse a value found by name. Drop the member refs after
the walk: the visit adds them one at a time, so that a reference to a
later member stays a property access, as in an enum that is not nested.
Comment thread src/js_parser/visit/visit_stmt.rs Outdated
Comment thread src/js_parser/visit/visit_stmt.rs Outdated
Comment thread src/js_parser/visit/visit_stmt.rs Outdated
@robobun

robobun commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed d8f3e04 for the two review notes above: each walk evaluates every initializer again instead of reusing a value found by name (a second block of the same enum can redeclare it), and the member refs it adds to ref_to_ts_namespace_member are removed when the walk ends, so a reference to a later member of the same enum stays a property access as it does at top level (new test). f809a07 shortens the new comments to one line.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both concerns from my earlier pass are addressed in d8f3e04 — the "already computed" fast path is gone so merged-block redeclarations re-evaluate, and the per-member ref_to_ts_namespace_member inserts are now removed after the walk so s_enum's own visit still emits Z.B for a forward reference (pinned by the new test). No further findings; a human look is still worthwhile given this changes transpiler output semantics and is stacked on #42004.

Checked this round: the members_so_far cleanup keeps forward refs unfolded while still letting later members in the same block see earlier ones during evaluation; the current_scope swap is restored on all paths; the twin pre-visit loop in parse_entry.rs gets the same namespace arm; cache version bump to 31 is present.

Extended reasoning...

Overview

This PR extends the TypeScript enum pre-visit in visit_stmts (and its twin in parse_entry.rs) to recursively walk namespace bodies and evaluate nested enums' constant members ahead of time via eval_ts_constant_expression, so that a later top-level enum referencing NS.Inner.X folds correctly and its auto-increment successor gets a value. A helper ts_member_data_of_symbol resolves through namespace_alias for sibling-block references. RuntimeTranspilerCache version bumps to 31. Tests: ~13 printer-output cases in transpiler.test.js and one itBundled runtime case in esbuild/ts.test.ts.

Security risks

None. This is compile-time constant folding in the JS/TS parser; no I/O, network, auth, or untrusted-length arithmetic. The recursion into nested namespaces is guarded by stack_check.is_safe_to_recurse().

Level of scrutiny

Medium-high. Transpiler output correctness is user-visible and cached (hence the version bump), and REVIEW.md calls out that for ported code the reference implementation (tsc) is the spec. The change temporarily swaps current_scope, mutates shared exported_members maps and ref_to_ts_namespace_member, and interacts with merged enum/namespace blocks — subtle enough that a maintainer familiar with the enum/namespace visitor should sign off, especially since it is stacked on unmerged #42004.

Other factors

Since my earlier review, commit d8f3e04 removed the name-keyed "already computed" shortcut (fixing the merged-block redeclare miscount I flagged) and now scopes the ref_to_ts_namespace_member inserts to the pre-walk by removing them afterward, with a new test pinning that A = B inside a namespaced enum still emits Z["A"] = Z.B while M.Z.C folds to 6 for the outer enum. Commit f809a07 collapsed the doc comments to one-liners per repo style. All threads on the timeline are author-self-resolved, but the code diff independently confirms both fixes landed. No new issues found on this pass; the .unwrap() on exported_members.get_ptr_mut(name) is on a map the parser itself populated for every enum member during parse, so it is a provable invariant rather than a user-input path.

@robobun

robobun commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

No open review threads remain. Status: the diff is complete and its CI is green apart from the asan lane failure inherited from the base branch (fixed on main by #41965). This PR waits for #42004 to land, then I rebase it onto main and retarget it.

It reads only the symbol table and the member map, so it does not need
to be compiled once per P instantiation (mordant generic_body_not_generic).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

Noted, thanks. ecd1db0 moved ts_member_data_of_symbol out of the generic impl into a free function, which clears the advisory mordant finding (generic_body_not_generic). Every check on the head is green now except the x64-asan lane failure inherited from the base branch. No review threads are open. This PR still waits for #42004 to land.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant