Skip to content

parser: accept accessor keyword under experimentalDecorators: true - #29554

Closed
robobun wants to merge 4 commits into
mainfrom
farm/30b378c4/accessor-experimental-decorators
Closed

robobun wants to merge 4 commits into
mainfrom
farm/30b378c4/accessor-experimental-decorators

Conversation

@robobun

@robobun robobun commented Apr 21, 2026 •

Copy link
Copy Markdown
Collaborator

What

Fixes #29553. Fixes #27335. The accessor keyword (TC39 auto-accessors, TypeScript 4.9+) was gated behind the internal standard_decorators feature flag in the parser, so any class field written as public accessor foo: number failed to parse when a project's tsconfig.json had experimentalDecorators: true — a common default, especially in Angular-style projects.

# Reproduced at test/TEST1.ts:2 with v1.3.13-canary.1 (bf2e2cec)
2 |     public accessor computed: number;
                        ^
error: Expected ";" but found "computed"

Why

TypeScript accepts accessor in both decorator modes — they're independent proposals. The lowering path in lowerDecorators.zig already handles undecorated auto_accessor properties (WeakMap + getter/setter pair). All that was missing was the parser gate and the should_lower_standard_decorators condition.

How

  • src/ast/parseProperty.zig: drop the p.options.features.standard_decorators guard on the p_accessor keyword branch. Always recognise accessor in class bodies.
  • src/ast/parse.zig: set should_lower_standard_decorators whenever an auto-accessor is present (not only when standard_decorators is already on), so the accessor gets lowered correctly. JavaScriptCore doesn't parse accessor natively, so the rewrite is what makes it run.
  • src/ast/parse.zig: if a class mixes accessor with TS legacy @dec members under experimentalDecorators: true, emit a clear compile error. Otherwise the legacy decorators would be routed through the standard-proposal runtime (wrong signature), which is worse than a clear error.

Verification

test/regression/issue/29553.test.ts — 5 tests covering:

  • The issue's exact repro (bun test ./test with experimentalDecorators: true).
  • accessor with all TS modifiers (public, private, protected, static, readonly) under experimentalDecorators: true.
  • accessor in a plain TS file (no tsconfig).
  • accessor under standard decorators (regression guard).
  • The new error for legacy-@dec + accessor combined.

Before-fix: tests fail with the Expected ";" parse error. After-fix: 5 pass.

Nearby decorator regression suites (test/bundler/transpiler/es-decorators.test.ts, decorators.test.ts, decorator-metadata.test.ts, issue/27526.test.ts, issue/27575.test.ts) all still pass — 209 tests across those files, no regressions.

Related (not closed by this PR)

The `accessor` keyword (TC39 auto-accessors / TypeScript 4.9+) was
gated behind the internal `standard_decorators` feature flag, so any
class field written as `public accessor foo: number` failed to parse
when a project's tsconfig.json had `experimentalDecorators: true` — a
common default in Angular-style projects.

TypeScript accepts `accessor` in both decorator modes (they're
independent proposals), so remove the gate. The lowering side already
handles undecorated `auto_accessor` properties (WeakMap + getter/setter
pair); we just need `should_lower_standard_decorators` to fire whenever
an auto-accessor is present, regardless of whether any other members
are decorated.

Combining `accessor` with TS legacy `@dec` in the same class would
route the legacy decorators through the standard-proposal runtime
(wrong signature, wrong semantics). Emit a clear compile-time error
for that case rather than silently miscompiling.

Closes #29553
@robobun

robobun commented Apr 21, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:11 PM PT - Apr 21st, 2026

❌ @autofix-ci[bot], your commit b55e4dd has 2 failures in Build #46875 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29554

That installs a local version of the PR into your bun-29554 executable, so you can run:

bun-29554 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. Decorators on accessor class fields fail to parse in Bun #29197 - Decorators on accessor class fields fail to parse (@example accessor x produces Expected ";" but found "x")
  2. TypeScript accessor keyword in classes fails to parse #27335 - TypeScript accessor keyword in classes fails to parse (public accessor name: string errors out)
  3. this.#field not rewritten in class field initializers when class has @decorated accessor #28118 - this.#field not rewritten in class field initializers when class has @decorated accessor (related to should_lower_standard_decorators not being set for auto-accessor classes)

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #29197
Fixes #27335
Fixes #28118

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Apr 21, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5f872a85-51b2-4156-a8f2-71d46ff45c2e

