Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
7 changes: 6 additions & 1 deletion src/ast/g.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,12 @@ impl Class {
return false;
}

if property.kind == PropertyKind::Normal && f.contains(flags::Property::IsStatic) {
// Static auto-accessor initializers run at class-definition time
// just like static fields, so they get the same side-effect check.
Comment thread
robobun marked this conversation as resolved.
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
4 changes: 3 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,11 @@ 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));
}
// Auto-accessors duplicate the key across the synthesized get/set
// pair, so their computed keys must be hoisted to evaluate once.
Comment thread
robobun marked this conversation as resolved.
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)
{
computed_key_counter += 1;
let key_name: &'a [u8] = if computed_key_counter == 1 {
Expand Down
15 changes: 13 additions & 2 deletions src/js_parser/parse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,17 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
p.lexer.expect(T::TCloseBrace)?;

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

// Auto-accessor classes lower through the standard-decorator path, which
// would silently misapply any legacy TS decorators in the same class.
Comment thread
robobun marked this conversation as resolved.
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 +284,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
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),
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
4 changes: 2 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,10 @@ 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 [no LineTerminator here] ClassElementName`,
// valid under either decorator mode (TC39 / TS 4.9+).
Comment thread
robobun marked this conversation as resolved.
if opts.is_class
&& !p.lexer.has_newline_before
&& p.options.features.standard_decorators
&& PropertyModifierKeyword::find(raw)
== Some(PropertyModifierKeyword::PAccessor)
{
Expand Down
259 changes: 259 additions & 0 deletions test/regression/issue/29197.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,259 @@
// 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.

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

View check run for this annotation

Claude / Claude Code Review

Test file misplaced: #29197 is a missing feature, not a regression

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).
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, , exitCode] = await runBun(String(dir), "main.ts");
expect(stdout).toBe("first\nsecond\nthird\n");
expect(exitCode).toBe(0);
},
);

test.concurrent("anonymous `export default class` with a static accessor round-trips", 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, , exitCode] = await runBun(String(dir), "main.ts");
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);
});