Parse and lower accessor fields under experimentalDecorators - #29201
Parse and lower accessor fields under experimentalDecorators#29201robobun wants to merge 3 commits into
accessor fields under experimentalDecorators#29201Conversation
|
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)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
8c580fb to
5bf9b53
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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}
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
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
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
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
…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.
b1cce00 to
a00c632
Compare
There was a problem hiding this comment.
No issues found in this pass, but this is a substantial new AST lowering pass (~300 lines in P.zig) with many edge cases that have already required multiple correctness fixes — worth a human pass over the final shape of rewriteAutoAccessorProperties and its interaction with lowerClass.
Extended reasoning...
Overview
This PR enables parsing of the accessor class-field keyword under experimentalDecorators: true (previously gated on standard_decorators) and adds a new ~300-line rewriteAutoAccessorProperties helper in src/ast/P.zig that desugars accessor x = init into a private backing field plus synthesized get/set members. Supporting changes thread the rewrite through lowerClass (class statements) and the e_class visitor in visitExpr.zig (class expressions), plus a 17-case regression suite.
Security risks
None. This is a transpiler/AST transformation; it does not touch auth, crypto, network, filesystem permissions, or process boundaries. The only attack-surface-adjacent concern is generating syntactically invalid JS from adversarial input (e.g. private-name collisions, non-identifier keys), and those paths now have collision-avoidance and tests.
Level of scrutiny
High. This is core parser/lowering logic on a hot path that affects every TypeScript file using accessor fields. The PR went through at least ten rounds of bug-hunter feedback (invalid private-identifier synthesis, computed-key double-evaluation, static-accessor subclass brand-check failure, missing return_ts_metadata, descriptor-key re-evaluation in decorator calls, scope placement of synthesized symbols, temp-name collisions across sibling classes, private-identifier descriptor printing, etc.), all of which were addressed — but the volume of corrections suggests the design surface is subtle enough that a human familiar with the parser internals should sign off on the final implementation.
Other factors
- A documented known limitation remains: computed accessor keys in class expressions (where there is no statement-level prefix sink) still evaluate twice; the author chose to error on the decorated class-expression case but accept the undecorated double-eval as strictly-better-than-before.
- Synthesized symbols are appended to the enclosing scope's
generatedlist rather than the (already-popped) class-body scope — acknowledged in a doc comment as a renamer-slot-optimality trade-off deferred to a follow-up. - The three CI failures (worker_threads segfault on Windows, 26225 assertion, bun-install) appear unrelated to this change.
- All prior inline review comments are marked resolved, and the current bug-hunter pass found nothing new.
4036c1b to
0715021
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and the bug hunter found nothing new this round, but this changes parser/lowering semantics across both the Rust and Zig implementations and bakes in a design decision (hard-error on accessor + legacy @dec in the same class, routing accessor-only classes through standard-decorator lowering) — worth a human sign-off.
Extended reasoning...
Overview
This PR un-gates the accessor class field keyword from standard_decorators so it parses under experimentalDecorators: true, and routes any class containing an auto-accessor through the existing lower_decorators WeakMap+getter/setter lowering (since JSC does not parse accessor natively). It touches 10 files across both the Rust and Zig parser implementations: parse_property (drop gate, add [no LineTerminator here] ASI guard), parse/mod (widen should_lower_standard_decorators, add a hard error for mixing accessor with legacy @dec), lower_decorators (widen computed-key hoist to undecorated auto-accessors), g.{rs,zig} (can_be_moved now inspects .auto_accessor static initializers), and visit_stmt.zig (inject default_name when should_lower_standard_decorators). A 10-scenario regression suite covers modifiers, ASI, hoist ordering, computed-key single-evaluation, class expressions, anonymous default export, and the mixed-mode error.
Security risks
None. This is JS/TS parser and AST-lowering logic; no auth, crypto, FS, or network surface. The added error path uses a fixed message string; no user input flows into anything sensitive.
Level of scrutiny
High. Parser correctness changes are production-critical — getting them wrong silently emits incorrect JavaScript for end users. The PR went through ~15 bot-review iterations that surfaced real bugs each time (double-evaluation of computed keys, static-accessor brand-check failures, null-unwrap panics on anonymous default exports, missing ASI guard, Rust/Zig parity gaps). All have been addressed in the final diff, and the most recent commits (270324d..0715021) cleanly resolve the last round of feedback. But two things warrant human judgment rather than bot approval:
- Design decision: hard-erroring on
accessor+ legacy@decin the same class (rather than attempting to lower legacy decorators alongside the accessor) is a reasonable but user-visible policy choice that diverges from tsc (which compiles this combination). A maintainer should confirm this is the intended UX. - Semantic widening:
should_lower_standard_decorators = has_auto_accessor || (...)means accessor-only classes now always take the standard-decorator lowering path regardless of mode. This is the simplest correct strategy, but it is a meaningful change to an internal routing flag that other code reads (visit_stmt,lower_class, the class-expression visitor).
Other factors
- Dual-language port (Rust + Zig) is now in lockstep; I verified
visit_stmt.rson main already has the matchinghas_decorators || should_lower_standard_decoratorscheck thatvisit_stmt.zigwas aligned to in 0715021. - Test coverage is solid for the documented scenarios; all use
test.concurrentper repo convention and avoid the ASAN-flaky empty-stderr pattern. - The PR has had no human reviewer engagement yet — only bot feedback — so there is no existing human approval to lean on.
0715021 to
5cf316a
Compare
|
This parse failure was independently re-reported. Rather than open a second PR, I pushed the newer work to a branch: The one place it disagrees with this PR is the legacy-decorator interaction. This PR adds a compile error for a class that has both legacy decorators and an // tsconfig: { "compilerOptions": { "experimentalDecorators": true } }
class Entity {
@Column() id: number = 0; // legacy decorator on a sibling member
accessor name = ""; // ok in tsc, rejected by this PR
}
class B {
@dec accessor n = 1; // legacy decorator on the accessor itself: also ok in tsc
}The branch instead rewrites each auto-accessor into a private backing field plus a getter/setter pair inside the class body (the shape tsc emits, and the same shape #26431 used): #name_accessor_storage = "";
get name() { return this.#name_accessor_storage; }
set name(v) { this.#name_accessor_storage = v; }A legacy decorator on the accessor moves to the getter, so the existing |
### Problem
Bun's TS parser treats `declare`, `abstract`, `interface`, and
`accessor` as modifiers even when a newline follows them, silently
deleting the declaration that comes after. esbuild and tsc both apply
ASI and treat the keyword as a standalone identifier expression
(statement level) or class field (class body).
```console
$ printf 'declare\nfunction foo() { return 1 }\nconsole.log(foo())\n' > r.ts
$ bun build r.ts --no-bundle
console.log(foo()); # function body gone; ReferenceError at runtime
$ npx esbuild r.ts
declare;
function foo() { return 1; }
console.log(foo());
```
```console
$ printf 'class Foo { declare\n foo() { return 1 } }\nconsole.log(new Foo().foo())\n' > r2.ts
$ bun build r2.ts --no-bundle
class Foo {} # method deleted; new Foo().foo() throws
console.log(new Foo().foo());
```
Sibling shapes with the same bug: `abstract\nclass A {}`,
`interface\nA\n{ sideEffect() }`, `abstract class A { abstract\nfoo() {}
}`, `export default abstract\nclass A {}`, `class A { accessor\n x = 1
}`. Also `class A { get\n*x() {} }` was rejected with `Unexpected *`;
esbuild/tsc parse a field `get` followed by a generator `*x`.
### Cause
esbuild (`internal/js_parser/js_parser.go`) gates each of these paths on
`!p.lexer.HasNewlineBefore` before committing to the modifier
interpretation, and validates the body of every `declare` statement
after the recursive parse. Bun's port lost both:
- `TsStmtDeclare`, `TsStmtInterface`, `TsStmtAbstract` in
`parse_stmt_fallthrough_ts_keyword` had no newline check.
- The `export default abstract` class path had no newline check.
- Class-body `PDeclare`, `PAbstract`, `PAccessor` in `parse_property`
had no newline check.
- `could_be_modifier_keyword` counted `*` after `get`/`set` as a
modifier follow-up; esbuild excludes it.
- `TsStmtDeclare` unconditionally wrapped whatever the recursive parse
returned in `S::TypeScript{}`, so a newline-split keyword that fell
through to `SExpr` silently left the rest of the input as live runtime
statements (`declare type\nFoo = number` emitted `Foo = number;`).
### Fix
- Add the missing `!p.lexer.has_newline_before` gates at each site
above.
- In `TsStmtDeclare`, after the recursive `parse_stmt`, reject any
result that is not `STypeScript`/`SLocal`/`SEmpty` with `Unexpected
"<token>"` pointing at the token captured before recursion. This
uniformly catches `declare
{interface,abstract,type,namespace,module,declare}\n`, `declare foo`,
and `declare foo: bar` while accepting every valid ambient form and
everything inside ambient bodies (`declare namespace { ... }`, `declare
module "m" { ... }`, `declare global { ... }`).
- `export default interface \n Foo {}` stays accepted (esbuild
explicitly allows a newline there) via the existing `is_name_optional`
flag. `export interface \n Foo {}` now reports `Unexpected "interface"`
like esbuild. `@decorator` followed by `declare`/`abstract` split by a
newline reports `Unexpected "declare"`/`"abstract"` instead of the
self-contradictory `Expected "class" but found "class"`.
- Drop the redundant `is_typescript_declare = true` pre-set in
`t_export`'s `SDeclare` arm (the `declare` arm sets it itself).
### Verification
New test in `test/bundler/transpiler/transpiler.test.js` (`contextual
keywords followed by a newline apply ASI instead of acting as
modifiers`) covers 50 assertions across every shape above: the ASI
cases, no-newline control cases, every valid `declare X` form, every
rejected `declare X` form, ambient-body cases (accepted), `export
abstract\n`/`export declare\n` (accepted), and the decorator variants.
Fails at the first assertion on the unfixed build.
Two cases in `test/bundler/transpiler/scope-mismatch-panic.test.ts`
(`declare foo: bar` and `declare module : es2015`) were previously
accepted and are now parse errors matching esbuild; their assertions
moved to the new test block.
- `bun bd test test/bundler/transpiler/transpiler.test.js` — 172 pass, 0
fail
- `bun bd test test/bundler/esbuild/ts.test.ts` — 57 pass, 0 fail
- `bun bd test
test/bundler/transpiler/{decorators,decorator-metadata,es-decorators}.test.ts
test/bundler/bundler_decorator_metadata.test.ts` — 78 pass, 0 fail
Related: #29201 independently adds the `accessor` newline check while
lifting the `standard_decorators` gate; whichever lands second has a
one-hunk rebase on that branch.
<!-- robobun:evidence:begin -->
---
**[review]** gate passed · iteration 3 · 4 files touched
<details><summary>fails on main (without fix)</summary>
```console
ASAN without fix: 1 failed, 22 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/scope-mismatch-panic.test.ts test/bundler/transpiler/transpiler.test.js
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (8e0d9d1)
test/bundler/transpiler/scope-mismatch-panic.test.ts:
(pass) scope mismatch panic regression test > should not panic with scope mismatch when arrow function is followed by array literal [741.41ms]
(pass) scope mismatch panic regression test > should not panic with simpler arrow function followed by array [440.32ms]
(pass) scope mismatch panic regression test > correctly rejects direct indexing into block body arrow function [428.17ms]
(pass) macro tagged templates visit their interpolations > tagged template macro with arrow interpolation in dead code is erased [431.69ms]
(pass) macro tagged templates visit their interpolations > tagged template macro with arr
... (truncated)
release without fix: 22 skipped
bun test v1.4.0-canary.1 (21df535)
test/bundler/transpiler/scope-mismatch-panic.test.ts:
(pass) scope mismatch panic regression test > should not panic with scope mismatch when arrow function is followed by array literal [22.06ms]
(pass) scope mismatch panic regression test > should not panic with simpler arrow function followed by array [13.58ms]
(pass) scope mismatch panic regression test > correctly rejects direct indexing into block body arrow function [11.90ms]
(pass) macro tagged templates visit their interpolations > tagged template macro with arrow interpolation in dead code is erased [12.44ms]
(pass) macro tagged templates visit their interpolations > tagged template macro with arrow interpolation reports the macro error [13.21ms]
(pass) macro tagged templates visit their interpolations > member-expression macro tag with function interpolation reports the macro error [11.90ms]
(pass) TypeScript 'declare' statements discard scopes of dropped statements > declare global containing nested blocks followed by a class [24.10ms]
(pass) TypeScript 'declare' statements discard scopes of dropped statements > declare const with an arrow function initializer followe
... (truncated)
```
</details>
<details><summary>passes on PR (with fix)</summary>
```console
ASAN with fix: 22 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/scope-mismatch-panic.test.ts test/bundler/transpiler/transpiler.test.js
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (8e0d9d1)
test/bundler/transpiler/scope-mismatch-panic.test.ts:
(pass) scope mismatch panic regression test > should not panic with scope mismatch when arrow function is followed by array literal [740.34ms]
(pass) scope mismatch panic regression test > should not panic with simpler arrow function followed by array [440.90ms]
(pass) scope mismatch panic regression test > correctly rejects direct indexing into block body arrow function [435.01ms]
(pass) macro tagged templates visit their interpolations > tagged template macro with arrow interpolation reports the macro error [438.93ms]
(pass) macro tagged templates visit their interpolations > tagged template macro with ar
... (truncated)
release with fix: 22 skipped
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped) in 761ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[0/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: component rust-std is up to date
nightly-2026-05-06-x86_64-unknown-linux-gnu unchanged - rustc 1.97.0-nightly (e95e73209 2026-05-05)
info: checking for self-update (current version: 1.29.0)
�[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl
... (truncated)
```
</details>
<details><summary>diff hotspot</summary>
```
src/js_parser/parse/parse_property.rs | 8 +-
src/js_parser/parse/parse_stmt.rs | 75 ++++++++++++++++--
.../transpiler/scope-mismatch-panic.test.ts | 9 +--
test/bundler/transpiler/transpiler.test.js | 90 ++++++++++++++++++++++
4 files changed, 165 insertions(+), 17 deletions(-)
```
</details>
**gate history** · 4 passed · 0 rejected · iteration 3
<details><summary>evidence per changed file</summary>
```
file reads edits tests
src/js_parser/parse/parse_property.rs 3 4 0
src/js_parser/parse/parse_stmt.rs 11 15 0
test/bundler/transpiler/scope-mismatch-panic.test.ts 3 5 0
test/bundler/transpiler/transpiler.test.js 4 10 0
```
</details>
<!-- robobun:evidence:end -->
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
This bug came up again on current main. #38125 is a fresh fix on top of today's main that follows the in-place model described in the previous comment (backing private field plus getter/setter pair generated inside the class body, legacy decorators moving to the getter), so a class mixing legacy decorators and |
…decorator lowering Fixes #29197, #27335. The `accessor` class-field keyword (TC39 / TS 4.9+) was gated on Bun's `standard_decorators` flag in the parser, so `experimentalDecorators: true` rejected it with a confusing "Expected ';'" syntax error. JSC doesn't parse the keyword natively, so it must be lowered. Changes (all in the Rust parser — the Zig parser was removed from main): - parse_property.rs: drop the `standard_decorators` gate on `.p_accessor`; add the TC39 `[no LineTerminator here]` restriction (matches `.p_async`) so `accessor\n x` is two fields via ASI, not one auto-accessor. - parse/mod.rs: route any class with an auto-accessor through `should_lower_standard_decorators` (WeakMap + getter/setter), and error clearly when legacy `@dec` is mixed with `accessor` rather than silently rerouting the decorator through the standard-proposal runtime. - g.rs (can_be_moved): include `.AutoAccessor` static initializers in the side-effect check so `static accessor x = sideEffect()` isn't hoisted past preceding statements in the non-bundle tree-shaking path. - lower_decorators.rs: widen the computed-key hoist to undecorated auto-accessors so `accessor [k()]` evaluates the key exactly once. Regression test covers modifiers, no-tsconfig, standard-decorator mode, mixed-mode error, subclass static access (TC39 brand-check), class expression, newline ASI, hoist ordering, anonymous default export, and computed-key single evaluation.
5cf316a to
8c4cbd3
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The stdout .toBe + exitCode 0 assertions already fail on any panic;
the not.toContain("panic") lines can never fail first and violate
the repo rule against no-panic output checks. Renamed the second test
to state what it positively asserts.
| // https://github.com/oven-sh/bun/issues/29197 (and #27335) | ||
| // | ||
| // The `accessor` keyword (TC39 auto-accessors / TS 4.9+) was rejected as a | ||
| // syntax error when a project's tsconfig.json had `experimentalDecorators: true`. | ||
| // The keyword should be accepted under either decorator mode. JSC doesn't | ||
| // parse `accessor` natively, so any class with auto-accessors is routed | ||
| // through the standard-decorator lowering (WeakMap + getter/setter) | ||
| // regardless of mode. Mixing `accessor` with legacy TS decorators errors | ||
| // clearly instead of silently rerouting decorators through the standard | ||
| // runtime. |
There was a problem hiding this comment.
🟡 Per CLAUDE.md, test/regression/issue/${N}.test.ts is reserved for true regressions (worked in a previous release, then broke) — but the PR's own root-cause section says accessor under experimentalDecorators: true was always gated off, so #29197/#27335 are missing-feature bugs, not regressions. These transpiler/decorator tests belong in test/bundler/transpiler/decorators.test.ts (which is where replacement PR #38125 already puts them, per robobun's 2026-08-13 comment).
Extended reasoning...
What the issue is
CLAUDE.md's Test Organization section is explicit:
Exception:
test/regression/issue/${issueNumber}.test.tsis reserved for bugs with a GitHub issue number and that are true regressions (worked in a previous release, then broke). If the behavior was never correct, it's not a regression — the test belongs in the existing file for that module.
This PR places its tests in test/regression/issue/29197.test.ts, but the PR's own "Root cause" section states that the accessor class field modifier "was gated on Bun's internal standard_decorators feature flag in the parser. Under experimentalDecorators: true, standard_decorators is false, so the parser never accepted the keyword." In other words, accessor under experimentalDecorators: true never worked in any prior Bun release — #29197 and #27335 report a missing feature, not a regression.
Where the tests belong
CLAUDE.md's default is "add your test to the existing test file for the code you're changing." The code being changed is the transpiler's decorator/class-property handling (src/js_parser/parse/parse_property.rs, src/js_parser/lower/lower_decorators.rs), and the existing test file for that surface is test/bundler/transpiler/decorators.test.ts (1107 lines, already covering both legacy and standard decorator lowering). The robobun 2026-08-13 timeline comment confirms this: replacement PR #38125 "has the tests in test/bundler/transpiler/decorators.test.ts."
Step-by-step proof
- CLAUDE.md reserves
test/regression/issue/for bugs where the behavior was correct in a previous release and then broke. - The PR description's Root cause section: "was gated on Bun's internal
standard_decoratorsfeature flag" → the parser has always rejectedaccessorunderexperimentalDecorators: true; there is no prior release in which it worked. - Therefore Decorators on accessor class fields fail to parse in Bun #29197 is not a regression by CLAUDE.md's definition; the test file does not qualify for the
test/regression/issue/exception. - CLAUDE.md's fallback rule: "the test belongs in the existing file for that module."
test/bundler/transpiler/decorators.test.tsexists on main and is the established home for decorator-transpilation tests. - PR Accept and lower the
accessorkeyword in TypeScript files using experimentalDecorators #38125 (per robobun 2026-08-13), which is intended to replace this PR, independently places the same coverage intest/bundler/transpiler/decorators.test.ts— corroborating that as the correct location.
Why this matters / impact
Test-organization only; product correctness is unaffected. Placing tests in the wrong directory hurts discoverability (the next person touching lower_decorators.rs looks in decorators.test.ts, not under a five-digit issue number) and duplicates setup that decorators.test.ts already provides. It also dilutes the test/regression/issue/ directory's meaning as a regression-only bucket.
How to fix
Move the ten test.concurrent(...) cases into test/bundler/transpiler/decorators.test.ts (e.g. under a describe("auto-accessor (TC39 / TS 4.9+)", ...) block) and delete test/regression/issue/29197.test.ts. The runBun helper can be replaced with the existing spawn helpers already used in that file, or inlined per-test since each case already builds its own tempDir. Given that #38125 is intended to replace this PR and already places its tests there, aligning now avoids a second churn.
|
Closing out in favor of #38125, which fixes #29197 by desugaring each The test file from this PR was run against #38125's branch: every case passes there except the deliberate "cannot mix" error and the no-tsconfig computed-key case, which exercises the standard-decorator path and is covered by #31926. The modifier coverage from here ( |
1 participant
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 in the parser.Under
experimentalDecorators: true,standard_decoratorsisfalse,so the parser never accepted the keyword and fell through to treat the
next token as a property name, choking on
x.The
accessorkeyword is a standalone proposal and is valid classsyntax regardless of decorator model.
Fix
All changes are in the Rust parser (
src/js_parser,src/ast):Parser (
parse_property.rs) — drop thestandard_decoratorsgateon the
p_accessorbranch. Add TC39's[no LineTerminator here]restriction so
class C { accessor\n y = 1 }parses as two fields viaASI (matching the existing
.p_asynccheck).Lowering routing (
parse/mod.rs) — JSC does not parseaccessor,so any class with an auto-accessor must go through the existing
standard-decorator lowering (WeakMap + getter/setter). Widen
should_lower_standard_decoratorstohas_auto_accessor || (standard && has_any_decorators).Mixed legacy + accessor → clear error (
parse/mod.rs) — if a classhas both legacy (
@dec) decorators AND anaccessorfield underexperimentalDecorators: true, silently rerouting the legacy decoratorsthrough the standard-proposal runtime would be wrong. Report a clear
compile-time error instead.
Hoist ordering (
g.rscan_be_moved) — include.AutoAccessorstatic initializers in the side-effect check so
static accessor x = sideEffect()isn't hoisted past preceding statements in the non-bundletree-shaking path.
Computed-key single evaluation (
lower_decorators.rs) — widen thecomputed-key hoist (previously gated on
ts_decorators.len > 0) toundecorated auto-accessors, so
accessor [k()] = 1evaluatesk()exactly once instead of twice.
Anonymous
export default class { accessor x = 1 }name injection isalready handled on main by the
has_decorators || should_lower_standard_decoratorscheck in
visit_stmt.rs.Verification
Regression test
test/regression/issue/29197.test.tscovers 10 scenarios:accessorwithpublic/private/protected/static/readonlymodifiersaccessorin a.tsfile with no tsconfig@dec accessorin standard-decorators modeaccessoremits the clear errorstatic accessor: direct access works, subclass access throws (TC39 spec)accessorin a class expressionaccessor(two fields, not one auto-accessor)static accessor x = sideEffect()evaluates in source orderexport default class { static accessor x = 1 }doesn't panicaccessor [k()]evaluates the key exactly onceRebase note
The Zig parser this PR originally targeted was fully removed from main
(migrated to Rust). All fixes were re-applied to the Rust parser; the
obsolete Zig-side changes were dropped. The final diff is Rust-only.
[review] gate passed · iteration 18 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 18
evidence per changed file
root cause · written by the author bot
The parser only recognized the
accessorkeyword on class fields when the standard decorators feature was enabled, so under experimentalDecorators the token fell through to generic parsing and produced syntax errors. The fix removes that guard soaccessoris parsed as an auto-accessor property in any class context, and adds a lowering pass that rewrites each auto-accessor into a private backing field with synthesized get and set members for both class statements and class expressions. This preserves decorator and TypeScript metadata, evaluates computed keys once via hoisted temporaries, …