📥 Commits

Reviewing files that changed from the base of the PR and between 131fecc and b55e4dd.

📒 Files selected for processing (1)
  • test/regression/issue/29553.test.ts

Walkthrough

Parser recognizes accessor as an auto-accessor inside classes regardless of decorator mode, errors when auto-accessors coexist with legacy experimentalDecorators and any decorators, and makes auto-accessors follow the standard-decorator lowering path. A regression test exercises these cases.

Changes

Cohort / File(s) Summary
Parser: class parsing
src/ast/parse.zig
Emit an error if a class contains auto-accessor fields (accessor) while legacy decorator mode is active (!standard_decorators) and the class has any decorators. Update G.Class.should_lower_standard_decorators: auto-accessors force the standard lowering path; otherwise compute as standard_decorators && has_any_decorators.
Parser: property parsing
src/ast/parseProperty.zig
Stop gating recognition of the accessor modifier on p.options.features.standard_decorators; treat accessor as .auto_accessor for class properties when opts.is_class and the modifier mapping applies (preserving raw/p_static mapping).
Tests: regression
test/regression/issue/29553.test.ts
Add tests covering accessor auto-accessor parsing and runtime behavior across decorator configurations: with experimentalDecorators:true, without tsconfig, with experimentalDecorators:false plus a stage‑3 decorator, and an expected failure when mixing accessor with legacy decorators under experimentalDecorators:true.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: enabling the accessor keyword parser support under experimentalDecorators: true mode, which is the primary objective of the PR.
Description check ✅ Passed The description covers both required sections: 'What' explains the problem and fix, 'How' details the implementation approach, and includes verification details with test coverage.
Linked Issues check ✅ Passed The PR addresses all coding requirements from the linked issues [#29553, #27335]: parser now accepts accessor keyword in both decorator modes, auto-accessors are properly lowered, and the error handling for mixed legacy decorators is implemented with comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the objectives: parser modifications in parseProperty.zig and parse.zig, error handling for decorator mixing, and regression tests. No unrelated changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Parse and lower accessor fields under experimentalDecorators #29201 - Same parser fix in parseProperty.zig and parse.zig to accept and lower accessor under experimentalDecorators: true; fixes the identical symptom (Decorators on accessor class fields fail to parse in Bun #29197 / TypeScript accessor keyword in classes fails to parse #27335).
  2. feat(transpiler): lower auto-accessor class fields #26431 - Adds the same accessor parsing branch in parseProperty.zig and a lowerAutoAccessors pass; fixes the same underlying feature gap (Support TypeScript 4.9 class auto-accessors ("accessor" keyword) #6051).

🤖 Generated with Claude Code

@robobun

robobun commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — this is related to but not the same as #29201 / #26431.

If the comprehensive path in #29201 lands first, this PR should be closed. If #29201 stays stuck, this one unblocks the reported issues (#29553 and #27335) while leaving the mixed case as a surfaced "not supported" error instead of a silent miscompile.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the fix correctly threads accessor lowering through the existing WeakMap+getter/setter path by setting should_lower_standard_decorators whenever has_auto_accessor is true, regardless of decorator mode.

Extended reasoning...

Overview

Two small Zig changes and a new regression test file. parseProperty.zig drops the standard_decorators feature guard on the p_accessor branch so the keyword is accepted in class bodies under either decorator mode. parse.zig adjusts the should_lower_standard_decorators condition to has_auto_accessor or (standard_decorators and has_any_decorators) and adds a diagnostic for the mixed legacy-decorator + accessor case.

Security risks

None. This is a parser fix with no auth, crypto, or permissions code involved.

Level of scrutiny

Low-to-medium. The change is small and self-contained: one boolean condition in the property parser, one boolean condition in the class-body assembler, and one new error path. The existing lowerDecorators.zig already has the undecorated auto-accessor lowering path (WeakMap + getter/setter, line ~820), so no new lowering logic is introduced — the fix just ensures the right code path is reached. Five targeted regression tests cover the main scenario, modifier variants, no-tsconfig baseline, standard-decorator regression guard, and the new error case.

Other factors

The PR description notes that 209 tests across nearby decorator suites continue to pass. The mixed-mode error is a conservative safety net: rather than silently mis-route legacy decorators through the standard-proposal runtime, it rejects the combination with a clear diagnostic. That tradeoff is appropriate.

@robobun

robobun commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator Author

CI failures on b55e4ddf are pre-existing / infra, not caused by this PR:

The regression test added here (test/regression/issue/29553.test.ts) passes under debug/ASAN locally — 4/4, and fails 2/4 with src/ reverted, so the gate holds. Nothing code-side to do on my end; happy to re-push / re-run if a maintainer kicks CI.

@alii

alii commented Apr 21, 2026

Copy link
Copy Markdown
Member

@robobun Did you see #29201? How does it compare?

@robobun

robobun commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator Author

Yep — #29201 is the more comprehensive fix; this is a minimal subset. Breakdown:

#29201 (my earlier branch, 807+/10-, 4 files, 10 days old, BLOCKED):

  • Parser: drop the gate (same as here).
  • src/ast/P.zig: rewriteAutoAccessorProperties desugars every accessor field into #storage + get/set before the TS legacy decorator loop, so @legacyDec accessor x works end-to-end under experimentalDecorators: true. Handles decorated accessors, computed keys (single-eval via hoisted temp), private-identifier keys, static accessors, class-expression accessors, collision-avoiding backing names.
  • src/ast/visitExpr.zig: runs the same rewrite on class expressions.
  • Open CodeRabbit review feedback (4 items, mostly bun.handleOom style + one real concern about computed-key single-eval in class-expression fallback).

#29554 (this PR, 116+/3-, 3 files):

  • Parser: drop the gate.
  • Route auto-accessor classes through the existing standard-decorator lowering in lowerDecorators.zig (which already handles the undecorated accessor → WeakMap + get/set path at line 820). No new lowering code.
  • For @legacyDec method() + accessor x in the same class: emit a clean compile error rather than silently mis-route the legacy decorator through the standard-proposal runtime.

Trade-off is narrow: #29553 and #27335 fully fixed by either, but #29201 supports decorated accessors under experimentalDecorators while this PR rejects that combination with a clear diagnostic.

Happy with either landing. If you want the comprehensive one, #29201 needs a rebase onto current main (branch is 10d old) and the CodeRabbit threads addressed — I can do that on top of this. If you want the minimal path forward for now, this is ready and gate-verified.

@alii

alii commented Apr 21, 2026

Copy link
Copy Markdown
Member

Should we close this? What is more correct?

@robobun

robobun commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator Author

Honest take: #29201 is more correct, close this.

TypeScript compiles @legacyDec accessor x under experimentalDecorators: true by desugaring accessor to #storage + get/set first, then emitting __decorate([legacyDec], Foo.prototype, "x", null) on the synthesized getter. #29201 mirrors that model exactly. This PR rejects the same code with a clean error — a strictly narrower behavior than TS offers.

For the issue at hand (#29553), both work. For #29197 (@dec accessor x), only #29201 works.

I’ll rebase #29201 onto current main, address the 4 open CodeRabbit threads (mostly catch bun.outOfMemory → bun.handleOom, plus the computed-key single-eval in class-expression fallback), and close this one.

@robobun

robobun commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #29201 — rebased onto current main, squashed the 13 commits into one clean diff, reworded the commit message, and verified locally:

  • Regression test (test/regression/issue/29197.test.ts) — 17/17 pass.
  • Nearby decorator suites (decorators.test.ts, decorator-metadata.test.ts, es-decorators*.test.ts, issue/27526.test.ts, issue/27575.test.ts) — 207/207 pass, no regressions.
  • Issue Bun test issue when run through scripts (accessor) #29553 reporter's exact repro (bun test ./test with experimentalDecorators: true) — passes.
  • Mixed-case @legacyDec method() + @legacyDec accessor x — both decorators now fire with the correct descriptor shape (get/set for the accessor, value for the method), matching TS output.

#29201 now also closes #29553, #27335, #29197.

@robobun robobun closed this Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun test issue when run through scripts (accessor) TypeScript accessor keyword in classes fails to parse

2 participants