-
Notifications
You must be signed in to change notification settings - Fork 5k
fix(parser): allow accessor keyword with experimentalDecorators: true
#27336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, tempDir } from "harness"; | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/27335 | ||
| // The `accessor` keyword should work in TypeScript classes even when | ||
| // `experimentalDecorators: true` is set in tsconfig.json. | ||
|
|
||
| test("accessor keyword works with experimentalDecorators: true", async () => { | ||
| using dir = tempDir("issue-27335", { | ||
| "tsconfig.json": JSON.stringify({ | ||
| compilerOptions: { | ||
| experimentalDecorators: true, | ||
| }, | ||
| }), | ||
| "main.ts": ` | ||
| class Person { | ||
| public accessor name: string = "John"; | ||
| } | ||
|
|
||
| const p = new Person(); | ||
| console.log(p.name); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "main.ts"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("John\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("accessor keyword works with various modifiers and experimentalDecorators", async () => { | ||
| using dir = tempDir("issue-27335-modifiers", { | ||
| "tsconfig.json": JSON.stringify({ | ||
| compilerOptions: { | ||
| experimentalDecorators: true, | ||
| }, | ||
| }), | ||
| "main.ts": ` | ||
| class Foo { | ||
| accessor x = 1; | ||
| public accessor y = 2; | ||
| private accessor z = 3; | ||
| static accessor w = 4; | ||
|
|
||
| getZ() { return this.z; } | ||
| } | ||
|
|
||
| const f = new Foo(); | ||
| console.log(f.x); | ||
| console.log(f.y); | ||
| console.log(f.getZ()); | ||
| console.log(Foo.w); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "main.ts"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("1\n2\n3\n4\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("accessor keyword works without experimentalDecorators (standard mode)", async () => { | ||
| using dir = tempDir("issue-27335-standard", { | ||
| "tsconfig.json": JSON.stringify({ | ||
| compilerOptions: {}, | ||
| }), | ||
| "main.ts": ` | ||
| class Person { | ||
| public accessor name: string = "John"; | ||
| } | ||
|
|
||
| const p = new Person(); | ||
| console.log(p.name); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "main.ts"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("John\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("accessor with experimental decorators on other members", async () => { | ||
| using dir = tempDir("issue-27335-mixed", { | ||
| "tsconfig.json": JSON.stringify({ | ||
| compilerOptions: { | ||
| experimentalDecorators: true, | ||
| }, | ||
| }), | ||
| "main.ts": ` | ||
| function log(target: any, key: string) { | ||
| // simple experimental decorator | ||
| } | ||
|
|
||
| class MyClass { | ||
| @log | ||
| greet() { return "hello"; } | ||
|
|
||
| accessor count: number = 42; | ||
| } | ||
|
|
||
| const obj = new MyClass(); | ||
| console.log(obj.greet()); | ||
| console.log(obj.count); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "main.ts"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("hello\n42\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Critical: Bug: when
experimentalDecorators: trueand a class has anaccessorfield,should_lower_standard_decoratorsis set totruebecausehas_auto_accessoris now evaluated before thestandard_decoratorsguard. This routes the entire class (including any legacy experimental decorators on other members) through the TC39 standard decorator lowering path (lowerStandardDecoratorsStmt/__decorateElement), which calls decorators with(value, context)instead of the expected experimental signature(target, key, descriptor). The fix should gatehas_auto_accessoronstandard_decoratorsas well, or handle accessor lowering separately from the decorator lowering path when in experimental mode.Why this is a problem
What the Bug Is
The PR changes the assignment of
should_lower_standard_decoratorsfrom:to:
This boolean logic change means that the presence of an
accessorfield alone is now sufficient to setshould_lower_standard_decorators = true, regardless of whether the file is using standard or experimental decorators. In the old code,standard_decoratorswas a prerequisite for the entire expression, ensuring experimental decorator mode could never reach the standard lowering path. In the new code,has_auto_accessorshort-circuits past that guard.The Specific Code Path
When
experimentalDecorators: trueis set in a TypeScript project's tsconfig.json, the transpiler setsopts.features.standard_decorators = false(confirmed atsrc/transpiler.zig:1106andsrc/bundler/ParseTask.zig:1217). Under these conditions, a class like:will have
has_auto_accessor = true(from theaccessor xfield) andhas_any_decorators = true(from the@logdecorator). With the new code,should_lower_standard_decoratorsevaluates totrue or (false and true)=true. InP.ziglowerClass (line 4873), whenshould_lower_standard_decoratorsistrue, the class is immediately routed tolowerStandardDecoratorsStmtwhich returns, completely bypassing the legacy experimental decorator path starting at line 4884.Why Existing Tests Don't Catch It
The PR includes a test case (test case 4: "accessor with experimental decorators on other members") that appears to validate this scenario, but the
@logdecorator used in that test is a no-op:function log(target: any, key: string) {}. Since it discards its arguments, it produces no observable difference regardless of whether it's called with(value, context)(standard TC39 protocol) or(target, key, descriptor)(experimental protocol). A real-world experimental decorator that inspects or modifiestarget,key, ordescriptorwould silently receive incorrect arguments.Step-by-Step Proof
experimentalDecorators: truein tsconfig.json, sostandard_decorators = false.accessor x = 1and@myDecorator greet() {}, wheremyDecoratoris an experimental-style decorator expecting(target, key, descriptor).has_auto_accessor = trueandhas_any_decorators = true.should_lower_standard_decorators = true or (false and true) = true.P.ziglowerClass (line 4873), the conditionstmt.data.s_class.class.should_lower_standard_decoratorsistrue.lowerStandardDecoratorsStmtis called, which processes ALL decorators in the class using__decorateElement(inlowerDecorators.zig), calling each decorator with the TC39 standard protocol(value, context).@myDecoratorreceives(value, context)instead of(target, key, descriptor). Thetargetparameter gets a function value instead of the class prototype,keygets a context object instead of the method name string, anddescriptorisundefined.How to Fix It
The fix should ensure that
has_auto_accessoronly triggers the standard decorator lowering path whenstandard_decoratorsis alsotrue. One approach is:(i.e., reverting to the original logic) and handling the
accessorkeyword lowering for experimental decorator mode through a separate mechanism. Alternatively, if the intent is to supportaccessorwith experimental decorators, theaccessorfield should be lowered independently of the decorator lowering path, perhaps by transforming it into a getter/setter pair without routing throughlowerStandardDecoratorsStmt.