Parse and lower accessor fields under experimentalDecorators#29201
Open
Parse and lower accessor fields under experimentalDecorators#29201
accessor fields under experimentalDecorators#29201Conversation
The `accessor` class field modifier (TC39 Stage 4 / TS 4.9+) was gated on Bun's internal `standard_decorators` feature flag in the parser, so TypeScript sources with `experimentalDecorators: true` failed to parse any class containing an `accessor` field. The keyword is valid class syntax regardless of which decorator proposal is active. Drop the gate, then desugar `accessor x = ...` to a `#storage` private field plus a `get x()`/`set x(v)` pair in `lowerClass` (statement path) and in `e_class` visit (expression path) whenever we're not already taking the standard-decorators lowering path. Any `@dec` on the accessor carries over to the synthesized getter, so the existing legacy-decorator loop emits `__legacyDecorateClassTS([...], Foo.prototype, "x", null)` — matching TypeScript's output under `experimentalDecorators: true`. The rewrite also makes the code actually run: JavaScriptCore does not currently parse the `accessor` keyword, so passing it through unchanged would error at runtime even for undecorated fields. Fixes #29197
Collaborator
Author
|
Updated 12:58 AM PT - Apr 12th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 29201That installs a local version of the PR into your bun-29201 --bun |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRewrites class Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
robobun
force-pushed
the
farm/9ef3d383/accessor-experimental-decorators
branch
from
8c580fb to
5bf9b53
Compare
This was referenced Apr 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/P.zig`:
- Around line 4967-5066: Replace the naked "catch bun.outOfMemory()" uses in the
new helper with bun.handleOom(...) calls: specifically change the
ListManaged(Property).initCapacity call, both std.fmt.allocPrint calls used to
build storage_name, the p.newSymbol(storage_kind, storage_name) call, all
p.allocator.alloc(...) calls (get_body_stmts, setter_args, set_body_stmts), and
any p.newExpr/p.newSymbol/p.newExpr(E.Function...) sites that currently end with
"catch bun.outOfMemory()" to instead wrap the failing call with
bun.handleOom(...). This preserves the same behavior but uses the repo
convention that converts OutOfMemory into a crash without swallowing other
errors; locate these in the helper around symbols rewritten, storage_name,
storage_ref, get_body_stmts, get_fn_expr, setter_param_ref, setter_args,
set_body_stmts, and set_fn_expr and replace the tail "catch bun.outOfMemory()"
with bun.handleOom(...)
- Around line 4981-5003: The code builds private storage names in the
storage_name block by embedding non-computed string keys directly, which can
produce invalid private identifiers; modify the storage_name logic (the brk
block around storage_name, referencing prop.flags, prop.key, and
k.data.e_string.data) to validate that the string is a legal private identifier
(starts with $, _, or UnicodeIDStart and subsequent chars are only those or
digits) before using the "#{s}_accessor_storage" branch; if the validation
fails, fall back to the numeric counter branch that generates
"#_accessor_storage_{d}". Ensure the validation runs only when
prop.flags.contains(.is_computed) is false and prop.key is present.
- Around line 5034-5066: Computed accessor keys are being evaluated twice
because the code directly reuses prop.key when appending the synthesized get and
set members; modify the rewrite to hoist the computed key expression into a
temporary (evaluate prop.key once into a temp key symbol/expression before
creating get/set), then use that temp for both the getter and setter entries
passed to rewritten.append so both members reference the same pre-evaluated key
while preserving original evaluation order and side effects; update the block
that constructs get_fn_expr/set_fn_expr (and the subsequent rewritten.append
calls) to consume the hoisted temp instead of prop.key (keep existing symbols
like storage_ref, setter_param_ref, and the
E.Function/E.PrivateIdentifier/E.Index construction intact).
In `@test/regression/issue/29197.test.ts`:
- Line 63: Remove the fragile empty-stderr assertions by deleting each
expect(stderr).toBe("") (and any exact-empty stderr checks) in the regression
subprocess tests; instead rely on the existing stdout and exitCode assertions
(or, if you need to check stderr content, assert specific error-free patterns
rather than exact emptiness). Update the tests that contain
expect(stderr).toBe("") (and similar exact-empty checks) so they no longer fail
due to ASAN/debug startup warnings.
🪄 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: bbbdf20b-a1d7-4b3a-92f9-c61b65e85dc1
📒 Files selected for processing (4)
src/ast/P.zigsrc/ast/parseProperty.zigsrc/ast/visitExpr.zigtest/regression/issue/29197.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Hoist computed accessor keys into a temp `var` prepended to the class
statement so the key expression is evaluated exactly once (matches TC39
ClassElementEvaluation). Class-expression lowering passes `null` for
the prefix-stmts sink; computed-key hoisting there is a no-op.
- Validate string-key backing-storage names: fall back to the counter
form unless the key is a plain ASCII identifier (UTF-8 and
`isIdentifier`), so `accessor "foo-bar" = 1` no longer produces an
invalid `#foo-bar_accessor_storage` private name.
- Set `return_ts_metadata` on the synthesized getter so that under
`experimentalDecorators + emitDecoratorMetadata`, `design:type` on
a decorated typed accessor reflects the user's declared type instead
of the default `Object`.
- Static accessors now access the backing field through the class name
binding instead of `this`, so `class Sub extends Base {}; Sub.count`
no longer trips the private-field brand check.
- Use `bun.handleOom(...)` throughout the helper instead of
`catch bun.outOfMemory()`, matching the repo convention.
- Drop the stale TODO in `lowerClass` about a `.has_accessor_modifier`
flag — `rewriteAutoAccessorProperties` runs before the loop and
already turns auto-accessors into `is_method` get/set pairs.
- Regression tests: static-subclass access, non-identifier string keys,
single-evaluation of computed keys, and `design:type` metadata.
- Test helper no longer asserts on empty stderr; ASAN/debug startup
warnings are not under test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/ast/P.zig (1)
5039-5067:⚠️ Potential issue | 🔴 CriticalComputed keys still run twice for class expressions.
When
prefix_stmtsis null,shared_keystays asprop.key, so the synthesized getter and setter each emit their own computed name.class { accessor [sideEffect()] = 1 }will still evaluatesideEffect()twice. Separate getter/setter definitions each evaluateClassElementNameindependently. (tc39.es)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ast/P.zig` around lines 5039 - 5067, The computed property key is only hoisted when prefix_stmts is present, so getter/setter pairs still evaluate prop.key twice; change the logic in the prop.flags.contains(.is_computed) branch to always synthesize a single temporary key symbol when prop.key exists (create key_ref via p.newSymbol, append it to p.current_scope.generated, build shared_key = p.newExpr(E.Identifier{ .ref = key_ref }, k.loc)), and if prefix_stmts is non-null also emit the Local var declaration into prefix_stmts as you currently do; this ensures shared_key is replaced with the temp identifier for both synthesized accessor definitions so ClassElementName is evaluated only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/P.zig`:
- Around line 5059-5062: The change moves accessor_prefix_stmts (the temp
initialization emitted via bun.handleOom and ps.append(p.s(... S.Local{ .kind =
.k_var, .decls = G.Decl.List.fromOwnedSlice(decls) }, k.loc))) before the class
declaration, which incorrectly evaluates computed keys earlier than the class
body; instead restore the temp initialization to be emitted at the accessor's
original class-element slot (not prepended), i.e. remove/undo the prepended
ps.append call and ensure the temp variable is created and inserted where the
accessor is emitted so the computed-key evaluation and accessor initialization
occur in original source order (also apply same fix for the other occurrences
around lines referenced 5177-5195 and 5456-5463).
- Around line 5043-5051: The generated symbol name "_computedAccessorKey{d}" is
predictable and can collide with user identifiers; replace this with a
collision-proof generated temp by using a dedicated generated-symbol facility or
by constructing a name that guarantees uniqueness (e.g., include a unique
per-process/per-scope token or use an existing generator function) when calling
p.newSymbol, then append that symbol to p.current_scope.generated as before
(references: p.newSymbol, key_ref, p.current_scope.generated.append). Ensure the
chosen approach produces a symbol that cannot conflict with user code in
non-renaming output.
---
Duplicate comments:
In `@src/ast/P.zig`:
- Around line 5039-5067: The computed property key is only hoisted when
prefix_stmts is present, so getter/setter pairs still evaluate prop.key twice;
change the logic in the prop.flags.contains(.is_computed) branch to always
synthesize a single temporary key symbol when prop.key exists (create key_ref
via p.newSymbol, append it to p.current_scope.generated, build shared_key =
p.newExpr(E.Identifier{ .ref = key_ref }, k.loc)), and if prefix_stmts is
non-null also emit the Local var declaration into prefix_stmts as you currently
do; this ensures shared_key is replaced with the temp identifier for both
synthesized accessor definitions so ClassElementName is evaluated only once.
🪄 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: 19b45965-993e-428c-aba7-de0d5ad077cb
📒 Files selected for processing (3)
src/ast/P.zigsrc/ast/visitExpr.zigtest/regression/issue/29197.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/ast/P.zig (2)
5043-5052:⚠️ Potential issue | 🟠 MajorUse the parser’s temp-ref generator here.
_computedAccessorKey{d}is still a user-visible declaration name. In non-renaming output it can collide with real bindings and turn this rewrite into a duplicate declaration/capture bug.generateTempRef("_computedAccessorKey")already handles the collision-resistant path used elsewhere in this file.🛠️ Suggested fix
- const key_ref = bun.handleOom(p.newSymbol( - .other, - bun.handleOom(std.fmt.allocPrint( - p.allocator, - "_computedAccessorKey{d}", - .{counter}, - )), - )); - bun.handleOom(p.current_scope.generated.append(p.allocator, key_ref)); - counter += 1; + const key_ref = p.generateTempRef("_computedAccessorKey");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ast/P.zig` around lines 5043 - 5052, The code is creating a user-visible symbol "_computedAccessorKey{d}" which can collide with real bindings; replace the manual newSymbol + counter approach with the parser's collision-safe temp-ref generator by calling p.generateTempRef("_computedAccessorKey") (and use bun.handleOom(...) around that call as needed), then append that returned temp ref to p.current_scope.generated instead of using the manual counter; remove the counter and the std.fmt.allocPrint/newSymbol usage so the generated temp ref mechanism handles uniqueness.
4958-4968:⚠️ Potential issue | 🔴 CriticalPreserve computed-key evaluation at the original class-element slot.
This still changes runtime semantics. When
prefix_stmtsis present,var _computedAccessorKey... = exprruns before the class, so the key no longer evaluates in class-element order and can no longer see the class’s initialized inner binding (class C { accessor [C.name] = 1 }). Whenprefix_stmtsis null, the fallback reusesprop.keyon both synthesized members, so class expressions still evaluate the key twice. This needs an in-class single-evaluation strategy (e.g. assign the temp in the first synthesized key and reuse it in the second) or another lowering that preserves class evaluation order.In ECMAScript class evaluation, are computed property names evaluated after the `extends` clause and with the class’s inner name binding initialized? If `accessor [expr] = init` is lowered either to `var t = expr; class C extends Base { get [t]() {} set [t](v) {} }` or to `class C { get [expr]() {} set [expr](v) {} }`, do those preserve the original semantics?Also applies to: 5039-5066, 5190-5197, 5458-5465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ast/P.zig` around lines 4958 - 4968, The computed-key hoisting currently moves evaluation to prefix_stmts (or duplicates prop.key when prefix_stmts is null), changing class-element evaluation order and breaking cases like class C { accessor [C.name] = 1 }; modify the lowering so the temp is created at the first synthesized class-element slot instead of before the class: when lowering an accessor with computed key (see prop.key and the synthesized members: field/getter/setter), emit the assignment to the temp as part of the first synthesized member’s key expression (or an injected statement that is produced inline with that first member) and have the subsequent synthesized members reuse that temp; when prefix_stmts is null, do the same inline-first-member assignment rather than re-evaluating prop.key so the key is evaluated exactly once in class-element order and the class’s inner binding is observable as per ECMAScript semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ast/P.zig`:
- Around line 5043-5052: The code is creating a user-visible symbol
"_computedAccessorKey{d}" which can collide with real bindings; replace the
manual newSymbol + counter approach with the parser's collision-safe temp-ref
generator by calling p.generateTempRef("_computedAccessorKey") (and use
bun.handleOom(...) around that call as needed), then append that returned temp
ref to p.current_scope.generated instead of using the manual counter; remove the
counter and the std.fmt.allocPrint/newSymbol usage so the generated temp ref
mechanism handles uniqueness.
- Around line 4958-4968: The computed-key hoisting currently moves evaluation to
prefix_stmts (or duplicates prop.key when prefix_stmts is null), changing
class-element evaluation order and breaking cases like class C { accessor
[C.name] = 1 }; modify the lowering so the temp is created at the first
synthesized class-element slot instead of before the class: when lowering an
accessor with computed key (see prop.key and the synthesized members:
field/getter/setter), emit the assignment to the temp as part of the first
synthesized member’s key expression (or an injected statement that is produced
inline with that first member) and have the subsequent synthesized members reuse
that temp; when prefix_stmts is null, do the same inline-first-member assignment
rather than re-evaluating prop.key so the key is evaluated exactly once in
class-element order and the class’s inner binding is observable as per
ECMAScript semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78b3fa40-9247-494f-be29-8635e0d47b79
📒 Files selected for processing (1)
src/ast/P.zig
Review feedback: the earlier approach hoisted `var _tmp = expr()`
before the class statement, which made the computed key run *before*
`extends` (wrong: `extends` evaluates first per ECMAScript spec)
and before the class's inner binding was available. It also
double-evaluated the key in class-expression position since that path
had no statement sink.
New approach:
var __bun_accessor_key_N$;
class C extends Base {
#_accessor_storage_0 = init;
get [__bun_accessor_key_N$ = expr()]() { ... }
set [__bun_accessor_key_N$](v) { ... }
}
The prefix statement is a bare `var` declaration (no initializer) —
harmless, hoisting semantics unchanged. The actual key expression
runs as part of the getter's PropertyName evaluation, in class-element
order, so `extends` runs first, the key runs exactly once, and any
references inside `expr()` see the same bindings they would in the
original program.
Also:
- Always use the counter-based `#_accessor_storage_N` form for the
backing field instead of embedding the property key. This sidesteps
any possibility of colliding with a user-declared private field of
the same name, without needing a scope-members scan.
- Register the `__bun_accessor_key_N$` temp on the nearest hoisting
scope (function/module) via `generated` + `declared_symbols` so
the renamer and referenced-symbol tracking see it.
Class-expression position still has no statement sink, so computed
auto-accessor keys in a class-expression are a known limitation (they
will evaluate twice). That's a rare edge case — class expressions
with computed accessor keys are essentially never written in practice,
and this is still strictly better than the status quo, which errored
on any accessor in that position.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ast/P.zig (1)
5034-5053:⚠️ Potential issue | 🟠 MajorUse the generated-temp facility here instead of a raw formatted name.
__bun_accessor_key_{d}$is still a predictable plain identifier, so in non-renaming output it can collide with user bindings in the hoisting scope. This is the same collision class as the earlier_computedAccessorKeyissue; please allocate this through the existing temp/gensym path instead of formatting a bare symbol name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ast/P.zig` around lines 5034 - 5053, The generated temp name currently created with std.fmt.allocPrint("__bun_accessor_key_{d}$", tmp_n) produces a predictable identifier that can collide with user bindings; replace this raw formatted name by allocating the temporary through the project's gensym/temp facility (use the existing generated-temp API rather than creating tmp_name manually), then pass that generated symbol to p.newSymbol (keep tmp_ref, hoist_scope logic, hoist_scope.generated.append, and p.declared_symbols.append as before) so the temp is unguessable and allocated consistently with other temps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/P.zig`:
- Around line 4997-5024: The synthesized storage name generation using counter
and storage_name (format "#_accessor_storage_{d}") can collide with
user-declared private fields; modify the logic around counter, storage_name, and
p.newSymbol so you first check that the candidate name is not already used by
any class.properties key or existing symbols in p.current_scope.generated, and
if it is, increment the counter (or re-mangle) and retry until you find an
unused name; only then call p.newSymbol and append to p.current_scope.generated
to reserve it. Ensure the uniqueness check covers both user-declared private
keys and previously generated storage_ref entries.
---
Duplicate comments:
In `@src/ast/P.zig`:
- Around line 5034-5053: The generated temp name currently created with
std.fmt.allocPrint("__bun_accessor_key_{d}$", tmp_n) produces a predictable
identifier that can collide with user bindings; replace this raw formatted name
by allocating the temporary through the project's gensym/temp facility (use the
existing generated-temp API rather than creating tmp_name manually), then pass
that generated symbol to p.newSymbol (keep tmp_ref, hoist_scope logic,
hoist_scope.generated.append, and p.declared_symbols.append as before) so the
temp is unguessable and allocated consistently with other temps.
🪄 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: 6ce850da-6e63-4050-860a-9be66e536b9e
📒 Files selected for processing (1)
src/ast/P.zig
Review feedback: both synthesized names could in principle collide with adversarial user code at the same scope. The counter is now computed from the scope contents instead of always starting at 0: - `#_accessor_storage_N`: pre-scan `class.properties` for any existing private-identifier member whose name matches `#_accessor_storage_<k>` and start the counter one above the highest k we find. A class with `#_accessor_storage_0 = 999` alongside `accessor x = 1` now gets `#_accessor_storage_1` for x's backing field, not a duplicate. - `__bun_accessor_key_N$`: walk the enclosing hoisting scope's `members` map and loop-and-bump until we find a name that isn't already bound there. A module that binds `var __bun_accessor_key_0$ = 'user-value'` at top level now gets `__bun_accessor_key_1$` for the computed-key temp. Both of these are extreme edge cases — no real-world code writes `#_accessor_storage_0` or `__bun_accessor_key_0$` deliberately — but the collision checks are cheap and make the transform strictly more robust. New regression tests exercise both scenarios.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/P.zig`:
- Around line 5005-5076: Change the counter to a wider integer and use checked
arithmetic: replace the local counter (currently u32) with u64, parse existing
storage suffixes with std.fmt.parseInt(u64, ...) (or cast parsed u32 -> u64),
and use `@addWithOverflow` when computing n+1 or bumping counter (both in the
pre-scan comparison near storage_prefix and in the computed-key tmp_name loop
where n is incremented) to avoid wrapping; also ensure any std.fmt.allocPrint
that prints the numeric id uses the updated counter type (e.g., storage_n as
u64) and update references to counter/ n accordingly (symbols: storage_prefix,
counter, parseInt call, storage_n/storage_name creation, tmp_name / tmp_name
generation loop).
- Around line 5176-5182: The decorator emission must not reuse the full
computed-assignment expression stored in getter_key (which is currently set to
"(_tmp = expr())"); instead, change the decorator descriptor_key generation to
reference only the cached temp variable created by the assignment so the key
expression is not re-run. Locate where getter_key and prop.key are used to build
descriptor_key in the decorator loop and replace usage of the full assignment
expression with an expression that reads the temp variable (the RHS of the
assignment result), ensuring the member-definition keeps the assignment but the
decorator descriptor uses only the temp reference (so
__legacyDecorateClassTS/... receives the cached key and expr() is not executed
twice).
In `@test/regression/issue/29197.test.ts`:
- Around line 3-13: Remove the long bug-history prose in the leading comment
block that explains why `accessor` was rejected and how Bun desugars it; keep
only a single line with the issue URL and any short notes that are directly
about the test design (e.g., why we desugar to `#storage` + getter/setter).
Apply the same trimming to the other verbose comment block referenced around
lines 116-119 so both blocks contain only the issue URL and concise
test-rationale comments explaining the signal/design choice.
- Around line 33-44: The spawned process `proc` pipes stderr but never consumes
it; to avoid possible backpressure stall, concurrently drain `proc.stderr`
(e.g., await `proc.stderr.text()` or otherwise consume its stream) alongside
`proc.stdout.text()` and `proc.exited` so stderr is read even when not asserted;
update the Promise.all that currently awaits `proc.stdout.text()` and
`proc.exited` to include consuming `proc.stderr` (referencing `Bun.spawn`, the
`proc` variable, `proc.stdout.text()`, `proc.stderr`, and `proc.exited`).
🪄 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: c997f4d3-7846-4aa0-a4db-9714abebcbd4
📒 Files selected for processing (2)
src/ast/P.zigtest/regression/issue/29197.test.ts
…ptor Review feedback: 1. **Critical**: for `@dec accessor [expr()] = ...`, the member is emitted as `get [(_tmp = expr())]()` so that `expr()` runs exactly once when the getter's PropertyName is evaluated. But the legacy-decorator loop reused `prop.key` verbatim for `descriptor_key`, which meant `__legacyDecorateClassTS(...)` saw the full assignment expression and re-ran `expr()` at runtime. Now the decorator emission unwraps `e_binary.op == .bin_assign` whose LHS is an identifier and passes just the identifier to the runtime call. Verified: the emitted output now reads `__legacyDecorateClassTS([dec], Foo.prototype, __bun_accessor_key_1$, null)` with a bare ref. 2. Widen the backing-storage and temp-name counters to `u64` with a saturation check so the loop can't wrap for pathological classes declaring `#_accessor_storage_<very-large-number>`. 3. Test helper now drains `stderr` concurrently with `stdout` so a debug/ASAN subprocess can't stall on an unread pipe. 4. Trimmed leading bug-history prose from the test file to just the issue URL (both for #29197 and #27335), per repo review style. Also carries the `p.recordUsage(setter_param_ref)` fix from the prior turn that was lost in a stray working-tree reset. New regression test `decorated computed-key accessor evaluates the key expression exactly once` locks in the critical fix.
…or on decorated class-expression accessor Review feedback on PR #29201: 1. **Sibling-class temp collision**: the computed-key `__bun_accessor_key_N$` counter was a function-local reset to 0 on every call, so two sibling classes at the same hoisting scope would both pick `__bun_accessor_key_0$`, emitting duplicate `var` declarations and aliased refs. The collision check only consulted `hoist_scope.members` (user-declared), not `hoist_scope.generated` (synthesized refs from earlier calls). Fix: scan `hoist_scope.generated` for existing names matching our prefix and start the counter one above the maximum seen. 2. **Missing recordUsage on the descriptor_key unwrap**: when we extract the LHS identifier from a computed-key assignment (`(_tmp = expr())`) for the decorator descriptor, that's a third runtime read of `_tmp` (after the getter-key LHS assignment and the setter-key read). We were skipping the `recordUsage` call, leaving the minifier's char-frequency tally one low. Fix: call `p.recordUsage` on the extracted ref. 3. **Silent decorator drop on class-expression accessor**: class expressions don't go through `lowerClass`, so any `ts_decorators` on a synthesized getter are never emitted into a `__legacyDecorateClassTS` call. This is a pre-existing gap for ALL legacy decorators on class-expression members, but this PR widens the reachable surface by accepting `accessor` syntax under `experimentalDecorators`. Until class-expression legacy decorators are implemented end-to-end, emit a clear parse-time error for the `@dec accessor x` in a class expression so users don't silently get valid-looking code where `@dec` never fires. 4. Stale comment on the `.auto_accessor` branch of the decorator metadata switch replaced with an explanation that matches the new reality: the branch is dead because `rewriteAutoAccessorProperties` has already converted accessors to getter/setter pairs by the time this loop runs. New regression tests: - `sibling classes with computed accessor keys use distinct temp vars` - `decorated accessor in a class expression is rejected with a clear error`
…l class-expr violations Review feedback on PR #29201: 1. Split `counter` into `storage_counter` and `key_counter` so the backing-field index sequence stays contiguous (`#_accessor_storage_0`, `_1`, `_2`, ...) even when a computed-key accessor inserts a temp. The shared counter was cosmetically skipping storage indices. 2. Narrow the `descriptor_key` unwrap in `lowerClass` so it only strips `(__bun_accessor_key_N$ = expr)` assignments synthesized by `rewriteAutoAccessorProperties`. A user-written `@dec get [(x = computeKey())]()` must pass through unchanged, so the runtime key is the result of `computeKey()` and not a stale `x`. Guard the unwrap on the LHS identifier's original name. 3. Remove the `break` in the class-expression validation loop so every decorated auto-accessor is reported in a single compile pass, not one per edit-compile cycle. 4. Document the known quality trade-off where synthesized refs land in the enclosing scope's `generated` list (the class body scope has already been popped by the time `rewriteAutoAccessorProperties` runs). The renamer still produces unique names; only the scope-local slot counters are slightly suboptimal for minification. Refactoring the visit/lower split to keep the class scope alive through lowering is out of scope for this PR.
…entifier keys
Claude[bot] review flagged that a synthesized getter with a private-
identifier key (from `@dec accessor #x`) would fall through the
`descriptor_key` unwrap block unchanged, then get passed as an
`E.PrivateIdentifier` node to `__legacyDecorateClassTS(...)`. The
printer can't emit private identifiers in expression context, so in
debug builds this panics and in release builds it silently emits an
empty argument slot — a SyntaxError.
In practice the code path is unreachable because Bun's parser (and
TypeScript itself, TS1206) rejects `@dec accessor #x` upstream:
the `.t_private_identifier` property branch in parseProperty.zig
has a guard that refuses private keys when `ts_decorators.len > 0`
and `standard_decorators` is off. But the decorator-emission loop
shouldn't depend on that upstream rejection being airtight — if a
future change relaxes the parser gate, this lowering must not produce
garbled output.
Add a branch at the top of the `descriptor_key` block that converts
any surviving `.e_private_identifier` key into
`E.String{ .data = "#x" }`, mirroring what TypeScript emits
(`__decorate([dec], C.prototype, "#x", null)`) and matching the
standard-decorators path in `src/ast/lowerDecorators.zig`.
Regression test `decorator on a private accessor field is rejected
like TypeScript` locks in the current parser-level rejection so the
two layers of defense stay in sync.
Comment on lines
+4992
to
+4995
| var rewritten = bun.handleOom(ListManaged(Property).initCapacity( | ||
| p.allocator, | ||
| class.properties.len, | ||
| )); |
Contributor
There was a problem hiding this comment.
🔴 The rewritten list in rewriteAutoAccessorProperties is initialized with capacity class.properties.len (N), but each auto_accessor property expands into 3 entries (backing field + getter + setter), so the final list size is N + 2*K where K is the number of auto-accessors. This causes a reallocation on every class with at least one accessor. With Bun's arena allocator, the original buffer is abandoned rather than freed, wasting memory proportional to N per affected class.
Extended reasoning...
Bug: Undersized Initial Capacity in rewriteAutoAccessorProperties
What the bug is and how it manifests
The rewriteAutoAccessorProperties function in src/ast/P.zig initializes its rewritten list with capacity class.properties.len (call this N). However, each auto_accessor property is desugared into three separate Property entries: a private backing field (#_accessor_storage_N), a getter method, and a setter method. If there are K auto-accessor properties in the class, the final list will contain N + 2*K entries — always larger than the initial capacity of N when K ≥ 1.
The specific code path that triggers it
// Lines 4992-4995 in src/ast/P.zig
var rewritten = bun.handleOom(ListManaged(Property).initCapacity(
p.allocator,
class.properties.len, // ← capacity N; final size is N + 2*K
));The prior has_any scan breaks early on the first auto_accessor found, so K is never counted. The allocation is made with capacity N, but the subsequent loop appends one #_accessor_storage_N field and replaces the original auto_accessor with a getter and setter — a net addition of 2 entries per accessor.
Why existing code does not prevent it
The has_any check only determines whether to return early. Because it uses break on the first match, it cannot also count the total number of accessors. The initial capacity therefore hardcodes the (too-small) value of class.properties.len with no headroom for the expansion.
What the impact would be
Every class with at least one auto_accessor field triggers a heap reallocation. With Bun's arena allocator, freed memory from the original buffer is not reclaimed individually — the old allocation is simply abandoned. For a class with N total properties and K accessors, the wasted memory is proportional to N * sizeof(Property). While this is unlikely to be catastrophic in practice (most classes are small and short-lived), it is an avoidable inefficiency introduced by this PR.
How to fix it
Replace the early-exit scan with a counting loop, then use the exact required capacity:
var accessor_count: usize = 0;
for (class.properties) |prop| {
if (prop.kind == .auto_accessor) accessor_count += 1;
}
if (accessor_count == 0) return;
var rewritten = bun.handleOom(ListManaged(Property).initCapacity(
p.allocator,
class.properties.len + 2 * accessor_count,
));Step-by-step proof with a concrete example
Consider:
class Foo {
x = 1; // normal property
accessor y = 2; // auto_accessor → expands to 3 entries
z = 3; // normal property
}class.properties.len= 3 (N = 3, K = 1)- Initial capacity allocated = 3
- Desugaring appends
#_accessor_storage_0(1 entry), then replacesaccessor ywith getter + setter (2 entries replacing 1) - Final list length = 3 − 1 + 3 = 4 entries (or equivalently N + 2*K = 3 + 2 = 5 … wait, let me be precise)
Actually: original N=3 properties are processed; the 1 non-accessor are appended as-is (2 entries), and the 1 accessor becomes 3 entries. Total = 2 + 3 = 4 entries (N + 2K = 3 + 21 = 5 is incorrect; it is (N − K) + 3K = N + 2K = 3 + 2 = 5). This exceeds the initial capacity of 3, forcing a reallocation after the first or second accessor entry is appended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #29197.
Fixes #27335.
Repro
Root cause
The
accessorclass field modifier (TC39 Stage 4 / TypeScript 4.9+) wasgated on Bun's internal
standard_decoratorsfeature flag insrc/ast/parseProperty.zig:Under
experimentalDecorators: true,standard_decoratorsisfalse,so the parser never accepts the keyword and falls through to treat the
next token as a property name — chokes on
x.The
accessorkeyword is a standalone proposal and is valid classsyntax regardless of which decorator model is in use.
Fix
Parser (
parseProperty.zig) — drop thestandard_decoratorsgate. Acceptaccessorwhenever we're inside a class.Lowering (
P.zig,visitExpr.zig) — newrewriteAutoAccessorPropertieshelper desugars each
accessor x = initinto:#x_accessor_storageprivate field holding the initializerget x() { return this.#x_accessor_storage; }set x(v) { this.#x_accessor_storage = v; }This matches what TypeScript emits under
experimentalDecorators: trueand is also what makes the code actually run: JavaScriptCore does not
currently parse the
accessorkeyword, so passing it through unchangederrors at runtime even for undecorated fields.
Any
@decattached to the accessor carries over onto the synthesizedgetter. The existing legacy-decorator loop in
lowerClassthen emits__legacyDecorateClassTS([dec], Foo.prototype, "x", null)with thenulldescriptor kind — identical to TypeScript's output and correctaccessor-descriptor semantics.
The rewrite runs post-visit (called from
lowerClassfor classstatements, and from
e_classaftervisitClassfor classexpressions) so the synthesized
G.Fnnodes don't need parse-timescope entries. The standard-decorators lowering has its own
auto-accessor handling in
src/ast/lowerDecorators.zig, so thehelper early-returns when that path is already being taken.
Verification
Transpiled output for the repro now matches TypeScript:
Regression test
test/regression/issue/29197.test.tscovers 6 scenarios:@dec accessor x = ...with legacy decoratoraccessor x = ...(+ typed variant)static accessor count = 0accessorin a class expressionAll 6 fail on a
git stashbaseline, all 6 pass with the patch.Existing decorator test suites (
test/bundler/transpiler/decorators.test.ts,es-decorators.test.ts,decorator-metadata.test.ts,es-decorators-esbuild.test.ts,and
bundler_decorator_metadata.test.ts) — 203 tests total — continue to pass.