Keep the TS namespace closure argument from colliding with namespace members#31261
Conversation
…ace members
When a TypeScript namespace contains a member with the same name as the
namespace itself, the generated closure argument falls back to "_" + name
without checking whether that name is also declared in the namespace body.
The non-bundler print paths keep original symbol names, so the emitted
closure re-declared a block-scoped member and the output failed to parse:
namespace m2 { class m2 {} class _m2 {} }
// => ((_m2) => { class m2 {} class _m2 {} })(m2 ||= {})
// SyntaxError: Cannot declare a class twice: '_m2'
Keep prepending "_" until the argument name no longer collides with a
symbol declared in the namespace scope.
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 13 minutes and 17 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 4:24 AM PT - May 23rd, 2026
❌ @robobun, your commit 8e36361 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 31261That installs a local version of the PR into your bun-31261 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested fix for a fuzzer-found collision in the TS namespace closure argument name.
Extended reasoning...
Overview
This PR fixes a fuzzer-found bug in parse_type_script_namespace_stmt (src/js_parser/parse/parse_typescript.rs). When a TypeScript namespace contains a member with the same name as the namespace itself, the generated closure argument is renamed to _<name>. Previously this single-step rename did not check whether _<name> was also declared in the namespace body, producing invalid JS (duplicate block-scoped declaration) on the non-bundler print paths that use NoOpRenamer. The fix loops, prepending _ until the candidate no longer appears in current_scope().members. Three new tests are added in test/bundler/transpiler/transpiler.test.js: a baseline single-collision case, the double-collision repro, and a spawned runtime test that exercises the runtime transpiler path.
Security risks
None. This is a name-generation tweak in the TS→JS lowering path; no auth, crypto, filesystem, or network surface is touched. The loop is trivially bounded by the (finite) number of members in the namespace scope, so there is no DoS vector.
Level of scrutiny
Low-to-moderate. The diff is ~10 lines of logic in a single function, the change is mechanically obvious (turn a single _ prefix into a while contains_key { prepend _ } loop), and the surrounding code is unchanged. strings::cat returns Box<[u8]>, so .into_vec() → Vec<u8> → insert(0, b'_') → alloc_slice_copy(&prefixed) all type-check and preserve the existing arena-copy behavior. The enum closure path is intentionally left alone, and the PR description correctly explains why (enum bodies emit property assignments, not block-scoped declarations).
Other factors
- No CODEOWNERS entry for
src/js_parser/. - The bug hunting system found no issues.
- Tests cover both the printed-output invariant and actual runtime execution, and the PR description shows the relevant test suites passing.
- No prior human review comments to address.
…rena Allocate the underscore-prefixed candidates directly in p.arena instead of going through a temporary global-heap string, matching how the Zig parser allocated the prefixed name. The chosen name is the symbol's original name and is freed together with the AST arena.
There was a problem hiding this comment.
LGTM — straightforward extension of the existing _-prefix rename into a terminating loop, with arena-backed allocations and solid test coverage.
Extended reasoning...
Overview
This PR fixes a TypeScript namespace lowering bug where the generated closure argument (_name) could collide with a member declared inside the namespace body, producing invalid JS on non-bundler paths that skip the renamer. The fix replaces the single "_" + name fallback in parse_type_script_namespace_stmt with a loop that keeps prepending _ until the candidate is absent from current_scope().members. ~12 lines of logic in src/js_parser/parse/parse_typescript.rs plus three new tests in transpiler.test.js (baseline single-rename, double-collision printed output, and a runtime spawn test).
Security risks
None. The input is user source code already being parsed; the change only picks a different generated identifier name. The loop is bounded by the (finite) namespace member set, so there's no DoS vector beyond what arbitrary source already permits, and candidate allocations live in the parse arena which is freed with the AST.
Level of scrutiny
This is core parser code, but the change is mechanically simple: it's the exact same rename that already existed, iterated. alloc_slice_fill_copy returns &mut [u8] so the in-place copy_from_slice is sound, and the chosen candidate becomes the symbol's original_name exactly as the previous alloc_slice_copy result did. Discarded candidates stay in the arena until it's dropped — a few bytes in a pathological edge case, no different from other arena temporaries. The enum path is intentionally left alone (enum bodies don't contain block-scoped declarations), which the PR description calls out.
Other factors
Jarred already reviewed and raised the only substantive question (string lifetime/leak); the follow-up commit 4a032c2 addressed it by building candidates directly in p.arena rather than via a heap temporary, matching the original Zig p.allocator pattern. The bug hunter found no issues, no CODEOWNERS apply to these paths, and the new tests cover both the printed-output invariant and actual runtime execution.
…space-arg-collision
|
The |
There was a problem hiding this comment.
LGTM — small, well-tested fix; the loop's first iteration is byte-identical to the old single-_ path, so only the new collision case changes.
Extended reasoning...
Overview
This PR changes one branch in parse_type_script_namespace_stmt (src/js_parser/parse/parse_typescript.rs): when the generated namespace closure argument would collide with a member declared inside the namespace body, it now keeps prepending _ until the name is free, instead of unconditionally using a single _ prefix. Three tests are added to test/bundler/transpiler/transpiler.test.js: a baseline for the existing single-collision rename, an exact-output check for the double-collision case, and a spawn test that runs the colliding namespace through the runtime transpiler.
Security risks
None. This is symbol-name generation inside the TS→JS lowering path; no untrusted-input handling beyond what the parser already does, no auth/crypto/permissions.
Level of scrutiny
The JS parser is a hot, critical path, which would normally argue for human review. However, the diff is ~15 lines confined to a single already-guarded branch (if members.contains_key(name_text)), and the first loop iteration (underscores = 1) produces exactly the same bytes as the previous strings::cat(b"_", name_text) path — so the common case is unchanged and only the previously-broken double-collision case differs. The loop is bounded by the number of scope members (each candidate has a distinct underscore count, so it can collide at most once per member) and cannot diverge. alloc_slice_fill_copy(underscores + len, b'_') followed by candidate[underscores..].copy_from_slice(name_text) has matching slice lengths, so no panic. Discarded candidates live in the parse arena and are freed with it — Jarred raised this and the thread is resolved; the follow-up commit also removed the global-heap temporary in favor of building directly in p.arena, matching the Zig parser.
Other factors
The CI binary-size failure was a baseline artifact from the ICU zstd change landing on main between the branch point and the canary, not a regression from this diff (0.0 KB on every non-Linux target); main has since been merged in. No CODEOWNERS entry covers src/js_parser/. The bug-hunting pass found nothing, and the new tests cover both printed output and runtime execution.
|
@robobun send follow-up PR deleting these slop code comments you added |
2 participants
Repro
Found by parser fuzzing (invariant: printed output does not reparse, loader=tsx):
produced
which is not valid JavaScript — the closure argument
_m2re-declares the class_m2. The same input as a regular.tsfile fails at runtime, so this breaks executing valid TypeScript, not justBun.Transpiler:tsccompiles this fine (it names the argumentm2_1), and the bundler path is unaffected because the linker runs the symbol renamer.Cause
In
parse_type_script_namespace(src/js_parser/parse/parse_typescript.rs), when the namespace name collides with one of its own members, the generated closure argument falls back to"_" + namewithout checking whether that prefixed name is also declared in the namespace body. The comment inherited from esbuild says the renamer will fix collisions, but the non-bundler print paths (runtime transpiler,Bun.Transpiler,bun build --no-bundle) print symbols withNoOpRenamer, i.e. by their original names, so the collision ends up in the output as a duplicate block-scoped declaration.Fix
Keep prepending
_until the generated argument name no longer collides with any symbol declared in the namespace scope (_m2→__m2for the repro). Enum closures don't need the same treatment because their bodies only contain property assignments, never declarations.Verification
((__m2) => { ... })(m2 ||= {})and the output reparses; the runtime example prints42.test/bundler/transpiler/transpiler.test.js("generated closures"): exact printed output for the colliding case, a baseline for the existing single-collision rename, and a spawn test that runs the colliding namespace and checks its exports. The two collision tests fail on bun without this change and pass with it: