Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/ast/g.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,16 @@ impl Class {
return false;
}

if property.kind == PropertyKind::Normal && f.contains(flags::Property::IsStatic) {
// `.AutoAccessor` static initializers evaluate at class-definition
// time just like `.Normal` static fields (auto-accessors are lowered
// later by `lower_decorators` into a WeakMap + getter/setter pair),
// so include them in the side-effect check to avoid hoisting
// `static accessor x = sideEffect()` past preceding statements.
// (`can_be_moved` runs pre-visit, before lowering.)
Comment thread
robobun marked this conversation as resolved.
Outdated
if (property.kind == PropertyKind::Normal
|| property.kind == PropertyKind::AutoAccessor)
&& f.contains(flags::Property::IsStatic)
{
for val in [property.value, property.initializer].into_iter().flatten() {
match val.data {
ExprData::EArrow(..) | ExprData::EFunction(..) => {}
Expand Down
8 changes: 7 additions & 1 deletion src/js_parser/lower/lower_decorators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1266,9 +1266,15 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
);
pre_eval_stmts.push(p.var_decl(dec_ref, Some(arr), loc));
}
// Hoist computed keys for decorated props AND for undecorated
// auto-accessors. An undecorated `accessor [k()] = 1` lowers to a
// `get [k()]` / `set [k()]` pair that duplicates `prop.key`; the
// runtime would evaluate `k()` twice and install the getter/setter
// under different keys (TC39 auto-accessor spec requires exactly
// one evaluation).
Comment thread
robobun marked this conversation as resolved.
Outdated
if prop.flags.contains(Flags::Property::IsComputed)
&& prop.key.is_some()
&& prop.ts_decorators.len_u32() > 0
&& (prop.ts_decorators.len_u32() > 0 || prop.kind == PropertyKind::AutoAccessor)
Comment thread
robobun marked this conversation as resolved.
Outdated
{
computed_key_counter += 1;
let key_name: &'a [u8] = if computed_key_counter == 1 {
Expand Down
17 changes: 15 additions & 2 deletions src/js_parser/parse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,19 @@
p.lexer.expect(T::TCloseBrace)?;

let has_any_decorators = has_decorators || class_opts.ts_decorators.len() > 0;

// JSC doesn't parse `accessor` natively, so any class with auto-accessors must go
// through the standard-decorator lowering (WeakMap + getter/setter) regardless of
// mode. But mixing auto-accessors with legacy TS decorators would silently reroute
// those decorators through the standard-proposal runtime — reject that combination.
Comment thread
robobun marked this conversation as resolved.
Outdated
if has_auto_accessor && !p.options.features.standard_decorators && has_any_decorators {
p.log().add_error(
Some(p.source),
class_keyword.loc,
b"Cannot mix the `accessor` keyword with `experimentalDecorators: true` in the same class. Use standard decorators instead.",
);
Comment thread
robobun marked this conversation as resolved.
}

// `Expr: Copy` — safe arena-slice → owned Vec (one memcpy, no double-drop).
let ts_decorators = ExprNodeList::from_arena_slice(class_opts.ts_decorators);
Ok(G::Class {
Expand All @@ -273,8 +286,8 @@
body_loc,
properties: bun_ast::StoreSlice::new_mut(properties.into_bump_slice_mut()),
has_decorators: has_any_decorators,
should_lower_standard_decorators: p.options.features.standard_decorators
&& (has_any_decorators || has_auto_accessor),
should_lower_standard_decorators: has_auto_accessor
|| (p.options.features.standard_decorators && has_any_decorators),

Check notice on line 290 in src/js_parser/parse/mod.rs

View check run for this annotation

Claude / Claude Code Review

Routing accessor-only classes through lower_impl exposes pre-existing lower_decorators bugs

Pre-existing follow-up: routing accessor-only classes through `lower_impl` (via `has_auto_accessor || ...`) surfaces two `lower_decorators.rs` bugs already reachable in the default config — (1) unconditional `__decoratorMetadata` emission gives `class Foo { accessor x = 1 }` a spurious `Foo[Symbol.metadata]` (lower_decorators.rs:2218-2223; TC39/tsc/esbuild only attach it when a decorator is present), and (2) an undecorated `static accessor` initializer is pushed straight to `suffix_exprs` (lower
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
})
}

Expand Down
9 changes: 7 additions & 2 deletions src/js_parser/parse/parse_property.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,10 +463,15 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}
}
PropertyModifierKeyword::PAccessor => {
// "accessor" keyword for auto-accessor fields (TC39 standard decorators)
// `accessor` is a standalone proposal, not gated on the
// decorators mode (either legacy or standard is fine).
//
// TC39 grammar is `accessor [no LineTerminator here]
// ClassElementName`, matching `.p_async` above: a newline
// before the next token means `accessor` is an ordinary
// field name terminated by ASI, not the modifier keyword.
Comment thread
robobun marked this conversation as resolved.
Outdated
if opts.is_class
&& !p.lexer.has_newline_before
&& p.options.features.standard_decorators
&& PropertyModifierKeyword::find(raw)
== Some(PropertyModifierKeyword::PAccessor)
{
Expand Down
261 changes: 261 additions & 0 deletions test/regression/issue/29197.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
// 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.
Comment on lines +1 to +10

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.

🟡 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.ts is 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

  1. CLAUDE.md reserves test/regression/issue/ for bugs where the behavior was correct in a previous release and then broke.
  2. The PR description's Root cause section: "was gated on Bun's internal standard_decorators feature flag" → the parser has always rejected accessor under experimentalDecorators: true; there is no prior release in which it worked.
  3. 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.
  4. CLAUDE.md's fallback rule: "the test belongs in the existing file for that module." test/bundler/transpiler/decorators.test.ts exists on main and is the established home for decorator-transpilation tests.
  5. PR Accept and lower the accessor keyword in TypeScript files using experimentalDecorators #38125 (per robobun 2026-08-13), which is intended to replace this PR, independently places the same coverage in test/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.


import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";

async function runBun(cwd: string, ...args: string[]) {
await using proc = Bun.spawn({
cmd: [bunExe(), ...args],
env: bunEnv,
cwd,
stderr: "pipe",
stdout: "pipe",
});
return await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
}

test.concurrent("accessor with various modifiers under experimentalDecorators: true", async () => {
using dir = tempDir("issue-29197-modifiers", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: true },
}),
"main.ts": `class Foo {
accessor a = 1;
public accessor b = 2;
private accessor c = 3;
protected accessor d = 4;
static accessor e = 5;
readonly accessor f = 6;
getC() { return this.c; }
getD() { return this.d; }
}
const f = new Foo();
console.log(f.a, f.b, f.getC(), f.getD(), Foo.e, f.f);
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("1 2 3 4 5 6\n");
expect(exitCode).toBe(0);
});

test.concurrent("accessor without tsconfig (TS file, no decorator flags)", async () => {
using dir = tempDir("issue-29197-plain", {
"main.ts": `class Foo {
accessor x: number = 42;
}
console.log(new Foo().x);
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("42\n");
expect(exitCode).toBe(0);
});

test.concurrent("accessor still works under standard decorators mode", async () => {
using dir = tempDir("issue-29197-std", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: false },
}),
"main.ts": `function dec(value: any, context: any) {
console.log("dec", context.name, context.kind);
}
class Foo {
@dec accessor x: number = 7;
}
console.log(new Foo().x);
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("dec x accessor\n7\n");
expect(exitCode).toBe(0);
});

test.concurrent(
"mixing accessor with experimentalDecorators legacy @dec is a clear error, not silent wrong semantics",
async () => {
using dir = tempDir("issue-29197-mixed", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: true },
}),
"main.ts": `function legacyDec(target: any, key: string) {}

class Foo {
@legacyDec
doThing() {}

accessor x: number = 0;
}
`,
});

const [, stderr, exitCode] = await runBun(String(dir), "main.ts");
expect(stderr).toContain("Cannot mix the `accessor` keyword with `experimentalDecorators: true`");
expect(exitCode).not.toBe(0);
},
);

test.concurrent("static accessor field: direct access works; subclass access throws (TC39 spec)", async () => {
// The standard-decorator lowering stores static accessor state in a
// WeakMap keyed on the declaring class. `Counter.count` round-trips; a
// subclass access (`Sub.count`) invokes the inherited getter with
// `this === Sub`, which is not in the WeakMap — matches TC39's static
// private-field brand-check semantics (TypeError at the key lookup).
using dir = tempDir("issue-29197-subclass", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: false },
}),
"main.ts": `class Counter { static accessor count = 10; }
class Sub extends Counter {}
console.log(Counter.count);
Counter.count = 99;
console.log(Counter.count);
try {
console.log("Sub.count=", Sub.count);
} catch (e) {
console.log("Sub caught:", (e as any).name);
}
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("10\n99\nSub caught: TypeError\n");
expect(exitCode).toBe(0);
});

test.concurrent("accessor field in a class expression", async () => {
using dir = tempDir("issue-29197-expr", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: false },
}),
"main.ts": `const Foo = class { accessor x = 1; };
const f = new Foo();
console.log(f.x);
f.x = 2;
console.log(f.x);
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("1\n2\n");
expect(exitCode).toBe(0);
});

test.concurrent(
"newline between `accessor` and the name triggers ASI (two fields, not one auto-accessor)",
async () => {
// TC39 grammar: `accessor [no LineTerminator here] ClassElementName`.
// With a newline, `accessor` must be parsed as a plain field name
// terminated by ASI, and the following `y = 1` becomes a second
// data field — NOT a single auto-accessor `y`. Matches tsc/esbuild.
using dir = tempDir("issue-29197-asi", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: true },
}),
"main.ts": `class C {
accessor
y = 1
}
const c = new C() as any;
console.log("keys:", Object.getOwnPropertyNames(c).sort().join(","));
console.log("accessor:", c.accessor);
console.log("y:", c.y);
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("keys: accessor,y\naccessor: undefined\ny: 1\n");
expect(exitCode).toBe(0);
},
);

test.concurrent(
"`static accessor` with a side-effecting initializer is not hoisted past preceding statements",
async () => {
// Non-bundle tree-shaking calls `G::Class::can_be_moved()` on the
// pre-visit AST. `can_be_moved` used to only inspect `.Normal` static
// initializers, so a class with `static accessor x = sideEffect()` was
// (incorrectly) treated as movable and hoisted ahead of preceding
// statements, inverting evaluation order.
using dir = tempDir("issue-29197-hoist", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: false },
}),
"main.ts":
'console.log("first");\n' +
"export class Foo {\n" +
' static accessor x = (console.log("second"), 42);\n' +
"}\n" +
'console.log("third");\n',
});

const [stdout, stderr, exitCode] = await runBun(String(dir), "main.ts");
expect(stderr).not.toContain("panic");

Check warning on line 204 in test/regression/issue/29197.test.ts

View check run for this annotation

Claude / Claude Code Review

Forbidden expect(stderr).not.toContain("panic") assertions

Lines 204 and 230 use `expect(stderr).not.toContain("panic")`, which CLAUDE.md explicitly forbids ("NEVER write tests that check for no 'panic' … in the test output. These tests will never fail in CI"). Both tests already assert exact stdout via `.toBe(...)` and `exitCode === 0`, which fully cover the panic case — a panic exits nonzero and produces none of the expected stdout. Drop both `.not.toContain("panic")` lines (and optionally rename the second test to describe what it positively asserts,
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(stdout).toBe("first\nsecond\nthird\n");
expect(exitCode).toBe(0);
},
);

test.concurrent("anonymous `export default class` with a static accessor does not panic", async () => {
// Regression: `export default class { static accessor x = 1 }` used
// to trip a null-ref panic in `lower_standard_decorators_stmt` because
// `class.class_name` was only injected from `default_name` when the
// class had decorators. Auto-accessors go through the same lowering,
// so the name injection now also runs when any property is an
// `AutoAccessor`.
using dir = tempDir("issue-29197-default-export", {
"tsconfig.json": JSON.stringify({
compilerOptions: { experimentalDecorators: false },
}),
"base.ts": "export default class { static accessor x = 1; }\n",
"main.ts":
"import Base from './base';\n" +
"console.log('x=', Base.x);\n" +
"Base.x = 42;\n" +
"console.log('x=', Base.x);\n",
});

const [stdout, stderr, exitCode] = await runBun(String(dir), "main.ts");
expect(stderr).not.toContain("panic");
expect(stdout).toBe("x= 1\nx= 42\n");
expect(exitCode).toBe(0);
});

test.concurrent("accessor with a computed key evaluates the key exactly once", async () => {
// TC39 auto-accessor spec requires the PropertyName to be evaluated
// once. An undecorated `accessor [k()] = 1` lowers through
// `lower_decorators` into a `get [k()]` / `set [k()]` pair that shares
// `prop.key`; without the computed-key hoist (gated in older code on
// `ts_decorators.len > 0`), `k()` runs twice — breaking the spec and
// installing the getter/setter under different keys for a non-idempotent
// key. Widened the hoist gate to include `AutoAccessor`.
using dir = tempDir("issue-29197-computed-key", {
"main.ts": `let calls = 0;
const k = () => (calls++, "x");
class C {
accessor [k()] = 42;
}
const c = new C() as any;
console.log("calls=", calls);
console.log("x=", c.x);
c.x = 99;
console.log("x=", c.x);
console.log("calls=", calls);
`,
});

const [stdout, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("calls= 1\nx= 42\nx= 99\ncalls= 1\n");
expect(exitCode).toBe(0);
});
Loading