Let a decorated class body observe the class its decorators return - #38731
Let a decorated class body observe the class its decorators return#38731robobun wants to merge 9 commits into
Conversation
…s return
With standard decorators, a class declaration's body resolved its own name
to the class's immutable inner binding, so after
`Foo = __decorateElement(...)` installed a replacement class, methods,
getters and field initializers inside Foo still used the undecorated class
(`static create() { return new Foo() }` built undecorated instances).
Undecorated static fields also stayed in the class body, so they were
initialized on the undecorated class before the class decorators ran.
For class statements with class decorators, visit_class now resolves the
body's references to a separate binding. The lowering declares it
(`let _Foo;`) after the pre-evaluated element decorators and computed keys,
so those still observe the TDZ, captures the class in a leading static block
and reassigns it together with the class name
(`Foo = _Foo = __decorateElement(...)`). When the body never names the class
the binding is merged into the class name and nothing extra is emitted; this
also drops the unconditional `let _Foo = Foo` the lowering used to append.
The binding is recorded as a declared symbol so the bundler renames it per
file. Classes with private static members keep the old resolution, since
those members stay installed on the class as written.
Undecorated public static fields of a class with class decorators are moved
out of the body and defined with __publicField after the class decorators
have been applied, in source order with the static blocks and decorated
fields, with `this` (and, for class expressions, the class's own name)
rewritten to the decorated class. Classes with private members keep their
static fields in place because relocated initializers cannot use private
names.
|
Status: ready for review; the diff is final. Reproduced on the released bun with the snippet in the description ( Review: relocation of undecorated static fields is all or nothing per class and only happens when the class (statement or expression) has no private static member and no static accessor, the body of a class expression does not use the expression's own name, and every initializer and computed key is a shape the rewriter fully covers; otherwise every static field stays in the body exactly as before. Relocating also pre-evaluates every computed key, so key order and the class binding's TDZ are preserved (56659df, 333c9d1, fa14851, 6366245, 7e0342c, 72086c7). All review threads are resolved. CI (build 97278 on 72086c7, finished): 177 of 179 jobs passed with no failing tests; the handful of retried or batch-only flakes were in unrelated files (install, nodemailer, napi, importstar_ts, inspect-error-leak, 07261, runtime-transpiler). The two remaining jobs, the darwin 14 aarch64 test lanes, expired without ever being picked up: that queue (six arm64 previous-tier agents) is currently backlogged for every build, not something in this PR. The diff itself is green; a retrigger has already been used once, so I am leaving it for a maintainer. |
|
Warning Review limit reached
Next review available in: 18 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 (6)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 72086c7 has some failures in 🧪 To try this PR locally: bunx bun-pr 38731That installs a local version of the PR into your bun-38731 --bun |
Relocating an undecorated static field printed its initializer outside the class body verbatim, so `super.x`, `new.target`, private names, and `this` in positions rewrite_expr does not visit (arrow parameter defaults, computed object keys) either became syntax errors or lost their receiver. Relocation is now limited to expression shapes the rewriter walks completely (can_leave_class_body); anything else stays in the body as before. Relocating a field also hoists its computed key ahead of the class, which reordered it relative to the computed keys of members left in the body. When static fields are relocated, every computed key in the class is now pre-evaluated in source order (and relocation is skipped if one of them cannot be), which also keeps those keys observing the class binding's TDZ.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/lower/lower_decorators.rs:1827-1839— The per-fieldcan_leave_class_body(&init)gate relocates each static field individually, so@wrap class Foo extends Base { static a = 1; static b = super.x; static c = 3 }now initializesbfirst (in-body, on the undecorated class) anda/cafterwards (in suffix, on the replacement) — bun 1.4.0 initialized[a, b, c]in source order. The added test 'initializers that only work inside the class body stay there' lists all four non-relocatable fields before the two relocatable ones, so it doesn't catch the reordering. Fold the initializer check intorelocate_static_fields(all-or-nothing, same as computed keys already are): if any static field's initializer failscan_leave_class_body, keep every static field in the body.Extended reasoning...
What the bug is
The follow-up commit added
can_leave_class_bodyto keep initializers that usesuper/new.target/arrows/#names/computed-key objects inside the class body (fixing the earlier review comments about SyntaxErrors andrewrite_exprgaps). But it applies that check per field at lower_decorators.rs:1827-1839, whilerelocate_static_fields(:1316-1320) is decided once for the whole class and only checks computed keys. So when a class-decorated class mixes 'simple' and 'complex' static initializers, the complex ones fall through tonew_properties.push(prop_full_copy(prop))and stay in the class body (running during ClassDefinitionEvaluation, before the class decorator), while their simple siblings become__publicField(...)calls insuffix_exprs(running after__decorateElement).Step-by-step proof
const log = []; function wrap(cls, ctx) { log.push('decorator'); return class W extends cls {}; } class Base { static x = 0; } @wrap class Foo extends Base { static a = (log.push('a'), 1); static b = (log.push('b'), super.x); // ESuper → can_leave_class_body = false static c = (log.push('c'), 3); }
relocate_static_fields:class_decorators_len = 1, no computed keys →true.a:is_plain_static_field✓, initializer isEBinary(ECall, ENumber)→can_leave_class_body✓ → pushed torelocated_static_fields, recorded instatic_element_orderasPlainField.b:is_plain_static_field✓, but initializer containsESuperwhich hits_ => false→ falls through tonew_properties.push, stays in the body.c: same asa→ relocated.
Emitted (roughly):
let _Foo; var _init = __decoratorStart(undefined); class Foo extends Base { static { _Foo = this; } static b = (log.push('b'), super.x); // runs here } Foo = _Foo = __decorateElement(_init, 0, 'Foo', _dec, Foo); __publicField(Foo, 'a', (log.push('a'), 1)); // runs here __publicField(Foo, 'c', (log.push('c'), 3));
Observable result:
log = ['b', 'decorator', 'a', 'c'],Object.hasOwn(original, 'b') === truebutObject.hasOwn(original, 'a') === false. bun 1.4.0 producedlog = ['a', 'b', 'c', 'decorator']with all three own properties of the original class — not spec-perfect (spec runs the decorator first), but source-ordered and consistent, which is what code depending on 'the field above me is already set' relies on.Why the added tests don't catch it
The 'initializers that only work inside the class body stay there' test lists
fromSuper,newTarget,thisInParameter,thisInComputedKey(all non-relocatable) beforerelocatedandinstance(both relocatable), so the split coincidentally preserves source order and the ownership assertions pass. The 'class with instance private members' test even asserts the split ownership (Object.hasOwn(original, 'peek')/Object.hasOwn(Foo, 'self')) but doesn't log initialization order — same accident, since the arrow/#namefields are again listed before the relocatableself.Impact
Any class-decorated class that has a 'simple' static field textually before a static field whose initializer is an arrow, a function/class expression, uses
super.,new.target,#name, or an object with a computed key — a common DI/mixin pattern — now (a) initializes those fields out of source order, so an initializer reading the value of an earlier field seesundefined, and (b) puts the two groups on different class objects, soObject.hasOwn/Object.keyson the decorated class silently misses the ones left behind.Fix
Make the initializer check all-or-nothing at the class level, exactly as the follow-up already does for computed keys — fold it into
relocate_static_fields:let relocate_static_fields = class_decorators_len > 0 && class.properties.slice().iter().all(|prop| { (!prop.flags.contains(Flags::Property::IsComputed) || prop.key.is_none_or(|key| can_leave_class_body(&key))) && (!is_plain_static_field(prop) || prop.initializer.is_none_or(|init| can_leave_class_body(&init))) });
Then the per-field guard at :1827-1831 reduces to
relocate_static_fields && is_plain_static_field(prop). When any one initializer can't leave the body, every static field stays in place — same order and same owner as bun 1.4.0. And add a test that interleaves a non-relocatable initializer between two relocatable ones and asserts the log order.
Deciding per field split the static fields of one class between the body (initialized before the class decorators, on the class as written) and the suffix (after them, on the decorated class), which reordered them relative to each other and put them on different objects. If any static field initializer has to stay in the body, all of them now stay, as before.
|
Re the additional finding on the per-field |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/lower/lower_decorators.rs:1793-1805— The per-fieldcan_leave_class_bodygate relocates each static field independently, so a field that stays behind (arrow /super/new.targetinitializer) can be interleaved with siblings that move to the suffix — reordering their initializers and splitting ownership.@wrap class Foo { static config = {limit:10}; static make = () => this.config.limit }putsconfigon the replacement but leavesmake's lexicalthisas the original, soFoo.make()throwsTypeErrorwhere 1.4.0 returned10. Mirror the computed-key bail-out at line 1285: if any plain static field's initializer failscan_leave_class_body, relocate none.Extended reasoning...
What the bug is
56659df narrowed relocation to fields whose initializers pass
can_leave_class_body, but applies the check per field at lower_decorators.rs:1793-1805. A field that fails falls through tonew_properties.push(prop_full_copy(prop))and stays in the class body; a sibling that passes goes intostatic_element_orderand is emitted in the suffix after__decorateElement. When a stay-behind field sits between two relocatable ones, or before one that reads it, the split changes both initialization order and which class the properties land on.Why nothing else prevents it
relocate_static_fields(line 1285) is a class-wide flag, but it gates only on computed keys — there is no class-wide check on initializers. So a class likestatic a = 1; static b = super.x; static c = 3hasrelocate_static_fields = true, thenaandcrelocate whilebstays. The PR's test "initializers that only work inside the class body stay there" places all four stay-behind fields (fromSuper,newTarget,thisInParameter,thisInComputedKey) at indices 0–3 and both relocated fields at 4–5, so no interleaving is exercised, and it asserts only ownership, not relative order.Step-by-step proof — reordering
const log = []; function wrap(cls, ctx) { log.push('dec'); return class W extends cls {}; } class Base { static x = 0; } @wrap class Foo extends Base { static a = (log.push('a'), 1); // ENumber → passes → relocated static b = (log.push('b'), super.x); // EDot(ESuper) → '_ => false' → stays static c = (log.push('c'), 3); // → relocated }
Emitted (roughly):
let _Foo; class Foo extends _base { static { _Foo = this; } static b = (log.push('b'), super.x); // stays in body — runs FIRST } Foo = _Foo = __decorateElement(...); // 'dec' __publicField(Foo, 'a', (log.push('a'), 1)); __publicField(Foo, 'c', (log.push('c'), 3));
- This PR:
['b', 'dec', 'a', 'c']—aandbare swapped relative to source. - bun 1.4.0:
['a', 'b', 'c', 'dec']— off-spec placement, but source order preserved. - Spec:
['dec', 'a', 'b', 'c'].
So this trades one off-spec behaviour for a different off-spec behaviour that additionally reverses
aandb. Any observable side effect ina(mutation, throw, dependency ofb) now runs in the wrong order.Step-by-step proof — split
this(working → TypeError)function wrap(cls, ctx) { return class W extends cls {}; } @wrap class Foo { static config = { limit: 10 }; // EObject, non-computed key, ENumber value → passes → relocated static make = () => this.config.limit; // EArrow → '_ => false' → stays in body } Foo.make();
configis emitted as__publicField(Foo, 'config', {limit:10})— an own property of the replacement.makestays in the class body, so the arrow's lexicalthisis the original class (the one being defined whenstatic { _Foo = this }runs).Foo.make(inherited from the original) invokes the arrow → readsoriginal.config→undefined→.limitthrowsTypeError: Cannot read properties of undefined.
In 1.4.0 both fields stayed in the body, both were own properties of the original, the arrow's
this.configwas{limit:10}, andFoo.make()returned10. This is a working→broken regression on a plausible pattern (a config object plus a factory arrow that reads it).Fix
Mirror the existing computed-key all-or-nothing bail-out at line 1285: fold an initializer scan into
relocate_static_fieldsso that if anyis_plain_static_fieldhas an initializer that failscan_leave_class_body, no field is relocated. That preserves 1.4.0 semantics for classes containingsuper/new.target/arrow/function/class/computed-object-key static initializers while keeping the new behaviour for the common all-plain-values case, and requires no newrewrite_exprmachinery. (Alternatively, keep the per-field split but push the stay-behind initializer intostatic_element_orderas a static-block wrapper at the same index — more work, but closer to spec.) - This PR:
|
The second "additional findings" review describes the same per-field gate from 56659df; it is already replaced by the all-or-nothing check in 333c9d1 (see the comment above). Both examples from it now behave exactly as on the released bun with this branch: |
…r static accessors Undecorated private static fields stay in the class body and undecorated static auto-accessors are initialized before the class decorator runs, so neither goes through the source-ordered part of the suffix. Relocating the public static fields around them moved those out from under them; a class containing either now keeps all of its static fields in place.
…ted class A private static method (not just a field) already keeps the body's references on the class as written, and a named class expression's body always refers to the expression's own binding, yet static fields were still moved onto the decorated class in both cases, so a body reading them by name found nothing. The relocation gate now uses the same private-static check as the inner binding and, for class expressions, requires the body not to use the expression's name; the rewrite of that name in relocated initializers is therefore gone.
…ate statics too The private static check only applied to class statements; a class expression with a private static field still had its public static fields relocated around it, reordering the initializers. Both forms now share it.
There was a problem hiding this comment.
I reviewed this iteration and found no new issues — the four predicate-mismatch regressions from earlier rounds (private static field/accessor ordering, private static methods, decorated private statics, and the expression-branch gap) are each fixed and pinned by tests. Given the size of the semantic change to decorator lowering and the number of interacting gates (wants_inner_class_binding, keep_static_fields_in_body, can_leave_class_body), a maintainer look at the overall design would still be worthwhile.
What was reviewed:
keep_static_fields_in_bodynow shareshas_private_static_memberacross statement and expression branches, and agrees withwants_inner_class_binding— the two gates can no longer disagree on whether the body names the original class.visit_class's return value changed fromshadow_reftoinner_class_ref; all three callers were checked (the two statement callers thread it tolower_class, the expression caller discards it and receivesRef::NONE), and the removedshadow_ref.set(Ref::NONE)branch was dead (no caller read it).can_leave_class_bodywhitelist vsrewrite_exprcoverage — arrows, functions,super,new.target, private names, and computed object keys are excluded, so relocation only fires on shapes the walker fully rewrites.
Extended reasoning...
Overview
This PR changes standard-decorator lowering in src/js_parser/lower/lower_decorators.rs (~260 LOC) plus visit_class in visit/mod.rs and its three call sites, so that a decorated class body's references to its own name resolve to a reassignable _C binding that is updated after the class decorator runs, and undecorated public static fields are relocated to run after decoration (via __publicField) when it is safe to do so. 528 lines of new tests cover the replacement-class behavior, initialization order, computed-key pre-evaluation, the various bail-outs (super/new.target/arrows/private statics/static accessors), per-evaluation bindings, TDZ, class expressions, exports, and bundling.
Security risks
None. This is a transpiler code-generation change; no untrusted-input parsing surface is added, no syscalls, no network. The can_leave_class_body recursion is bounded by the AST it walks.
Level of scrutiny
High. This changes user-visible transpilation semantics (when static fields initialize, which class body references resolve to) in a subtle area of the spec. Over the review it went through four rounds of predicate-mismatch regressions — each time a static-element kind slipped through one gate but not the other, reordering initializers or splitting them across the original and replacement class. Each was fixed with a test, and the final keep_static_fields_in_body = has_private_static_member(class) || (is_expr && name-used) is now structurally aligned with wants_inner_class_binding. Still, the number of interacting conditions and the whitelist-based can_leave_class_body design are the kind of thing a maintainer should sign off on rather than land purely on automated review.
Other factors
All four earlier findings are resolved and covered by tests that pass on the released bun (regression guards) or fail on it (behavior fixes), per the PR's evidence block. The esbuild conformance suite and the other decorator/transpiler suites are reported passing. The visit_class return-value change was traced through every caller and the removed shadow_ref reset was confirmed dead. No outstanding unresolved review threads remain.
|
Heads-up for the |
2 participants
Problem
static create()under a class-replacing decorator (DI / mixin decorators) builds undecorated instances.visit_class(src/js_parser/visit/mod.rs) resolves the body'sCto the same symbol as the outerC, so the printedclass C { ... new C ... }binds to the class's own immutable inner binding. The lowering (src/js_parser/lower/lower_decorators.rs, step 5) only reassigns the outer binding:C = __decorateElement(...). Thelet _C = Cit appended after everything only served the TDZ emulation for element decorator expressions.static selfRef = Cran while the class was being defined, i.e. before the class decorator was even called, and landed on the undecorated class. Per spec (and tsc's emit) static fields are initialized after the class decorators, on the class they returned, andthisin their initializers is that class.Fix
visit_class: for a class statement that has class decorators, the body's references to the class name now resolve to a separate symbol (_C), created in the scope containing the statement and handed tolower_class. Ordinary name resolution does the rewriting, so every position in the body (nested functions, shadowing, etc.) is covered.lower_impl, statement mode, when that symbol is referenced:let _C;after the pre-evaluated element decorators / computed keys (they still hit the TDZ the spec gives them; the esbuild conformance test "Class binding" keeps passing) and before the class statement;let, so a class statement in a loop body or function gets a fresh binding per evaluation;static { _C = this }as the first element, so anything still evaluated inside the body sees the class while it is being defined;C = _C = __decorateElement(...);class Cproduced two top-levellet _Cin one bundle, a SyntaxError).let _C = C;that every lowered class statement used to end with.wants_inner_class_binding): those members stay installed on the class as written, soC.#countinside the body has to keep naming that class; pointing it at the replacement would turn such accesses into brand-check TypeErrors. esbuild behaves the same way for these.__publicField(C, key, init)after the class decorators are applied (__publicFieldkeeps [[Define]] semantics).thisin the initializer is rewritten to the decorated class and, for class expressions, the class's own name to_class, with the existingrewrite_exprwalker.can_leave_class_body: literals, identifiers,this, calls,new, member access, operators, arrays, objects without computed keys, templates). If one of them is not, in particularsuper.x,new.target, private names, or an arrow / function / class (whose parameter defaults, keys and heritage the walker does not visit), or if the class has any private static member (for statements the body then keeps naming the class as written, see above; and a private static field is initialized in the body, so moving its siblings would reorder them) or an undecorated static auto-accessor (its storage is initialized outside the source-ordered part of the suffix), every static field stays in the class body, in source order, and behaves exactly as before; arrows are lazy anyway, sostatic create = () => new C()still picks up the decorated class through_C._computedKeytemp, as decorated members already did), so key evaluation keeps its source order and undecorated members' keys observe the class binding's TDZ as well._classis a hoisted temp shared by every evaluation), so a named class expression's body still sees the undecorated class, same as esbuild. For the same reason their static fields are only relocated when the body never uses the expression's name (anonymous expressions being the common case); otherwise they stay with the class the body names, as before.test/bundler/transpiler/es-decorators.test.ts, newclass body observes the class returned by a class decoratorblock (16 tests): the report's repro plus getters / instance fields /this, initialization order and ownership of relocated statics, computed keys interleaved with in-body members (plus the bail-out), numeric / quoted keys, a class with onesuper/new.target/ arrow initializer keeping all of its fields in the body in order (and its arrow still seeing the decorated class), the same for a class statement or expression with a private static field, or a static accessor, the common eager shapes all relocating together, [[Define]] vs inherited setter, per-evaluation bindings (function and loop), TDZ for heritage / element decorators / computed keys, instance-private and private-static classes (including a private static method next to a public static field the body reads), anonymous and self-referencing class expressions,export/export default(with an imported value in an initializer), a two-fileBun.build(plain and minified), and that a class which never names itself gets no extra binding. 12 fail on the released bun (the TDZ, private-static and the two bail-out ones are regression guards), all pass with this build.es-decorators-esbuild.test.ts(esbuild's 147 conformance tests),es-decorators.test.ts,decorators.test.ts,decorator-metadata.test.ts,bundler_decorator_metadata.test.ts,ts-use-define-for-class-fields.test.ts,regression/issue/27575.test.ts,transpiler.test.js,esbuild/ts.test.ts,runtime-transpiler.test.ts: all pass.cargo clippy -p bun_js_parserclean.Background
lower_decorators.rs) is modeled on esbuild's: the class is printed mostly as written, followed by a suffix of__decorateElement(...)calls that apply the decorators; a class decorator's result is assigned back to the class binding. Static blocks and decorated static fields are moved into that suffix so they run after decoration, withthisrewritten to the class binding byrewrite_expr.class C { m() { return C } }; C = 1; new ...m()still returns the class). A class decorator that returns a replacement only ever updates the outer one, which is why the body needs its own reassignable binding. tsc solves the same problem by printing the class as an anonymous expression and rewriting body references to a_classThisvariable.thisbeing that class; heritage, element decorator expressions and computed keys are evaluated before that, while the binding is in its TDZ.__publicField(obj, key, value)(runtime.js) defines a data property the way field syntax does, instead of a[[Set]]that would run inherited setters.this/ class name in the already-relocated decorated initializers of class expressions, and wideningrewrite_expr;can_leave_class_bodycan grow with it), Give decorator lowering temporaries file-unique names #31930 (collisions between the generated_init/_dec/_Cnames and user identifiers; this PR keeps the existing naming scheme), js_parser: keep [[Define]] semantics for decorated class fields #35537 (__publicFieldfor decorated fields), Lower super in static blocks and static initializers relocated by decorator lowering (stacked on #38769) #38730 (super.xin already-relocated static code).Emitted code for the repro, before and after
Before:
After:
Behavior matrix (this build vs bun 1.4.0)
C,this.x,new C(), literals, objects, templates...)thisis the replacementsuper,new.target,#names, an arrow, function or class, a computed key that cannot be pre-evaluated, a private static member, or a static accessor_C)let _C = C)class C, bundledlet _Ctwice)_C/_C2[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file