Repository navigation
Conversation
TC39 decorators do not include parameter decorators. When a .ts file uses @dec on a constructor or method parameter without experimentalDecorators set, the parser stored the decorator expression on G::Arg.ts_decorators, but the standard-decorator lowering and the printer both ignore that field, so the decorator was silently dropped and never called at runtime. Emit a parse error instead, matching esbuild and tsc: - TS: 'Parameter decorators only work when experimental decorators are enabled' with a note pointing at the tsconfig option - JS: 'Parameter decorators are not allowed in JavaScript'
Contributor
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
Collaborator
Author
|
Status: Ready for review. Diff is green where CI ran; remaining red is infra.
Reproduced with: mkdir /tmp/pd && cd /tmp/pd
cat > pd.ts <<'TS'
const seen: string[] = [];
function d(...a: any[]) { seen.push("called:" + a.length); }
class C { constructor(@d y?: number) {} m(@d x: number) { return x; } }
new C(1); new C().m(2);
console.log(seen);
TS
bun pd.ts # prints [] with no error; should error per tsc/esbuildAlso folds in a one-line resolver fix (16ac6a7): the tsconfig |
Collaborator
Author
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — small, targeted parser diagnostic that matches esbuild/tsc behavior.
What was reviewed:
- Verified
standard_decoratorsis set only whenexperimentalDecorators/emitDecoratorMetadataare off (transpiler.rs:1566, ParseTask.rs:2455), so the experimental path is unaffected — confirmed by the newstill work with experimentalDecoratorstest. - Confirmed the
!IS_TYPESCRIPT_ENABLEDbranch is reachable:allow_ts_decoratorsis true for JS class stmts/exprs under standard decorators, andparse_type_script_decoratorsparses@for JS whenstandard_decoratorsis set. - Checked
at_rangeis captured before the@is consumed so the error span points at the decorator, and thatadd_range_error_with_notes/add_range_erroronp.log()match existing call-site conventions inparse_property.rs/parse_entry.rs.
Extended reasoning...
Overview
This PR adds a parse-time error when parameter decorators (constructor(@d x) / m(@d x)) are encountered while the parser is in TC39 standard-decorator mode. Previously the decorators were parsed into G::Arg.ts_decorators and silently discarded by the standard lowering pass and the printer. The change is ~23 lines in src/js_parser/parse/parse_fn.rs inside the existing opts.allow_ts_decorators block, plus four new tests in test/bundler/transpiler/es-decorators.test.ts.
Security risks
None. This adds a diagnostic on already-parsed syntax; no new input surfaces, no allocation driven by untrusted sizes, no FFI.
Level of scrutiny
Moderate — it changes user-visible parser behavior, turning previously-silent (but broken) code into a hard error. I traced the gating flag: standard_decorators is derived as !loader.is_typescript() || !(experimental_decorators || emit_decorator_metadata) in both the runtime transpiler and bundler ParseTask, so TS files with experimentalDecorators: true keep standard_decorators = false and never hit the new branch (covered by the fourth new test). For JS files, standard_decorators is always true and allow_ts_decorators is set on both class-statement (parse/mod.rs:710) and class-expression (parse_prefix.rs:580) paths, so the JS message is reachable and tested via runDecorator. at_range is captured before parse_type_script_decorators() advances past the @, so the reported span is correct.
Other factors
The error text and note follow the repo's diagnostic conventions (p.log().add_range_error_with_notes with a bun_ast::Data note, matching the Cannot use import statement... call site in parse_entry.rs). Tests cover the CLI (TS + JS), Bun.Transpiler, and the negative case with the flag enabled; they follow existing harness patterns in the same file. This aligns Bun with esbuild and tsc, and the PR description reports the full decorator suites still pass. The change is small, self-contained, and the previous behavior was a silent-wrong-result bug, so an error is a strict improvement.
Contributor
There was a problem hiding this comment.
Beyond the inline note on the pre-existing extends-chain merge, I checked that opts.allow_ts_decorators in FnOrArrowDataParse is only set from parse_property.rs:107 (class methods/constructors), so this error cannot fire on plain function declarations/expressions. at_range is captured before parse_type_script_decorators() so the diagnostic points at the @ token.
Extended reasoning...
The parser change itself is small and correct — it gates on standard_decorators (false for TS with experimentalDecorators, true otherwise), branches the message on the IS_TYPESCRIPT_ENABLED const-generic, and uses the existing add_range_error_with_notes machinery. Tests cover TS ctor+method, JS, Bun.Transpiler, and the positive experimentalDecorators control. The one thing worth a human decision is whether to fold in the resolver one-liner from the inline finding, since this PR turns that pre-existing silent miscompile into a hard error whose remedy hint will look wrong for affected extends-chain users.
…hain The extends-chain merge in resolver.rs whitelists fields to copy from child configs into the merged config. emitDecoratorMetadata was on the list but experimentalDecorators was not, so a leaf tsconfig that set the flag while extending a base that omitted it had the flag silently dropped. With the new parameter-decorator error in place, that config shape would produce a hard error telling users to add a flag they already have.
Contributor
There was a problem hiding this comment.
LGTM — the resolver extends-chain fix from my earlier comment is folded in with a test, and the parser diagnostic itself is small and matches esbuild/tsc.
What was reviewed:
parse_fn.rs: confirmedallow_ts_decoratorsis set for JS class bodies too (viaparse_prefix.rs:580/mod.rs:710), so the!IS_TYPESCRIPT_ENABLEDbranch is reachable and covered by the JS test.resolver.rs: OR-merge mirrors theemit_decorator_metadataline directly above; can't distinguish unset fromfalse(same limitation as the sibling field), but strictly better than the prior deepest-base-only behavior.- Test coverage: constructor + method params, TS/JS/
Bun.Transpilerentry points, positive case withexperimentalDecorators: true, and the extends-chain leaf-override case.
Extended reasoning...
Overview
Two changes: (1) src/js_parser/parse/parse_fn.rs emits a range error when parameter decorators are parsed under standard_decorators, with a TS-specific message + tsconfig hint and a separate JS message; (2) src/resolver/resolver.rs OR-merges experimental_decorators in the tsconfig extends-chain merge loop, mirroring the existing emit_decorator_metadata line. Five new tests in es-decorators.test.ts cover the TS reject, JS reject, Bun.Transpiler reject, extends-chain positive, and direct experimentalDecorators: true positive.
Security risks
None. The parser change only adds a diagnostic on an already-parsed decorator list; no new allocations or lifetime concerns. The resolver change is a boolean OR on an already-merged struct field.
Level of scrutiny
Moderate — this converts silently-dropped parameter decorators (previously [] output, no error) into a hard parse error, which is a user-visible behavior change. However, it aligns with both tsc and esbuild, and the prior behavior (parse, store on G::Arg.ts_decorators, then silently discard in the standard-decorator lowering) was strictly wrong. The resolver fix I flagged earlier ensures the one config shape that would have been spuriously broken (leaf tsconfig sets the flag while extending a base that doesn't) now works correctly, with a test.
Other factors
at_rangeis captured beforeparse_type_script_decorators()advances the lexer, so the diagnostic points at the first@— correct.- The JS branch is reachable: class-statement parsing (
parse/mod.rs:710) and class-expression parsing (parse_prefix.rs:580) both setallow_ts_decoratorsfor JS whenstandard_decoratorsis on, andparse_type_script_decoratorsreturns non-empty for JS under that flag. - The resolver OR-merge cannot express "leaf sets
falseto override a base'strue" because the field isbool, notOption<bool>. This is the same pre-existing limitation asemit_decorator_metadataon the line above and is not made worse by this PR (before, the value was simply the deepest base's). - My prior inline comment thread is resolved; the author confirmed and folded in the resolver fix as 16ac6a7 with the extends-chain test.
- Full decorator suites reported passing (63 + 24 + 154 + 130).
One of the dropped-class-member scope tests uses a parameter decorator
(h(@(() => {})ny) {}) to exercise the scope-discard path. Parameter
decorators are now rejected under standard decorators, so pass
experimentalDecorators to the Transpiler so the test still parses.
Only the first case in the dropped-class-member scope suite uses a parameter decorator; the other six exercise member decorators and computed keys under standard-decorator parsing. Thread the flag per-case so those six keep covering the standard-decorator path.
Contributor
There was a problem hiding this comment.
LGTM — all three rounds of prior feedback are addressed and the bug hunter found nothing new.
What was reviewed:
at_rangeis captured while the lexer is on@and only read whents_decoratorsis non-empty, so the diagnostic points at the first decorator.allow_ts_decoratorsis reachable for JS class members (mod.rs:710 / parse_prefix.rs:580 gate onstandard_decorators), so the JS-branch message is live and covered by the new test.- The resolver OR-merge mirrors
emit_decorator_metadataon the line above; the leaf-cannot-set-false limitation is pre-existing and acknowledged as follow-up. - cd245e7 restores standard-decorator coverage for scope-mismatch cases 2-7 by making the flag a per-case column.
Extended reasoning...
Overview
Two source changes: (1) parse_fn.rs adds a diagnostic when parameter decorators are parsed while standard_decorators is active — the parse itself is unchanged, only p.log().add_range_error* is called, so control flow and AST shape are identical; (2) resolver.rs adds a one-line OR-merge for experimental_decorators in the tsconfig extends-chain loop, mirroring the emit_decorator_metadata line directly above. Five new tests in es-decorators.test.ts cover TS reject / JS reject / Bun.Transpiler reject / extends-chain propagation / experimental-mode still works. scope-mismatch-panic.test.ts gains a per-case experimentalDecorators column so only the one case containing a parameter decorator opts in.
Security risks
None. This is a parser diagnostic addition and a config-merge whitelist entry; no untrusted input reaches new allocation, no FFI, no I/O.
Level of scrutiny
Medium — the parser is a critical path, but the change is additive (log an error) rather than a control-flow or grammar change. The previous behavior was a silent miscompile (parameter decorator parsed then discarded), so converting it to a diagnostic that matches tsc and esbuild is a bug fix, not a policy tightening. The resolver change is a one-line pattern copy of the sibling field.
Other factors
This PR went through three prior review rounds from me: the extends-chain resolver bug (folded in as 16ac6a7), the test-coverage regression from a blanket experimentalDecorators flag (fixed in cd245e7), and the Option<bool> override limitation (acknowledged, deferred as a broader follow-up consistent with emit_decorator_metadata). All threads are resolved. Test coverage spans runtime (bun subprocess), Bun.Transpiler API, and the extends-chain config path, and the PR description shows the USE_SYSTEM_BUN=1 fail / bun bd pass verification. The full decorator suites (~380 tests) pass per the description.
Jarred-Sumner
added a commit
that referenced
this pull request
Sep 13, 2026
…0833) ### What was wrong Lowering a class with standard decorators (or a bare `accessor`) moved part of the class out of its body: decorated fields into the constructor, decorated static fields and every static block to after the class, decorator lists to before it. Whatever moved lost the context it was written in, and whatever stayed ran in a different order: ```js class A { @dec a = 1; b = this.a } // b saw a === undefined: `b` stayed, `a` moved class A extends B { @dec x = 1 } // B's `set x` ran: `this.x = …` instead of a field class A { @dec static x = this } // `this` was the module's class A extends B { @dec static x = super.y; static { super.z } } // SyntaxError const C = class Foo { @dec static s = Foo.f() } // ReferenceError: Foo class A { accessor x; static #m; static { this.#m } } // SyntaxError: #m (#31921) class A { @dec m() {} #q = 1; d = this.#q } // SyntaxError: #q (#28118) class A { static accessor a; static {} static accessor b } // ran a, b, block ``` Every `#private` member was turned into a WeakMap as soon as one member was decorated, so that moved code could still reach it, and the hand-written walker that rewrites those accesses misses many forms (`this.#x++`, `this.#x ??= v`, nested functions and classes, parameter defaults). ### The fix `lower_class_body` (`src/js_parser/lower/lower_decorators.rs`, mostly rewritten: +912 −1889) no longer moves members. Fields, static blocks and undecorated `#private` members stay native, so order, [[Define]] semantics, `this`, `super`, `new.target` and private names are right by construction instead of by rewriting. What the runtime helpers need is added around them: ```js class A extends (_base = B) { static { // first static element _init = __decoratorStart(_base); __decorateElement(_init, 1, "m", _dec2, this); __decorateElement(_init, 5, "a", _dec, this); __decoratorMetadata(_init, this); } [(_dec = [dec], "a")] = __runInitializers(_init, 8, this, 1); // still a field b = (__runInitializers(_init, 11, this), this.a); // a's extra initializers, then b [(_dec2 = [dec], "m")]() {} } ``` - **Decorator lists** are evaluated in the computed key of their member. That is their position in the spec: in order with computed keys, with the outer `this` / `await` / `arguments`, inside the class's private scope, before the class binding is initialized. A decorated `#private` member has no key and borrows a neighbor's. A class with no usable key at all (only `#private` members and a constructor) gets a `static [(lists, "__decorators")]() {}` that the leading block deletes first. - **A leading `static {}`** applies the decorators in the helper's order, the class decorators last, so they run before any static field, as the spec says. - **Decorated fields** keep their place and wrap their initializer. Decorated static fields are followed by a `static {}` for their extra initializers. - **Between two instance fields** there is nowhere to put a statement, so accessor storage, lowered `#private` fields and extra initializers ride in the initializer of the next instance field, or in the constructor after the last one. An anonymous function or class in that initializer is still named by its field: `h = (effects, { h: () => {} }.h)` (the JIT sinks the object; 1e7 constructions measure the same with and without it). - **Only `#private` names with a decorated member become WeakMaps**, because `__decorateElement` reaches them through `__privateGet`. The access rewriter now also walks nested functions, classes, parameter and destructuring defaults and object keys. - An undecorated `accessor #x` is a plain `#x` field. A class with only accessors gets no `_init` and no `Symbol.metadata`. A class expression without class decorators is returned as is, so its context names it; `(class { @dec x }).name` is `""`, not `_class$1`. The inner binding that used to name such classes (and captured references to outer variables of the same name) is gone. - The extra initializers of class decorators still run after the class is bound to its name, so they can refer to it. - `LowerUsingDeclarationsContext::finalize` (`p.rs`) moves every `export class` in front of the `try` it wraps a module with top-level `using` in. A lowered class applies its decorators in a static block and evaluates their lists in its keys, so an exported class with static blocks or computed keys now stays in place as `var A = class A {}` plus an export clause, like exported `const`s already do. - The generic `ReplaceThis` / `ReplaceRef` tree rewriter, static block extraction (and its arrow IIFE), `inner_class_ref` (`let _Foo = Foo`, which nothing read) and the `prop_copy` / `class_copy` `ptr::read`s are deleted. No new runtime helpers. - `RuntimeTranspilerCache` version 31 → 32. ### Behaviour that changes on purpose - Static fields and static blocks of a class with **class decorators** now run after the class decorators (spec, tsc); before, undecorated ones ran first. - When a class decorator **returns a different class**, static members stay on the class as written, and a static block that names its own class sees that one too (methods and fields already did). `@wrap class A { @dec static accessor n = 1 }; A.n` now throws like the native `static #n` on a subclass does; 1.4.1 installed decorated static members on what the decorator returned. - Decorated fields are defined, not assigned (the second example above). ### Tests All in `test/bundler/transpiler/es-decorators.test.ts` (68 → 416 tests): - "members stay where they are written": 18 single-purpose tests (order, [[Define]], static order, `new.target`, `super`, decorator lists in key position, class decorators before static fields, anonymous class names, function names on fields that carry effects, classes with no usable key, exported class next to a lowered `using`). 12 fail on 1.4.1; the other 6 pin what 1.4.1 already did right and the new placement has to keep. - "ES decorators lowering matrix" (330 tests, built from the fixture of the previous revision): member kind × static × private × class decorator × derived, each cell a class with undecorated instance, private and static fields and a static block around the decorated member, run as `.js`, `.ts` and through `bun build`; plus fixture sections for the reported repros (#28118, #31917, #31921, #31929) and for `super` / `this` / private forms in static code, checked against what node prints for the undecorated class. On 1.4.1 the fixture does not load. - Unchanged and passing: `es-decorators-esbuild.test.ts` (esbuild's 147 decorator tests), `decorators.test.ts`, `decorator-metadata.test.ts`, `bundler_edgecase.test.ts`, `transpiler.test.js`, `ts-use-define-for-class-fields.test.ts`, `lower-using-bun-target.test.ts`, `regression/issue/{27526,27575}`. - I also compared 500 randomly generated classes (all member kinds, static / private / computed, class and member decorators, `extends`, explicit constructors) against TypeScript's ES2022 decorator emit under node, as `bun run`, `bun build` and `bun build --minify`: no difference except 8 cases where tsc's own output throws (anonymous class with a static accessor). ### Cost Release builds of the same tree with and without `src/`, `bun build` of 3000 decorated classes (1.1 MB), user-space instructions: 529.8 M → 520.0 M bundled, 437.3 M → 429.4 M with `--no-bundle`, 568.4 M → 552.9 M with `--minify`. 3000 undecorated classes: 197.7 M → 197.6 M. The binary is 28 KB smaller. The output for that fixture is 6% larger (static blocks and computed keys). ### Not in this PR - `this.#x++`, `this.#x += v`, destructuring and `for-of` targets, `o?.#x` and tagged templates **on a decorated `#x`** still print invalid or wrong code, and `super` in a decorated `#m()` is a SyntaxError: its body still becomes a function expression. Both are unchanged; undecorated privates no longer reach that path. Fixing them needs `__privateWrapper` / `__superGet`-style runtime helpers. - Effects after the last instance field go into the constructor after a top-level `super(...)` statement, as before. `const r = super()` or `if (x) super(); else super()` still puts them first. When an instance field follows, they now ride in it and this works. - With TypeScript's `useDefineForClassFields: false`, fields of a decorated class are still defined, as on main. - From the previous revision of this PR, dropped: the `__superGet` / `__superSet` / `__superWrapper` / `__privateWrapper` helpers and the rewriters that used them (no longer needed for static code), the parse errors for `@x static {}` and parameter decorators in JS (#35544), and `var {…} = require("bun:wrap")` for CommonJS-wrapped modules (a decorated class in a `.cjs` file still fails to load; it is a module wrapper bug, not a lowering one). - A class expression evaluated more than once still shares its `var` temporaries: #38933, #38904. Fixes #28118 Fixes #31917 Fixes #31921 <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 10 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 343 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/es-decorators.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/es-decorators.test.ts: (pass) ES Decorators > class decorators > basic class decorator [310.54ms] (pass) ES Decorators > class decorators > class decorator receives correct context [292.39ms] (pass) ES Decorators > class decorators > class decorator can replace class [309.17ms] (pass) ES Decorators > class decorators > multiple class decorators apply in reverse order [377.09ms] (pass) ES Decorators > method decorators > instance method decorator [293.94ms] (pass) ES Decorators > method decorators > static method decorator [302.05ms] (pass) ES Decorators > method decorators > method decorator context has correct access [397.91ms] (pass) ES Decorators > getter decorators > getter decorator [409.87ms] (pass) ES Decorators > setter decorators > setter decorator [378.03ms] (pass) ES Decorators > field decorators > field decorator receives undefined value [297.63ms] (pass) ES Decorators > field decorators > multiple field decorators [297.73ms] ( ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (d83b736) test/bundler/transpiler/es-decorators.test.ts: (pass) ES Decorators > class decorators > basic class decorator [7.49ms] (pass) ES Decorators > class decorators > class decorator receives correct context [9.75ms] (pass) ES Decorators > class decorators > class decorator can replace class [6.42ms] (pass) ES Decorators > class decorators > multiple class decorators apply in reverse order [7.29ms] (pass) ES Decorators > method decorators > instance method decorator [6.36ms] (pass) ES Decorators > method decorators > static method decorator [6.63ms] (pass) ES Decorators > method decorators > method decorator context has correct access [6.29ms] (pass) ES Decorators > getter decorators > getter decorator [6.35ms] (pass) ES Decorators > setter decorators > setter decorator [6.33ms] (pass) ES Decorators > field decorators > field decorator receives undefined value [6.88ms] (pass) ES Decorators > field decorators > multiple field decorators [6.84ms] (pass) ES Decorators > field decorators > static field decorator [7.26ms] (pass) ES Decorators > non-ASCII string-literal keys > Bun.Transpiler output preserves the key [0.88ms] (pass) ES Deco ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/es-decorators.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/es-decorators.test.ts: (pass) ES Decorators > class decorators > basic class decorator [338.17ms] (pass) ES Decorators > class decorators > class decorator receives correct context [308.25ms] (pass) ES Decorators > class decorators > class decorator can replace class [396.76ms] (pass) ES Decorators > class decorators > multiple class decorators apply in reverse order [316.26ms] (pass) ES Decorators > method decorators > instance method decorator [391.13ms] (pass) ES Decorators > method decorators > static method decorator [393.40ms] (pass) ES Decorators > method decorators > method decorator context has correct access [317.29ms] (pass) ES Decorators > getter decorators > getter decorator [334.81ms] (pass) ES Decorators > setter decorators > setter decorator [310.19ms] (pass) ES Decorators > field decorators > field decorator receives undefined value [312.04ms] (pass) ES Decorators > field decorators > multiple field decorators [303.38ms] ( ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 625ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [1/5] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser) �[1m�[92m Compiling�[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver) �[1m�[92m Compiling�[0m bun_ini v0.0.0 (/workspace/bun/src/ini) �[1m�[92m Compiling�[0m bun_bundler v0.0.0 (/workspace/bun/src/bundler) �[1m�[92m Compiling�[0m bun_router v0.0.0 (/workspace/bun/src/router) �[1m�[92m Compiling�[0m bun_standalone_graph v0.0.0 (/workspace/bun/src/standalone_graph) �[1m�[92m Compiling�[0m bun_transpiler v0.0.0 (/workspace/bun/src/transpiler) �[1m�[92m Compiling�[0m bun_bunfig v0.0.0 (/workspace/bun/src/bunfig) �[1m�[92m Compiling�[0m bun_install v0.0.0 (/workspace/bun/src/install) �[1m�[92m Compiling�[0m bun_jsc v0.0.0 (/workspace/bun/src/jsc) �[1m�[92m Compiling�[0m bun_js_parser_jsc v0.0.0 (/workspace/bun/src/js_parser_js ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js_parser/lower/lower_decorators.rs | 2852 +++++++++---------------- src/js_parser/p.rs | 39 +- src/js_parser/visit/mod.rs | 2 +- src/js_parser/visit/visit_expr.rs | 2 +- src/jsc/RuntimeTranspilerCache.rs | 3 +- test/bundler/transpiler/es-decorators.test.ts | 1151 ++++++++++ 6 files changed, 2150 insertions(+), 1899 deletions(-) ``` </details> **gate history** · 7 passed · 1 rejected · iteration 10 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js_parser/lower/lower_decorators.rs 44 37 33 src/js_parser/p.rs 6 3 29 src/js_parser/visit/mod.rs 2 2 29 src/js_parser/visit/visit_expr.rs 1 0 28 src/jsc/RuntimeTranspilerCache.rs 1 1 28 test/bundler/transpiler/es-decorators.test.ts 3 2 27 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Collaborator
Author
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…en-sh#40833) ### What was wrong Lowering a class with standard decorators (or a bare `accessor`) moved part of the class out of its body: decorated fields into the constructor, decorated static fields and every static block to after the class, decorator lists to before it. Whatever moved lost the context it was written in, and whatever stayed ran in a different order: ```js class A { @dec a = 1; b = this.a } // b saw a === undefined: `b` stayed, `a` moved class A extends B { @dec x = 1 } // B's `set x` ran: `this.x = …` instead of a field class A { @dec static x = this } // `this` was the module's class A extends B { @dec static x = super.y; static { super.z } } // SyntaxError const C = class Foo { @dec static s = Foo.f() } // ReferenceError: Foo class A { accessor x; static #m; static { this.#m } } // SyntaxError: #m (oven-sh#31921) class A { @dec m() {} #q = 1; d = this.#q } // SyntaxError: #q (oven-sh#28118) class A { static accessor a; static {} static accessor b } // ran a, b, block ``` Every `#private` member was turned into a WeakMap as soon as one member was decorated, so that moved code could still reach it, and the hand-written walker that rewrites those accesses misses many forms (`this.#x++`, `this.#x ??= v`, nested functions and classes, parameter defaults). ### The fix `lower_class_body` (`src/js_parser/lower/lower_decorators.rs`, mostly rewritten: +912 −1889) no longer moves members. Fields, static blocks and undecorated `#private` members stay native, so order, [[Define]] semantics, `this`, `super`, `new.target` and private names are right by construction instead of by rewriting. What the runtime helpers need is added around them: ```js class A extends (_base = B) { static { // first static element _init = __decoratorStart(_base); __decorateElement(_init, 1, "m", _dec2, this); __decorateElement(_init, 5, "a", _dec, this); __decoratorMetadata(_init, this); } [(_dec = [dec], "a")] = __runInitializers(_init, 8, this, 1); // still a field b = (__runInitializers(_init, 11, this), this.a); // a's extra initializers, then b [(_dec2 = [dec], "m")]() {} } ``` - **Decorator lists** are evaluated in the computed key of their member. That is their position in the spec: in order with computed keys, with the outer `this` / `await` / `arguments`, inside the class's private scope, before the class binding is initialized. A decorated `#private` member has no key and borrows a neighbor's. A class with no usable key at all (only `#private` members and a constructor) gets a `static [(lists, "__decorators")]() {}` that the leading block deletes first. - **A leading `static {}`** applies the decorators in the helper's order, the class decorators last, so they run before any static field, as the spec says. - **Decorated fields** keep their place and wrap their initializer. Decorated static fields are followed by a `static {}` for their extra initializers. - **Between two instance fields** there is nowhere to put a statement, so accessor storage, lowered `#private` fields and extra initializers ride in the initializer of the next instance field, or in the constructor after the last one. An anonymous function or class in that initializer is still named by its field: `h = (effects, { h: () => {} }.h)` (the JIT sinks the object; 1e7 constructions measure the same with and without it). - **Only `#private` names with a decorated member become WeakMaps**, because `__decorateElement` reaches them through `__privateGet`. The access rewriter now also walks nested functions, classes, parameter and destructuring defaults and object keys. - An undecorated `accessor #x` is a plain `#x` field. A class with only accessors gets no `_init` and no `Symbol.metadata`. A class expression without class decorators is returned as is, so its context names it; `(class { @dec x }).name` is `""`, not `_class$1`. The inner binding that used to name such classes (and captured references to outer variables of the same name) is gone. - The extra initializers of class decorators still run after the class is bound to its name, so they can refer to it. - `LowerUsingDeclarationsContext::finalize` (`p.rs`) moves every `export class` in front of the `try` it wraps a module with top-level `using` in. A lowered class applies its decorators in a static block and evaluates their lists in its keys, so an exported class with static blocks or computed keys now stays in place as `var A = class A {}` plus an export clause, like exported `const`s already do. - The generic `ReplaceThis` / `ReplaceRef` tree rewriter, static block extraction (and its arrow IIFE), `inner_class_ref` (`let _Foo = Foo`, which nothing read) and the `prop_copy` / `class_copy` `ptr::read`s are deleted. No new runtime helpers. - `RuntimeTranspilerCache` version 31 → 32. ### Behaviour that changes on purpose - Static fields and static blocks of a class with **class decorators** now run after the class decorators (spec, tsc); before, undecorated ones ran first. - When a class decorator **returns a different class**, static members stay on the class as written, and a static block that names its own class sees that one too (methods and fields already did). `@wrap class A { @dec static accessor n = 1 }; A.n` now throws like the native `static #n` on a subclass does; 1.4.1 installed decorated static members on what the decorator returned. - Decorated fields are defined, not assigned (the second example above). ### Tests All in `test/bundler/transpiler/es-decorators.test.ts` (68 → 416 tests): - "members stay where they are written": 18 single-purpose tests (order, [[Define]], static order, `new.target`, `super`, decorator lists in key position, class decorators before static fields, anonymous class names, function names on fields that carry effects, classes with no usable key, exported class next to a lowered `using`). 12 fail on 1.4.1; the other 6 pin what 1.4.1 already did right and the new placement has to keep. - "ES decorators lowering matrix" (330 tests, built from the fixture of the previous revision): member kind × static × private × class decorator × derived, each cell a class with undecorated instance, private and static fields and a static block around the decorated member, run as `.js`, `.ts` and through `bun build`; plus fixture sections for the reported repros (oven-sh#28118, oven-sh#31917, oven-sh#31921, oven-sh#31929) and for `super` / `this` / private forms in static code, checked against what node prints for the undecorated class. On 1.4.1 the fixture does not load. - Unchanged and passing: `es-decorators-esbuild.test.ts` (esbuild's 147 decorator tests), `decorators.test.ts`, `decorator-metadata.test.ts`, `bundler_edgecase.test.ts`, `transpiler.test.js`, `ts-use-define-for-class-fields.test.ts`, `lower-using-bun-target.test.ts`, `regression/issue/{27526,27575}`. - I also compared 500 randomly generated classes (all member kinds, static / private / computed, class and member decorators, `extends`, explicit constructors) against TypeScript's ES2022 decorator emit under node, as `bun run`, `bun build` and `bun build --minify`: no difference except 8 cases where tsc's own output throws (anonymous class with a static accessor). ### Cost Release builds of the same tree with and without `src/`, `bun build` of 3000 decorated classes (1.1 MB), user-space instructions: 529.8 M → 520.0 M bundled, 437.3 M → 429.4 M with `--no-bundle`, 568.4 M → 552.9 M with `--minify`. 3000 undecorated classes: 197.7 M → 197.6 M. The binary is 28 KB smaller. The output for that fixture is 6% larger (static blocks and computed keys). ### Not in this PR - `this.#x++`, `this.#x += v`, destructuring and `for-of` targets, `o?.#x` and tagged templates **on a decorated `#x`** still print invalid or wrong code, and `super` in a decorated `#m()` is a SyntaxError: its body still becomes a function expression. Both are unchanged; undecorated privates no longer reach that path. Fixing them needs `__privateWrapper` / `__superGet`-style runtime helpers. - Effects after the last instance field go into the constructor after a top-level `super(...)` statement, as before. `const r = super()` or `if (x) super(); else super()` still puts them first. When an instance field follows, they now ride in it and this works. - With TypeScript's `useDefineForClassFields: false`, fields of a decorated class are still defined, as on main. - From the previous revision of this PR, dropped: the `__superGet` / `__superSet` / `__superWrapper` / `__privateWrapper` helpers and the rewriters that used them (no longer needed for static code), the parse errors for `@x static {}` and parameter decorators in JS (oven-sh#35544), and `var {…} = require("bun:wrap")` for CommonJS-wrapped modules (a decorated class in a `.cjs` file still fails to load; it is a module wrapper bug, not a lowering one). - A class expression evaluated more than once still shares its `var` temporaries: oven-sh#38933, oven-sh#38904. Fixes oven-sh#28118 Fixes oven-sh#31917 Fixes oven-sh#31921 <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 10 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 343 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/es-decorators.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/es-decorators.test.ts: (pass) ES Decorators > class decorators > basic class decorator [310.54ms] (pass) ES Decorators > class decorators > class decorator receives correct context [292.39ms] (pass) ES Decorators > class decorators > class decorator can replace class [309.17ms] (pass) ES Decorators > class decorators > multiple class decorators apply in reverse order [377.09ms] (pass) ES Decorators > method decorators > instance method decorator [293.94ms] (pass) ES Decorators > method decorators > static method decorator [302.05ms] (pass) ES Decorators > method decorators > method decorator context has correct access [397.91ms] (pass) ES Decorators > getter decorators > getter decorator [409.87ms] (pass) ES Decorators > setter decorators > setter decorator [378.03ms] (pass) ES Decorators > field decorators > field decorator receives undefined value [297.63ms] (pass) ES Decorators > field decorators > multiple field decorators [297.73ms] ( ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (d83b736) test/bundler/transpiler/es-decorators.test.ts: (pass) ES Decorators > class decorators > basic class decorator [7.49ms] (pass) ES Decorators > class decorators > class decorator receives correct context [9.75ms] (pass) ES Decorators > class decorators > class decorator can replace class [6.42ms] (pass) ES Decorators > class decorators > multiple class decorators apply in reverse order [7.29ms] (pass) ES Decorators > method decorators > instance method decorator [6.36ms] (pass) ES Decorators > method decorators > static method decorator [6.63ms] (pass) ES Decorators > method decorators > method decorator context has correct access [6.29ms] (pass) ES Decorators > getter decorators > getter decorator [6.35ms] (pass) ES Decorators > setter decorators > setter decorator [6.33ms] (pass) ES Decorators > field decorators > field decorator receives undefined value [6.88ms] (pass) ES Decorators > field decorators > multiple field decorators [6.84ms] (pass) ES Decorators > field decorators > static field decorator [7.26ms] (pass) ES Decorators > non-ASCII string-literal keys > Bun.Transpiler output preserves the key [0.88ms] (pass) ES Deco ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/transpiler/es-decorators.test.ts bun test v1.4.3 (6a92015) test/bundler/transpiler/es-decorators.test.ts: (pass) ES Decorators > class decorators > basic class decorator [338.17ms] (pass) ES Decorators > class decorators > class decorator receives correct context [308.25ms] (pass) ES Decorators > class decorators > class decorator can replace class [396.76ms] (pass) ES Decorators > class decorators > multiple class decorators apply in reverse order [316.26ms] (pass) ES Decorators > method decorators > instance method decorator [391.13ms] (pass) ES Decorators > method decorators > static method decorator [393.40ms] (pass) ES Decorators > method decorators > method decorator context has correct access [317.29ms] (pass) ES Decorators > getter decorators > getter decorator [334.81ms] (pass) ES Decorators > setter decorators > setter decorator [310.19ms] (pass) ES Decorators > field decorators > field decorator receives undefined value [312.04ms] (pass) ES Decorators > field decorators > multiple field decorators [303.38ms] ( ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 625ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [1/5] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser) �[1m�[92m Compiling�[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver) �[1m�[92m Compiling�[0m bun_ini v0.0.0 (/workspace/bun/src/ini) �[1m�[92m Compiling�[0m bun_bundler v0.0.0 (/workspace/bun/src/bundler) �[1m�[92m Compiling�[0m bun_router v0.0.0 (/workspace/bun/src/router) �[1m�[92m Compiling�[0m bun_standalone_graph v0.0.0 (/workspace/bun/src/standalone_graph) �[1m�[92m Compiling�[0m bun_transpiler v0.0.0 (/workspace/bun/src/transpiler) �[1m�[92m Compiling�[0m bun_bunfig v0.0.0 (/workspace/bun/src/bunfig) �[1m�[92m Compiling�[0m bun_install v0.0.0 (/workspace/bun/src/install) �[1m�[92m Compiling�[0m bun_jsc v0.0.0 (/workspace/bun/src/jsc) �[1m�[92m Compiling�[0m bun_js_parser_jsc v0.0.0 (/workspace/bun/src/js_parser_js ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js_parser/lower/lower_decorators.rs | 2852 +++++++++---------------- src/js_parser/p.rs | 39 +- src/js_parser/visit/mod.rs | 2 +- src/js_parser/visit/visit_expr.rs | 2 +- src/jsc/RuntimeTranspilerCache.rs | 3 +- test/bundler/transpiler/es-decorators.test.ts | 1151 ++++++++++ 6 files changed, 2150 insertions(+), 1899 deletions(-) ``` </details> **gate history** · 7 passed · 1 rejected · iteration 10 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js_parser/lower/lower_decorators.rs 44 37 33 src/js_parser/p.rs 6 3 29 src/js_parser/visit/mod.rs 2 2 29 src/js_parser/visit/visit_expr.rs 1 0 28 src/jsc/RuntimeTranspilerCache.rs 1 1 28 test/bundler/transpiler/es-decorators.test.ts 3 2 27 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
TC39 standard decorators do not include parameter decorators. When a TypeScript file used
@decon a constructor or method parameter withoutexperimentalDecoratorsenabled, Bun parsed the decorator, stored it onG::Arg.ts_decorators, and then silently discarded it: the standard-decorator lowering pass and the printer both ignore that field. The decorator function was never called and no diagnostic was emitted.Repro
Before: prints
[], no error. A DI codebase (constructor(@Inject(X) x)) whose tsconfig lost the flag gets constructors with no injection and no indication anything is wrong.Fix
parse_fn.rsnow emits a parse error when parameter decorators are found andstandard_decoratorsis active, matching esbuild and tsc:For
.jsfiles the message isParameter decorators are not allowed in JavaScript.Also fixes a pre-existing resolver bug surfaced by review: the tsconfig
extends-chain merge inresolver.rswhitelistedemitDecoratorMetadatabut notexperimentalDecorators, so a leaf tsconfig that set the flag while extending a base that omitted it had the flag silently dropped. That config shape would otherwise hit the new error with a hint telling users to add a flag they already have.Verification
Full decorator suites pass:
es-decorators.test.ts(64),decorators.test.ts(24),es-decorators-esbuild.test.ts+decorator-metadata.test.ts+bundler_decorator_metadata.test.ts(154),bundler_edgecase.test.ts(130),tsconfig-extends-leak.test.ts+tsconfig-override.test.ts(8).