Lower the public fields of a class whose private members get lowered by decorator lowering - #38742
Lower the public fields of a class whose private members get lowered by decorator lowering#38742robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 37 seconds 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 (2)
Comment |
When a class has both a private member and a decorated member, the standard decorator lowering moves every private member into WeakMap/WeakSet storage, but left the undecorated public fields in the class body and never rewrote the initializers it had moved into the constructor or into static blocks. Field initializers that mention a lowered #name were printed as written and failed to parse, and the fields that stayed in the body initialized before the private ones. Move the undecorated public fields out of the body as well, as __publicField calls in source order next to the private __privateAdd calls (one ordered sequence per side: private method adds, then method extra initializers, then the fields), pre-evaluate their computed keys like decorated members' keys, and run the private access rewrite over the moved initializers.
6493e7d to
815bded
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// An undecorated public field (`x = 1`, `static [k];`, `1n;`, …), i.e. an | ||
| /// element whose key can be passed to `__publicField` as-is (a literal, or a | ||
| /// computed key once Phase 2 has hoisted it into a temporary). The visitor also | ||
| /// synthesizes initializer-less fields keyed by an `E::Identifier` for TypeScript | ||
| /// parameter properties; those have nothing to evaluate and stay in the body. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Emit `__privateAdd(this, storage[, value])` into the instance list (spliced | ||
| /// into the constructor) or the static list (a static block at the top of the | ||
| /// class body); `this` is the instance or the class respectively. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Emit `__publicField(this, key[, initializer])` for an undecorated field | ||
| /// that leaves the class body; same placement rules as `emit_private_add`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // With a private and a decorated member, every private member is lowered | ||
| // to WeakMap/WeakSet storage and its initializer moves into the | ||
| // constructor / a static block. The public fields move along: a field left | ||
| // in the body would initialize before all of them, i.e. out of source | ||
| // order and before the storage its rewritten initializer reads exists. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Elements that leave the class body must have their computed key | ||
| // evaluated here, once, at class definition time. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Method `__privateAdd`s run before the field initializers, which stay in | ||
| // source order. Instance lists are spliced into the constructor in Phase 7, | ||
| // static lists become a static block. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The public static fields stay in the body, so there is no | ||
| // ordered static sequence to join; initialize after the class. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Only `lower_all_private` classes get here, so every field initializer | ||
| // is in `*_field_inits` or `*_init_entries`, none in the body or suffix. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Private methods are installed first, then the extra initializers added | ||
| // by method/accessor decorators run, then the fields initialize in order. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing in favor of #35708. The duplicate check above is right about #35708 (not about #31405, which leaves the in-body field case throwing at construction and has changes requested). #35708 fixes the same defect by keeping the undecorated fields in the class body and emitting the lowered private members' The test cases from this branch are linked from #35708 in case they are useful there. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Elements that leave the class body must have their computed key | ||
| // evaluated here, once, at class definition time. | ||
| let leaves_body = | ||
| prop.ts_decorators.len_u32() > 0 || (lower_all_private && is_public_field(prop)); | ||
| if prop.flags.contains(Flags::Property::IsComputed) && prop.key.is_some() && leaves_body | ||
| { | ||
| computed_key_counter += 1; | ||
| let key_name: &'a [u8] = if computed_key_counter == 1 { |
There was a problem hiding this comment.
🟡 Hoisting only the field's computed key to pre_eval_stmts while an undecorated method's computed key stays in the body reverses their relative evaluation order: in a lower_all_private class, class C { #p; [k1()](){}; [k2()] = 1; @dec m(){} } now runs k2() before k1(), whereas before this PR (and per spec) both stayed in the body and ran in source order. Very narrow trigger (side-effectful computed keys on both an undecorated method and field, plus a private + decorated member), and the same mismatch already exists for decorated members' hoisted keys — the full fix (hoist all in-body computed keys once any leaves, as esbuild does) can be a follow-up.
Extended reasoning...
What changed
Phase 2's key-hoisting condition went from prop.ts_decorators.len_u32() > 0 to leaves_body = prop.ts_decorators.len_u32() > 0 || (lower_all_private && is_public_field(prop)). When leaves_body is true and the key is computed, the key expression is stashed into a temporary and pushed into pre_eval_stmts (line 1361), which is emitted at line 2596 via out.extend_from_slice(&pre_eval_stmts) — before the class statement itself. In-body computed keys, by contrast, evaluate during ClassDefinitionEvaluation in source order.
is_public_field (line 148) requires !IsMethod and value.is_none(), so an undecorated public method with a computed key does not satisfy leaves_body: its key stays in the body (Phase 4 falls through to new_properties.push(prop_full_copy(prop)) at line 1709). An undecorated public field with a computed key does satisfy it, so its key is hoisted.
Why this is a regression
Before this PR, in a lower_all_private class the undecorated field also fell through to new_properties.push(prop_full_copy(prop)) in Phase 4, so both the method's and the field's computed keys evaluated inside the class body in source order. After this PR the field's key is evaluated in pre_eval_stmts (before the class) while the method's is evaluated inside the class body, so the field's key always runs first regardless of source position.
Step-by-step proof
function dec(v, ctx) {}
let n = 0;
class C {
#p; // triggers has_any_private
[(console.log('method-key'), n++)]() {} // undecorated method, computed key
[(console.log('field-key'), n++)] = 1; // undecorated field, computed key
@dec m() {} // triggers has_any_decorated
}lower_all_private= true (private member + decorated member).- Phase 2, method element:
ts_decorators.len_u32() == 0,is_public_field→ false (IsMethodset) ⇒leaves_body= false ⇒ key not hoisted. - Phase 2, field element:
ts_decorators.len_u32() == 0,is_public_field→ true (IsComputed, notIsMethod,value.is_none()) ⇒leaves_body= true ⇒ key hoisted:pre_eval_stmts.push(var _computedKey = (console.log('field-key'), n++)). - Phase 4: method →
new_properties.push(prop_full_copy)with its original computed key; field →emit_public_fieldintoinstance_field_initswith key =_computedKey. - Emission:
pre_eval_stmtsruns first (field-key,nbecomes 1), then the class definition evaluates the in-body method key (method-key,nbecomes 2).
Native / bun 1.4.0: method-key, field-key; method has key 0, field has key 1.
After this PR: field-key, method-key; field has key 0, method has key 1.
The added test "instance fields initialize in source order with computed keys evaluated once" only asserts keys == 1 (evaluate-once); it has no other in-body computed key to observe relative order against, so this regression is not caught.
Impact and severity
This is an observable spec deviation (computed keys must evaluate in source order), but the trigger is contrived: it requires side-effectful computed keys on both an undecorated method and an undecorated field, in a class that also has a private member and a decorated member. Nothing crashes or throws — only the relative ordering of side effects in the key expressions changes (and, if those side effects produce the key value as in the n++ example, the two elements swap keys).
The same mismatch already exists pre-PR between decorated members' hoisted keys and undecorated in-body keys, so the underlying "hoist some but not all" imperfection is not new; this PR extends it to the undecorated-method ↔ undecorated-field pairing, which used to be correct. The PR description's claim that for fields not mentioning a private name "the only behavior change for them is the order fix" is slightly overstated on this axis.
Fix
The complete fix is esbuild's rule: when lower_all_private causes any computed key to be hoisted, hoist all in-body computed keys (methods, getters, setters included) into pre_eval_stmts in source order, so their relative evaluation order is preserved. That is a broader change than this PR's scope and can reasonably be a follow-up; a minimal alternative for now would be a comment noting the known limitation next to the leaves_body definition.
| // Same order as the constructor, as a single static block. | ||
| let mut static_inits = static_private_method_adds; | ||
| static_inits.extend_from_slice(&static_field_inits); | ||
| if !static_inits.is_empty() { | ||
| let block = p.make_static_block(&static_inits, loc); | ||
| new_properties.insert(0, block); | ||
| } |
There was a problem hiding this comment.
🟡 The new "private methods are installed before any field initializer runs" test only covers undecorated #sm(); the decorated variant (static x = C.#sm(); @dec static #sm(){}) still fails, because _sm_fn is declared with no value in prefix_stmts and only assigned by __decorateElement in suffix_exprs, after the static block runs — so __privateMethod(C, _sm, _sm_fn).call(C) throws TypeError: undefined is not a function. Not a regression (was SyntaxError before this PR), and the same applies to decorated static private getters/setters/accessors read from a moved static field; probably worth listing alongside the other "Not changed" items or handling in a follow-up.
Extended reasoning...
What the gap is
The PR splits private-method __privateAdd calls into static_private_method_adds and prepends them to the top-of-body static block so that static x = C.#sm() can run before #sm is declared in source order. That works for undecorated private static methods because at line 1533-1540 the method body is assigned to its function ref (_sm_fn = function(){…}) in prefix_stmts, which run before the class expression is evaluated. So by the time the static block executes, _sm (the WeakSet) has the class in it and _sm_fn holds the function.
For a decorated static private method the picture is different. At line 1797 the function ref is declared with no initializer (var_decl2(ws_ref, Some(wse), fn_ref, None) → var _sm = new WeakSet, _sm_fn;), and the actual assignment _sm_fn = __decorateElement(_init, 25, "#sm", _sm_dec, C, _sm) is pushed into static_non_field_elements at line 2003, which flows into suffix_exprs at line 2075 and executes after the class body has finished evaluating. The __privateAdd(this, _sm) for the WeakSet does go into static_private_method_adds (line 1995-2000), so the receiver check inside __privateMethod passes — but the third argument, _sm_fn, is still undefined.
Concrete walk-through
function dec(v, ctx) { return v; }
class C {
static x = C.#sm();
@dec static #sm() { return 20; }
}static x = C.#sm()is undecorated, matchesis_public_field, andlower_all_privateis set →emit_public_fieldpushes__publicField(this, "x", C.#sm())intostatic_field_inits(line 1700-1707).- Phase 5 rewrites
C.#sm()instatic_field_initsto__privateMethod(C, _sm, _sm_fn).call(C)(line 2035,rewrite_private_accesses_in_stmts). - Lines 2461-2465 assemble
static_private_method_adds ++ static_field_initsinto a single static block inserted at index 0 of the class body. - At runtime: prefix stmts run (
var _sm = new WeakSet, _sm_fn;— no value), the class body evaluates, the static block runs__privateAdd(this, _sm)then__publicField(this, "x", __privateMethod(C, _sm, _sm_fn).call(C)).__privateMethod(src/runtime.js:234) does__accessCheck(obj, member); return method;— the access check passes (step 3 addedthisto_sm), and it returns_sm_fn, i.e.undefined..call(C)onundefinedthrowsTypeError. - Only after the class body would
suffix_exprsrun_sm_fn = __decorateElement(...).
Native / esbuild / tsc all evaluate this to C.x === 20. The instance-side equivalent (x = this.#m(); @dec #m(){}) is not affected, because suffix_exprs run before any new C().
Why this isn't caught by the new tests
The PR's test "private methods are installed before any field initializer runs" uses @dec n() {} on a public method plus undecorated #m() / static #sm(). Undecorated private methods take the line-1533 path where _fn is assigned in the prefix, so the test passes. There is no test where the private method being called from a moved static field is itself decorated. The same failure mode applies to decorated static private getters/setters/accessors (their _get/_set/_acc refs are also only assigned in the suffix).
Why this is a nit, not a blocker
Before this PR, the same source produced SyntaxError: Cannot reference undeclared private names — Phase 5 rewrote only nprop.value, never nprop.initializer, so the raw #sm stayed in the body pointing at a removed declaration. After this PR it produces a TypeError at class-definition time. Both fail at module load; nothing that previously worked is broken. The PR description's "Not changed" section already acknowledges that decorated static elements run after the class, and fixing this would require reordering the __decorateElement assignments for private static methods relative to the static block — a structurally separate change (and one that has to be careful because for public methods __decorateElement needs the class object, which doesn't exist until the body starts evaluating; for private methods it does not, so hoisting just those assignments ahead of the static block is a plausible fix).
Suggested action
Either add the decorated-static-private-method case to the "Not changed (pre-existing)" list in the PR description, or open a follow-up that emits the private-method __decorateElement assignments before the static block (they don't reference the class object, only _init, the decorator array, and the WeakSet, all of which exist in the prefix). A test alongside the existing "private methods are installed before any field initializer runs" case with @dec static #sm() instead of an undecorated one would pin it.
1 participant
Closes #28118
Problem
SyntaxError: Cannot reference undeclared private names: "#sp"("#p"without the static pair). Same failure for the shapes inthis.#fieldnot rewritten in class field initializers when class has@decorated accessor#28118, where the initializer that mentions the private name is itself a private field (#callback = () => this.#name,static #m = ...used from a field).lower_implinsrc/js_parser/lower/lower_decorators.rslowers every private member of such a class (lower_all_private) to WeakMap/WeakSet storage, so the#namedeclarations disappear from the class body, butnew_properties, Phase 5 rewrotevalue, which fields do not use), and__privateAdd(this, _p, <initializer>)calls Phase 4 built for private fields and undecorated accessors (constructor_inject_stmts,static_private_add_blocks,suffix_exprs) were not passed through the Phase 5 rewrite at all.x = __privateGet(this, _p)would throw a TypeError because__privateAdd(this, _p, 1)has not happened yet. Ordering is already observable today without any#namein an initializer:static pub = 3; static #priv = O.pub * 2givesNaN, because the private adds run from a static block at the top of the body, before the public statics.Fix
lower_all_privateis set, undecorated public fields leave the body too, as__publicField(this, key[, init])statements emitted in source order into the same lists as the private field adds (instance_field_inits/static_field_inits); undecorated static accessors join the static list instead of the suffix in this case. Initializer-less fields still get defined (__publicField(this, "y")), so own-property order is kept. Computed keys of these fields are pre-evaluated once in Phase 2 exactly like decorated members' keys, so they are not re-evaluated per construction. The initializer-less fields the visitor synthesizes for TypeScript parameter properties (identifier key) are left in the body, where they already come first, matching tsc's order.__privateAdds are collected separately and emitted first (*_private_method_adds), so a field may call a private method declared later in the class, as it can natively. Assembly order per side is: method adds,__runInitializers(_init, 5, this)(method extra initializers, which the spec runs before fields), undecorated fields in source order, then the decorated fields Phase 7 already appended. Statics become one static block at the top of the body instead of one block per private member.debug_asserts pin that noinitializeris left innew_propertiesand that Phase 4 put nothing insuffix_exprs), and each initializer is rewritten exactly once.__runInitializers(_init, 5, this); __privateAdd(this, _p, 1); __publicField(this, "x", __privateGet(this, _p) + 1).__publicFieldkeeps the [[Define]] semantics the fields had as native fields (an inherited setter is not invoked), andthisis still the instance / class in the constructor / static block, so initializers need no other rewriting. Classes withoutlower_all_privateare untouched; in classes with it, fields that did not mention a private name were already being constructed through the injected constructor, so the only behavior change for them is the order fix. The gating is disjoint from Let a decorated class body observe the class its decorators return #38731, which relocates the statics of class-decorated classes without private members.test/bundler/transpiler/es-decorators.test.ts, new blockfields in classes with lowered private members(14 tests): the repro, thethis.#fieldnot rewritten in class field initializers when class has@decorated accessor#28118 shapes, static private fields and accessors, instance and static initialization order with computed / symbol / numeric / quoted keys, private-before-public dependencies, private methods declared after their use, method extra initializers before fields, [[Define]] semantics, class expressions withthisin static initializers, derived classes with and without an explicitsuper()call, receiver capture temps in moved initializers, TypeScript parameter properties, and a reparse check that no#namesurvives in the output. Every expected output was produced by running esbuild's lowering of the same source; 13 fail on bun 1.4.0 (the [[Define]] one is a guard for the__publicFieldchoice), all pass with this build.runDecoratorTSwas hoisted to module scope so the parameter-property test can use it.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,regression/issue/27575.test.ts,regression/issue/27526.test.ts,transpiler.test.js,esbuild/ts.test.ts,esbuild/lower.test.ts,esbuild/default.test.ts,esbuild/dce.test.ts: all pass with the debug build.cargo clippy -p bun_js_parserandcargo fmt --checkclean.lower_all_privatekeep their fields in the body.Background
lower_decorators.rs) prints the class mostly as written and appends__decorateElement(...)calls after it. Private members cannot be decorated that way, so a class with private and decorated members has all private members lowered the way esbuild lowers them for old targets: avar _p = new WeakMapper field (aWeakSetper method),__privateAdd(this, _p, init)where the field used to be initialized, and everyobj.#pread / write rewritten to__privateGet/__privateSet(Phase 5 oflower_impl,rewrite_private_accesses_in_*).super()returns in a derived class); static fields and static blocks run in source order while the class is being defined. Anything the lowering moves into the constructor therefore runs after every field that stays in the body, which is why the body cannot keep some fields once others have been moved.__publicField(obj, key, value)(already insrc/runtime.js, also used by js_parser: keep [[Define]] semantics for decorated class fields #35537 and Let a decorated class body observe the class its decorators return #38731) defines an own data property with the same semantics as a class field declaration;obj.key = valuewould instead invoke an inherited setter.