Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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 @@
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.",
);

Check warning on line 273 in src/js_parser/parse/mod.rs

View check run for this annotation

Claude / Claude Code Review

New compile error rejects tsc-valid accessor + legacy-decorator classes; gate is also over-broad (parameter decorators)

This error rejects tsc-valid code: TypeScript 4.9-5.9 compile both `class E { @column() id = 0; accessor name = '' }` and `@dec accessor n = 1` under `experimentalDecorators: true` (private backing field + `__decorate` on the getter), so per REVIEW.md's "the reference implementation is the spec" this leaves #29197/#27335 unfixed for TypeORM/NestJS users. The gate is also over-broad: `has_any_decorators` folds in `opts.has_argument_decorators` (line 243), so `class C { constructor(@Inject() x) {}
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 @@
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 failure on line 288 in src/js_parser/parse/mod.rs

View check run for this annotation

Claude / Claude Code Review

Adding `accessor` under useDefineForClassFields:false silently flips sibling fields to [[Define]] semantics

Widening `should_lower_standard_decorators` to `has_auto_accessor || (...)` forces `use_define = true` at visit/mod.rs:1016-1017 for the whole class, so under `useDefineForClassFields: false` sibling `Normal` instance fields skip the `this.x = init` rewrite (:1044) and are emitted verbatim as in-body class fields by `lower_impl` (lower_decorators.rs:1666) — always [[Define]] semantics. In the canonical TypeORM/NestJS config (`experimentalDecorators: true` + `useDefineForClassFields: false`), `cl
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
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");
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