Lower accessor-only classes in place instead of relocating static elements - #31926
Lower accessor-only classes in place instead of relocating static elements#31926robobun wants to merge 4 commits into
Conversation
WalkthroughAdds in-place lowering for undecorated classes that converts auto-accessors into private backing fields and getter/setter pairs. The change preserves computed-key evaluation, avoids private-name collisions, supports expression and statement output, updates default-export handling, and adds runtime and snapshot tests. ChangesAuto-accessor lowering for decorator-less classes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:26 AM PT - Aug 13th, 2026
✅ @robobun, your commit 6159800fd12eb60918959af48e6d7453583f5fb8 passed in 🧪 To try this PR locally: bunx bun-pr 31926That installs a local version of the PR into your bun-31926 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
StatusThis PR is the consolidated fix for undecorated classes that use Reproduced how: the issue's snippet throws Related PRs that stay open because they fix different things in the decorated path: #31930 (file-unique lowering temporaries, which is what the decorated-getter variant in the #29837 comment thread needs) and #35708 (class-body ordering for undecorated accessors and lowered privates inside decorated classes). |
There was a problem hiding this comment.
I didn't find any bugs, but this adds a new ~230-line lowering path in the transpiler (private-name collision avoidance, computed-key temporaries, in-place class rewriting) and narrows the export default name-injection condition — worth a human pass on the parser changes.
Extended reasoning...
Overview
This PR changes how Bun's transpiler lowers ES accessor class members when the class has no decorators. It adds lower_auto_accessors_in_place (~230 lines) in src/js_parser/lower/lower_decorators.rs, which rewrites each accessor x = init into a #x = init backing field plus get x()/set x(v) pair in situ, instead of routing through the full decorator relocation machinery. It also extracts a shared push_accessor_get_set_pair helper, fixes computed accessor keys to evaluate once (on both the new path and the existing decorated path), and narrows the export default class name-injection check in visit_stmt.rs from has_decorators || should_lower_standard_decorators to just has_decorators. Twelve new runtime tests cover private-name scoping, static/instance evaluation order, subclass override, backing-name collisions (own and enclosing-class), computed keys, and the .name === "default" case.
Security risks
None. This is pure AST-to-AST transformation in the transpiler; there's no I/O, auth, crypto, or untrusted-input parsing surface beyond what already exists. The worst failure mode is emitting incorrect JavaScript.
Level of scrutiny
High. The JS parser/transpiler is on the hot path for every file Bun loads, and this changes the output shape for an entire class of inputs (any class containing accessor and no decorators). The new path involves several subtle pieces: iterating the global symbol table to collect taken private names, generating fresh #name symbols with suffix-based deduplication, hoisting var _computedKey declarations to the nearest statement list (with separate handling for expression vs. statement position), and preserving/clearing property flags correctly. The visit_stmt.rs change is sound given that class.has_decorators is set from has_any_decorators (which already covers both class-level and member-level decorators per parse/mod.rs:264 / parse.zig:202), so the only case dropped is the accessor-only one — but that coupling is non-obvious and worth a human confirming.
Other factors
The PR is well-described and well-tested (12 targeted runtime tests, plus the existing es-decorators / esbuild / decorator-metadata suites reportedly pass). The bug-hunting system found nothing. CODEOWNERS does not cover src/js_parser/. The CI failure is an unrelated -no-pie linker warning across all build-rust jobs. Still, this is a non-trivial refactor of spec-sensitive lowering logic with several interacting edge cases (private auto-accessors, nested classes, computed keys in expression position), so I'm deferring rather than approving.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/lower/lower_decorators.rs`:
- Around line 961-1008: The bug is that drain_capture_temp_decls(...) currently
dumps all receiver-capture temps into the one-shot class prefix, causing temps
created while rewriting instance initializers to be shared across constructions;
update the lowering so temps created during instance-initializer rewriting are
not drained into the class-level prefix but are declared per-construction
alongside the injected initializer statements. Concretely: change the flow in
declare_capture_temps_in_fn_body/where prefix_stmts are built so that only true
class-eval temps are collected by drain_capture_temp_decls(...) for the class
prefix, while temps flagged/created during instance initializer rewriting are
captured and emitted into the per-construction initializer statement list (i.e.,
insert the decl stmt returned by drain_capture_temp_decls(...) into the injected
initializer stmts instead of prefix_stmts); apply the same fix at the other
sibling site mentioned (around the 2730-2740 region) so all
instance-construction temps follow the per-construction declaration path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73b8d8b0-6a6a-4090-afcb-c9a2b5423c86
📒 Files selected for processing (2)
src/js_parser/lower/lower_decorators.rstest/bundler/transpiler/es-decorators.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/lower/lower_decorators.rs`:
- Around line 961-1008: The bug is that drain_capture_temp_decls(...) currently
dumps all receiver-capture temps into the one-shot class prefix, causing temps
created while rewriting instance initializers to be shared across constructions;
update the lowering so temps created during instance-initializer rewriting are
not drained into the class-level prefix but are declared per-construction
alongside the injected initializer statements. Concretely: change the flow in
declare_capture_temps_in_fn_body/where prefix_stmts are built so that only true
class-eval temps are collected by drain_capture_temp_decls(...) for the class
prefix, while temps flagged/created during instance initializer rewriting are
captured and emitted into the per-construction initializer statement list (i.e.,
insert the decl stmt returned by drain_capture_temp_decls(...) into the injected
initializer stmts instead of prefix_stmts); apply the same fix at the other
sibling site mentioned (around the 2730-2740 region) so all
instance-construction temps follow the per-construction declaration path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73b8d8b0-6a6a-4090-afcb-c9a2b5423c86
📒 Files selected for processing (2)
src/js_parser/lower/lower_decorators.rstest/bundler/transpiler/es-decorators.test.ts
🛑 Comments failed to post (1)
src/js_parser/lower/lower_decorators.rs (1)
961-1008:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep constructor-time receiver temps out of the one-shot class prefix.
declare_capture_temps_in_fn_body()only fixes nested functions/arrows. Any_objcreated while rewriting an instance initializer still falls through to the finaldrain_capture_temp_decls(...), so it is emitted once inprefix_stmtsand shared by every construction. That leaves the same re-entrancy hole here:__privateGet(_obj = recv, ..., getter).call(_obj)can synchronously construct another instance before the outer.call(_obj)reads_obj, and the inner construction overwrites the shared binding. These temps need a per-construction declaration path alongside the injected initializer statements; only true class-evaluation temps should be drained into the class prefix.
As per coding guidelines, "Rust code: fix the whole bug class in the same PR - grep for every sibling site sharing the pattern".Also applies to: 2730-2740
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/lower/lower_decorators.rs` around lines 961 - 1008, The bug is that drain_capture_temp_decls(...) currently dumps all receiver-capture temps into the one-shot class prefix, causing temps created while rewriting instance initializers to be shared across constructions; update the lowering so temps created during instance-initializer rewriting are not drained into the class-level prefix but are declared per-construction alongside the injected initializer statements. Concretely: change the flow in declare_capture_temps_in_fn_body/where prefix_stmts are built so that only true class-eval temps are collected by drain_capture_temp_decls(...) for the class prefix, while temps flagged/created during instance initializer rewriting are captured and emitted into the per-construction initializer statement list (i.e., insert the decl stmt returned by drain_capture_temp_decls(...) into the injected initializer stmts instead of prefix_stmts); apply the same fix at the other sibling site mentioned (around the 2730-2740 region) so all instance-construction temps follow the per-construction declaration path.Source: Coding guidelines
|
Re CodeRabbit's finding on drain_capture_temp_decls (lines 961-1008, 2730-2740): declining, for two reasons.
|
There was a problem hiding this comment.
I didn't find any bugs, but this introduces a new ~230-line lowering path in the transpiler with subtle spec-ordering and private-name-scoping implications, so it's worth a human pass before merge.
Extended reasoning...
Overview
This PR adds a new in-place lowering path (lower_auto_accessors_in_place) in src/js_parser/lower/lower_decorators.rs for classes that contain accessor members but no decorators, replacing each accessor with a private backing field plus a getter/setter pair without relocating any other class elements. It also extracts a shared push_accessor_get_set_pair helper, fixes computed-key double-evaluation in the existing decorated-class path, and tightens the export default class name-injection condition in visit_stmt.rs to check has_decorators only. Twelve new runtime tests are added.
Security risks
None. This is pure transpiler/codegen logic with no auth, network, filesystem, or trust-boundary changes.
Level of scrutiny
High. The JS parser/lowering pipeline runs on every transpiled file, and this PR adds an entirely new code path that constructs AST by hand: it iterates the global symbol table to over-approximate taken private names, synthesizes new private symbols, rewrites property lists, and hoists computed-key temporaries via nearest_stmt_list_mut for class expressions. The correctness depends on subtle invariants — e.g. that emitting #backing = init textually before get [_computedKey = key()] still yields spec-correct evaluation order (it does, because method computed keys evaluate before static field initializers, but that's exactly the kind of thing a maintainer should sanity-check), and that class.class.has_decorators is set for both legacy-TS and standard decorators so the visit_stmt.rs narrowing doesn't regress export default @dec class {} name injection.
Other factors
Test coverage is strong (12 new end-to-end runtime tests covering private-name scoping, static/instance order, subclass override, name collisions, computed keys, and export default naming; existing decorator/transpiler suites pass per the description), and the only CI failure (bunx.test.ts) is a known fleet-wide flake unrelated to this change. The bug-hunting system found nothing. Still, the size and centrality of the change put it outside the bar for bot auto-approval.
bbe6bbd to
9fceb66
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~300-line change to core transpiler lowering (new in-place accessor path, private-name collision logic, narrowed export default name-injection condition) that affects emitted-JS correctness, so it's worth a human look.
Extended reasoning...
Overview
This PR touches three files: src/js_parser/lower/lower_decorators.rs (the bulk — a new lower_auto_accessors_in_place function plus a push_accessor_get_set_pair helper and a refactor of the existing undecorated-accessor branch in the decorated path), src/js_parser/visit/visit_stmt.rs (narrows the condition for injecting a class name on export default class), and test/bundler/transpiler/es-decorators.test.ts (12 new tests). The change reroutes classes that contain accessor members but zero decorators away from the full relocation machinery into an in-place rewrite that keeps every class element in source order, and it fixes computed accessor keys evaluating twice on both paths.
Security risks
None identified. This is transpiler output-shape logic with no auth, crypto, network, filesystem, or permission surface. The risk profile is correctness of emitted JavaScript, not security.
Level of scrutiny
Moderate-to-high. This is core transpiler code: a bug here silently produces wrong JS for any user code with accessor fields (wrong evaluation order, private-name collisions/shadowing, duplicate computed-key evaluation). The new path includes hand-rolled private-name uniqueness logic (iterating the full symbol table as an over-approximation of names to avoid), new symbol creation with specific PrivateField/PrivateStaticField kinds, and statement-list manipulation that differs between class-statement and class-expression forms. The visit_stmt.rs change drops should_lower_standard_decorators from the name-injection guard, relying on has_decorators alone — a reviewer familiar with how that flag is set (class-level vs. member-level decorators, legacy vs. standard) should confirm it still covers every case that reaches the full lower_impl machinery and needs an injected name.
Other factors
Test coverage is strong (12 targeted tests covering the issue repros, ordering, private-name scoping, collision avoidance, subclass override, computed keys on both paths, and the .name === "default" case), the PR description states 8 of 12 fail on an unfixed build, and existing decorator/transpiler suites pass. The one CI failure (bunx.test.ts) is a known fleet-wide flake unrelated to this change. The bug-hunting system found nothing. Still, the scope and the fact that this rewrites how a language feature is lowered put it outside what I'd approve without a human reviewer.
…ments A class with accessor members but no decorators has nothing to run after class creation, yet lower_impl still extracted its static blocks and moved static accessor initializers into the module-level suffix chain. That carried native #name references out of the class body (SyntaxError: Cannot reference undeclared private names) and lost the source order of static elements. Such classes now take an in-place path: each accessor becomes a private backing field plus a getter/setter pair (the same shape esbuild emits), and every other class element is left untouched. Backing names are made unique against every private name in the file so they cannot duplicate a declaration or capture an enclosing class's private name. Computed accessor keys now evaluate exactly once (assigned to a temporary in the getter's key position, reused for the setter) on both the in-place path and the decorator-machinery path; previously the key expression was duplicated into both the getter and the setter. export default classes with only accessor fields stay anonymous, keeping .name === "default"; the synthesized default name is only injected when the class actually has decorators. Fixes #31921
…-name, instance private accessor and output shape cases
006420f to
33dfb96
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Branch housekeeping: a previous push merged an older copy of this branch into newer work, and GitHub's compare fell back to a stale merge base, showing ~11.7k changed files. That phantom diff is what the comment-cop review ran against; all 100 of its comments target files this PR does not touch (the real diff is 4 files). I have linearized the branch onto current main (same content, verified: es-decorators 75 pass, es-decorators-esbuild 147 pass, decorators 24 pass), the compare now shows the true 4-file diff, and I am resolving those threads as not applicable to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/lower/lower_decorators.rs`:
- Around line 1222-1229: Avoid rebuilding taken_private_names by scanning all
p.symbols in each lower_auto_accessors_in_place call. Build and retain the
private-name set once per parse, then add every generated backing name as
classes are lowered; preserve the current over-approximation so generated names
remain unique across classes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a95ec35-111b-4d3e-8ed5-75d1686173cb
📒 Files selected for processing (4)
src/ast/symbol.rssrc/js_parser/lower/lower_decorators.rssrc/js_parser/visit/visit_stmt.rstest/bundler/transpiler/es-decorators.test.ts
The set of in-use private names was rebuilt by scanning the whole symbol table for every accessor-only class, making files with many such classes pay O(classes x symbols). Cache it on the parser after the first scan and extend it with each generated backing name instead.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It adds a new ~230-line in-place lowering path in the JS transpiler and refactors the decorator path's accessor emission, so a human look would still be worthwhile.
What was reviewed:
- Verified
has_decoratorscovers class-level, member-level, and argument decorators (parse/mod.rs:264), so the narrowed name-injection condition invisit_stmt.rsstill fires for every decoratedexport default classand only skips accessor-only classes. - Checked
taken_private_names: built once from all parse-time private symbols, extended per generated backing name, and re-stored onp— over-approximates safely across nested/sibling classes and avoids rescanning. - Confirmed the decorator path's new computed-key temporary reuses the existing
computed_key_counterat lower_decorators.rs:1553, so_computedKeynames stay unique within a class that mixes decorated computed keys and undecorated computed accessors. push_accessor_get_set_pairis a straight extraction of the previous inline getter/setter construction; the only behavioural change on the decorator path is the single-evaluation computed key, which has a dedicated test.
Extended reasoning...
Overview
This PR fixes #29837 and #31921 by adding an in-place lowering path for classes that use accessor members but have no decorators. It touches five files: src/js_parser/lower/lower_decorators.rs (new lower_auto_accessors_in_place ~230 lines, extracted push_accessor_get_set_pair helper, computed-key single-evaluation for the decorator path), src/js_parser/p.rs (new taken_private_names: Option<HashMap<&'a [u8], ()>> parser field), src/js_parser/visit/visit_stmt.rs (narrowed name-injection condition for export default class), src/ast/symbol.rs (Kind::is_private widened to pub), and 16 new tests in es-decorators.test.ts.
Security risks
None. This is transpiler output-shape logic; no untrusted input parsing beyond what the JS lexer already handles, no I/O, no crypto/auth.
Level of scrutiny
Medium-high. The transpiler runs on every JS/TS file Bun processes, and the change introduces a new code path plus refactors an existing one. The description is thorough, the test matrix is broad (16 tests covering ordering, private-name collisions in three shapes, computed keys on both paths, private accessors, subclass override, and export default naming), and 10 of 16 fail on the unfixed build per the description. I verified the key invariants (has_decorators semantics, computed_key_counter sharing, taken_private_names lifecycle, nearest_stmt_list_mut usage matching existing patterns) and found nothing wrong, but the size and criticality put it outside the auto-approve envelope.
Other factors
- No prior human review on the thread; the ~100 comment-cop inline comments were against a phantom 11.7k-file diff from a stale merge base and have been resolved as inapplicable (per the robobun housekeeping note).
- The decorator-path refactor into
push_accessor_get_set_pairis behaviour-preserving except for the computed-key single-evaluation fix, which is intentional and tested. - The
is_privatevisibility widening is a revert to a previously-public state and is used only by the new path.
|
Status check against #38734, which replaced #31930 as the fix for the temporary name collisions (#31930 is closed):
|
Fixes #29837
Fixes #31921
Problem
accessormembers but has no decorators is still run through the full standard-decorator lowering (should_lower_standard_decoratorsis set foraccessoralone), and that lowering relocates code out of the class body. Three user-visible breakages follow:accessor namelowered to the same module-levelvar _name = new WeakMap, so constructing the subclass threwTypeError: Cannot add the same private member more than once.#namereference inside them ended up outside any class body (SyntaxError: Cannot reference undeclared private names: "#m"), and the relocated initializers always ran before every static block, sostatic accessor a; static { } static accessor bevaluated asa, b, blockinstead ofa, block, b.c = this.bsaw an uninitializedb).lower_implinsrc/js_parser/lower/lower_decorators.rshas nothing to do after class creation when there are no decorators, but its relocation machinery ran anyway;lower_all_privatestays false without decorators, so the relocated code kept native private names.Fix
Classes with no class decorators and no member decorators take a new in-place path,
lower_auto_accessors_in_place: eachaccessoris replaced, at its own position, by a private backing field plus a getter/setter pair, and every other element is left alone. This is the shape the proposal's desugaring and esbuild use:Why it is correct: nothing leaves the class body, so private names stay in scope, static elements keep source order, instance initializers interleave with plain fields in source order, and the backing storage is a real per-class private name, so a subclass override or a same-named accessor in another class cannot collide and
super.namestill reaches the base accessor. Backing names are chosen to be unique against every private name in the file (#x/#x2), which covers a user-declared#xin the same class, an instance and a static accessor sharing a name, and an enclosing class's#xreferenced from inside the class body.accessor #pkeeps itsget #p/set #ppair and stores in#_p; non-identifier keys get#_accessor_storageN.Two adjacent fixes that fall out of the same code:
accessor [key()]) were duplicated into the getter and the setter, so the key expression ran twice and could define two different properties. Both the in-place path and the decorator path now evaluate it once (get [_computedKey = key()]/set [_computedKey]), through a sharedpush_accessor_get_set_pairhelper that also removes the duplicated getter/setter construction from the decorator path.export default class { accessor a }no longer gets a synthesized class name, since only the relocating path needed one, so.namestays"default".symbol::Kind::is_privateis madepubagain (it was narrowed topub(crate)on main while unused outsidebun_ast); the in-place path uses it to collect the taken private names.Decorated classes are intentionally unchanged here. The related bugs there are split out: Give decorator lowering temporaries file-unique names #31930 gives the decorator lowering's module-level temporaries file-unique names (the
_init/_dec/_nameWeakMap collisions between decorated classes, which is also what the decorated-getter variant in the Class auto-accessor in subclass causes "Cannot add the same private member more than once" error #29837 comment thread hits), and js_parser: lower undecorated auto-accessors to a native #-private storage field #35708 keeps undecorated accessors and lowered privates on the class-body timeline inside decorated classes. Its cases for undecorated classes are covered by this PR, and the ones that add coverage were folded in here.Verification:
test/bundler/transpiler/es-decorators.test.ts,auto-accessor without decorators(16 tests: the Accessor-only class lowering relocates static initializers out of class scope (private name SyntaxError, wrong evaluation order) #31921 and Class auto-accessor in subclass causes "Cannot add the same private member more than once" error #29837 repros, static and instance ordering against static blocks, static fields and plain fields, private references from static blocks and initializers, static and instance private accessors, name uniqueness in the three collision shapes, computed keys on both paths, theexport defaultname, and an inline snapshot of the emitted shape). 10 of the 16 fail on the unfixed build; all pass with it. With this branch on top of currentmain,es-decorators(75),es-decorators-esbuild(147),decorators,decorator-metadata,bundler_decorator_metadata,transpiler.test.js,esbuild/ts,esbuild/defaultandesbuild/lowerall pass with a debug build, and the repro from the issue printsB/A.Background
accessor x = v(from the decorators proposal) declares a getter/setter pair over an implicit private slot. JavaScriptCore does not implement the keyword, so Bun's transpiler lowers it; the proposal defines it as exactly the#x+get/setdesugaring above.lower_impl) has to run decorator functions after the class object exists, so it pulls static initializers and static blocks out into statements that follow the class and, when decorators touch private members, rewrites every#nameinto aWeakMap/WeakSetlookup. Those two steps are what make relocation safe; an undecorated class gets neither the need for relocation nor the private-name rewriting.#xin classAand#xin classBare different names, which is why backing storage as a private field fixes the subclass case, while a module-scopeWeakMapnamed after the key cannot.Earlier description
The previous revision of this description covered the same change with fewer tests (12). While consolidating with #35708 the branch was brought up to date with
main(which needed theis_privatevisibility change) and gained the static-field ordering, instance/static shared-name, instance private accessor and emitted-shape cases. The remaining gaps in the decorated path that the earlier text mentioned (#31405 for private references in relocated code, #31922 forthis/ inner-name substitution) are still separate PRs.[review] gate passed · iteration 10 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 10
evidence per changed file
root cause · written by the author bot
The root cause was that the accessor-only lowering path reused the decorator relocation machinery, which moved static accessor initializers and static blocks out of the class body into a module-level statement chain; since no decorators were present, private members were never WeakMap-lowered, so the relocated code retained native #name references that are illegal outside a class and the initializers were emitted ahead of static blocks regardless of source order. The fix stops relocating entirely for classes with accessors but no decorators and instead lowers each accessor in place, replaci…