Conversation
…pace blocks When a merged namespace block references an export declared in a sibling block, find_symbol hands back a proxy symbol carrying a namespace alias. handle_identifier only substitutes that alias for reads, so assignments (`v = 1`, `v++`, `v += 1`, destructuring and for-in/of targets) printed the proxy's bare name, which either threw a ReferenceError or wrote to an unrelated outer variable with the same name. Reads were already emitted as `ns.v`. Register the proxy in is_exported_inside_namespace as well, so writes go through the same `ns.name` rewrite that exports declared in the current block already use. This matches what tsc emits.
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 23 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: ready for review. The diff is green; the remaining CI red is unrelated to this change.
Reproduced on 1.4.0 with let v = "outer";
namespace N { export let v = 1 }
namespace N {
export function set() { v = 99 as any }
export function inc() { v++ }
export function rd() { return v }
}
N.set(); N.inc();
console.log(JSON.stringify({ "N.v": N.v, rd: N.rd(), outer: v }));Before: Tests: six new cases under "generated closures" in test/bundler/transpiler/transpiler.test.js and two |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line fix routing sibling-namespace proxy symbols through the existing is_exported_inside_namespace rewrite so writes emit ns.name like reads already do.
What was reviewed:
- Traced
handle_identifierinsrc/js_parser/p.rs: the namespace-alias path (reads, enum inlining, nested-namespace resolution) still returns first at :1531/:1589, so read behavior is unchanged; writes now fall through to theis_exported_inside_namespacelookup at :1644, which is the same path the current block's own exports already use. - Confirmed
arg_refis the enclosing block's closure argument (in scope at the reference site), matching how same-block exports are rewritten. - Tests cover the write-form matrix, forward references, nesting, local shadowing, and a runtime check — nothing vacuous.
Extended reasoning...
Overview
This PR fixes TypeScript namespace lowering: when a namespace is split across multiple namespace N { ... } blocks and one block assigns to an export declared in a sibling block, Bun previously emitted the assignment as a bare identifier (v = 99) instead of a property access on the closure argument (N.v = 99). Reads were already correct. The fix is a single line in src/js_parser/scan/scan_symbols.rs: when find_symbol creates the per-block proxy symbol for a sibling export, it now also records that proxy in is_exported_inside_namespace (proxy ref → closure-arg ref), which is the map handle_identifier already consults — unconditionally on assignment target — to rewrite same-block exports.
Security risks
None. This is a TypeScript-only transpiler code path (gated behind IS_TYPESCRIPT_ENABLED), affects only how identifiers inside merged namespace blocks are printed, and touches no auth/crypto/permissions/filesystem code.
Level of scrutiny
Medium. The parser is a hot, critical component, but this change is extremely narrow: it only fires when (a) TypeScript is enabled, (b) the reference is inside a namespace block, (c) the name is not a member of the current scope, and (d) the name IS in the shared exported_members map of a sibling block. The fix reuses an existing, well-exercised rewrite mechanism (is_exported_inside_namespace) rather than adding new logic. I traced handle_identifier (p.rs:1500–1686) and confirmed: for non-assignment-target reads, the namespace_alias branch at :1531 still fires first and returns via EImportIdentifier / enum-inline / nested-namespace at :1589/:1557/:1582, so enum member inlining and nested-namespace resolution are preserved; for assignment targets and delete, the alias branch is skipped, is_import_item and ref_to_ts_namespace_member don't contain the fresh proxy ref, and the new map entry at :1644 produces the ns.name E::Dot — exactly the path the current block's own exports already take.
Other factors
- Six new tests in
transpiler.test.jscover every write form (=,++,+=,??=, array/object destructuring targets,for-of/for-in), reassigning an exported function, an export declared in a later block, a write from a nested inner namespace, local shadowing (a same-namedletin the block still shadows), and a spawned runtime check that the outer variable is untouched. The tests assert exact printed output and follow harness conventions (bunExe/bunEnv, drain both pipes concurrently, exit-code asserted last). - The PR intentionally diverges from esbuild 0.25.12 (which shares Bun's old bug) to match
tsc, which is the correct reference for TypeScript-namespace semantics — the prior behavior was aReferenceErroror a silent write to the wrong variable, so there's no compatibility argument for keeping it. - No CODEOWNERS entry covers
src/js_parser/. No prior human review comments to address. The bug-hunting system found nothing.
|
Updated 2:30 AM PT - Sep 8th, 2026
❌ @robobun, your commit 34e2391 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 38709That installs a local version of the PR into your bun-38709 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs — the fix is a one-line map insert that routes assignment targets through the same is_exported_inside_namespace path already used for a block's own exports, and reads are provably unchanged (the alias branch at p.rs:1531 returns first). Deferring only because it deliberately diverges from esbuild to follow tsc; a maintainer should confirm that call.
What was reviewed:
- Traced
handle_identifierinsrc/js_parser/p.rs: alias substitution is gated on non-assignment targets, the new map entry is only reached for writes/deletes, and the proxy'sKind::Otheravoids the "Cannot assign to import" bundling error. - Confirmed
arg_refis the per-block closure argument, so the nested-namespace and later-block cases resolve to the right parameter. - Test coverage spans every write form, shadowing, forward reference, nested namespace, and a runtime check — matches tsc output.
Extended reasoning...
Overview
Two-line source change in src/js_parser/scan/scan_symbols.rs (find_symbol): when a lazily-created proxy symbol is minted for a reference to an export declared in a sibling TypeScript namespace block, also record it in is_exported_inside_namespace (proxy → closure arg). handle_identifier already consults that map without an assign-target guard (p.rs:1644), so writes now emit ns.x instead of a bare x. 168 lines of new tests in test/bundler/transpiler/transpiler.test.js cover =, ++, compound/logical assignment, destructuring targets, for-of/for-in, exported functions, forward references, nested namespaces, local shadowing, and a spawned runtime check.
Security risks
None. Pure TypeScript-lowering codegen; no untrusted-input parsing changes, no I/O, no allocation changes.
Level of scrutiny
Moderate-to-high — find_symbol is a hot parser path that runs on every identifier lookup. The change itself is mechanically trivial (one HashMap insert on a cold TypeScript-only branch, behind Self::IS_TYPESCRIPT_ENABLED), and I verified reads still short-circuit through the namespace_alias branch at p.rs:1531-1589 before reaching the new entry, so enum inlining and nested-namespace resolution are untouched. The proxy symbol is Kind::Other, so the "Cannot assign to import" bundling error at p.rs:1514 correctly does not fire.
Other factors
The PR explicitly diverges from esbuild 0.25.12 (which shares the bug) to follow tsc. REVIEW.md treats esbuild as the reference for ported parser code, so a maintainer should sign off on that deliberate divergence even though following the TypeScript compiler for TypeScript-specific lowering seems clearly correct here. The comment-cop review thread is resolved (comment shortened in 62adf4d). Test coverage is thorough and the PR body demonstrates the six new tests fail on release and pass on the branch.
With --minify-identifiers the bare identifier that was printed for an assignment to a sibling namespace block's export is not reserved by the renamer, so the namespace closure argument can receive the same name and `x = []` silently replaces the namespace object instead of throwing. Add a bundled and a non-bundled minify case that runs the output.
…space-cross-block-assign
This branch has not been deployed
2 participants
Problem
namespace N { export let v = 1 } namespace N { export function set() { v = 99 } }emitsv = 99, notN.v = 99. Reads already emitN.v. The result isReferenceError: v is not definedor a write to an outerv.--minifythe renamer can hand the unreserved bare name to the closure argument:((x)=>(x=[],x.y=2))(e||={})silently replaces the namespace object.find_symbol(src/js_parser/scan/scan_symbols.rs:76) resolves the reference to a proxy symbol that only carries anamespace_alias, andhandle_identifier(src/js_parser/p.rs:1531) applies the alias to reads only.Fix
is_exported_inside_namespace.handle_identifieralready uses that map, in every position, to print a block's own exports asN.v.tsc. esbuild, which this parser is ported from, emits the same bare write, so the divergence is deliberate.test/bundler/transpiler/transpiler.test.jsand two--minifycases intest/bundler/bundler_minify.test.tsfail on 1.4 and pass here.Background
namespace N { ... }block to((N) => { ... })(N ||= {}). An export becomesN.v = 1, andis_exported_inside_namespacemaps its symbol to the closure argument so references print asN.v.namespace_aliasmeans "<closure arg>.<name>".namespace_aliasalso represents an import rewritten to a property access. An assigned-to import must stay a bare identifier, sohandle_identifierskips the alias for assignment targets.Notes
=,++,+=,??=, array and object destructuring targets (including renamed and nested),for (v of ...),for (v in ...), reassigning an exported function, a write to an export declared in a later block, a write from a nested namespace to the outer namespace's export, and a check that a localletof the same name still shadows the export. A spawned runtime case checks that the outer variable is left alone.--minifyoutput of the fuzz input (namespace N { export let x: any = 1 } namespace N { x = []; export const y = 2 }plus an enum that biases the name alphabet towardx) and expect{"x":[],"y":2} 1. On 1.4 both print{"x":1} 1; 1.3 threw a ReferenceError instead, so the silent form is a 1.4 regression of the same bug.v = 99output. tsc 5.9 emitsN.vfor every read and write form.delete, nested namespaces, a namespace that exports a member with its own name, local shadowing) produces the same output astsc's emitted JS under node when run throughbun run,bun build,bun build --minifyandbun --hot.test/bundler/transpiler/transpiler.test.js,test/bundler/esbuild/ts.test.ts,default.test.ts,dce.test.ts,extra.test.ts,bundler_edgecase.test.ts,bundler_regressions.test.tsandbundler_minify.test.tspass with the change.scan_symbols.rs) was addressed in 62adf4d.