Repository navigation
Conversation
…r lowering The standard decorator/auto-accessor lowering moves every class static block out of the class body so it runs after decoration, but it only lowered private members when a decorated property was present. With only an accessor field or only a class decorator, a static block containing `#a in x` or `this.#a()` was emitted after the class with the private name intact, producing output that does not parse. The pre-scan now also forces private lowering when a static block references one of the class's own private names, and private auto-accessors record their storage WeakMap in the lowering map so brand checks rewrite to __privateIn.
|
Warning Review limit reached
More reviews will be available in 22 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR enhances ES decorator lowering: it pre-scans extracted static blocks and auto-accessor initializers for private-name usage, forces full private-member lowering when found, records extracted-only accessor WeakMaps, and expands private-access rewriting to moved code paths and property initializers. ChangesPrivate names in static blocks and field initializers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 8:27 PM PT - May 25th, 2026
✅ @robobun, your commit bf086da16357a1b4824b483e7504000d18e7bcd3 passed in 🧪 To try this PR locally: bunx bun-pr 31405That installs a local version of the PR into your bun-31405 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Initializers of lowered private fields/accessors are emitted inside __privateAdd calls in the constructor, in static __privateAdd blocks, or after the class, and kept public field initializers stay in the class while the privates they reference are removed from it. Run the private-access rewrite over those containers too so references like `() => this.#name` in a private field initializer keep working. Fixes #28118
…function declarations Recording the auto-accessor storage in the lowering map whenever all privates were lowered would also rewrite previously-valid references like `this.#x++` into `__privateGet(...)++`, which does not parse. Only record it when lowering was forced because a static block references the class's private names — the case whose output was invalid before. Also recurse into function declarations in both the private-reference scan and the rewrite pass so a function declared inside a static block is handled like a function expression.
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:2080-2082— Thenprop.initializerrewrite makes the second reparse-test input (pub = this.#name) parse, but it still doesn't run:pubstays a native class field (line 1765) while__privateAdd(this, _name, 2)goes into the constructor body (line 1621), and native field initializers execute before the constructor body, so__privateGet(this, _name)throws at instantiation. Not a regression (was a SyntaxError before), but the reparse-only test labelled "kept public field initializer referencing a lowered private" gives false confidence — a full fix would need to move such kept public instance fields intoconstructor_inject_stmtsin source order alongside the__privateAddcalls.Extended reasoning...
What the bug is
The new lines 2080-2082 rewrite
nprop.initializerfor properties that remain innew_properties. For a kept public instance field likepub = this.#name(where#nameis being lowered), this producespub = __privateGet(this, _name). However,pubis still emitted as a native class field (it reachesnew_properties.push(prop_full_copy(prop))at line 1765 andnew_propertiesbecomesclass.propertiesat the end oflower_impl), while the lowered#namebecomes__privateAdd(this, _name, 2)pushed intoconstructor_inject_stmtsat line 1621 and later spliced into the constructor body (lines ~2434-2516).Per the ES spec, for a base class
[[Construct]]runsInitializeInstanceElements(all native instance field initializers) beforeOrdinaryCallEvaluateBody(the constructor body). For a derived class, native field initializers run immediately aftersuper()returns — still before the spliced statements atsuper_index + 1. Sopub's initializer evaluates__privateGet(this, _name)before__privateAdd(this, _name, 2)has registeredthisin the WeakMap →TypeError: Cannot read private member from an object whose class did not declare it.Step-by-step proof
Input:
function id(v, c) { return v; } class Foo { @id accessor a = 1; #name = 2; pub = this.#name; } new Foo();
- Pre-scan (line 1489):
@id accessor a→has_any_decorated = true;#name→has_any_private = true; solower_all_private = true. - Property loop,
#name = 2: undecorated private field, non-accessor → enters the branch at lines 1606-1631.__privateAdd(this, _name, 2)is pushed toconstructor_inject_stmts(line 1621).#nameis removed from the class. - Property loop,
pub = this.#name: undecorated, public, non-accessor, not a static block → falls through to line 1765, pushed tonew_propertiesunchanged. It remains a native instance field on the emitted class. - Phase 5 (lines 2080-2082, new in this PR):
pub's initializer is rewritten to__privateGet(this, _name). - Constructor injection (lines ~2434-2516): a constructor is synthesized (or the existing one is patched) with
__privateAdd(this, _name, 2)in its body. The keptpubfield is not touched here. - Emitted class (simplified):
class Foo { pub = __privateGet(this, _name); // native field — runs FIRST constructor() { __privateAdd(this, _name, 2); // body — runs SECOND ... } get a() {...} set a(v) {...} }
new Foo():InitializeInstanceElementsevaluatespub's initializer →__privateGet(this, _name)→_nameWeakMap doesn't havethisyet → TypeError.
Why existing code doesn't prevent it
Nothing in
lower_implmoves undecorated public instance fields out ofnew_properties. The only mechanism that controls instance-field ordering relative to lowered privates isconstructor_inject_stmts, and public fields never go there. The Phase-5 rewrite at 2080-2082 only changes the expression, not where the field is evaluated.Why this is not a regression
Before this PR, lines 2080-2082 didn't exist and only
nprop.valuewas rewritten. Sopub = this.#namekept a raw#namereference while#namehad been removed from the class → the printed output failed to parse withSyntaxError: Cannot reference undeclared private names: "#name". After this PR the output parses butnew Foo()throws at runtime. That's strictly less broken (the module loads; the error is per-instantiation rather than per-module), so this PR is a net improvement for this input.Why it's worth flagging anyway
The PR adds a reparse-only test entry for exactly this input — second
inputs[]element in the new "Bun.Transpiler output with private references in field initializers reparses" test, with the comment// kept public field initializer referencing a lowered private. That test passes (the output now reparses), which could lead a reader to believe the case is fully handled. The two new runtime tests in the same describe block cover private→private (where both sides land inconstructor_inject_stmts, so ordering is preserved) and static cases, but not public-instance-field→private. A runtime test for the secondinputs[]entry would fail.How to fix
When
lower_all_privateis true, undecorated public instance fields whose initializers reference a lowered private (or, more simply, all undecorated public instance fields when any private is lowered) should also be moved intoconstructor_inject_stmtsasthis.pub = <init>assignments, interleaved in source order with the__privateAddcalls so that define-order semantics are preserved. Alternatively, drop the secondinputs[]entry / its comment so the test file doesn't imply this shape is fixed, and track it as a follow-up. - Pre-scan (line 1489):
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5755c54b-8dfc-4009-a55e-6c082e6fdedb
📒 Files selected for processing (2)
src/js_parser/lower/lower_decorators.rstest/bundler/transpiler/es-decorators.test.ts
Undecorated static auto-accessor initializers are emitted after the class as __privateAdd(Class, storage, init), so a private reference inside one needs private lowering just like a static block. Rename the gating flag accordingly and clarify the reparse test for kept public field initializers.
|
Re the additional finding about |
|
CI status for f13cd0e (build 58038): the three red lanes are unrelated to this diff —
All lanes that ran the decorator suites are green, and |
… code Private auto-accessors keep their getter/setter declaration on the class, so their storage entries now live in a separate map that is applied only to code emitted outside the class body (extracted static blocks, suffix expressions, pre-eval/prefix statements). Retained class code keeps native access, so update and compound assignment on the accessor keep working, and the scan now also runs when lowering was already forced by a decorated member so those classes get the same treatment.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Come up with a faster approach. Do not add extra passes. And, delete all your code comments.
The map is only ever applied to code emitted outside the class body, so recording every undecorated private auto-accessor is safe and covers all the containers that can carry such references (extracted static blocks, lowered private method bodies in prefix statements, decorator element arguments, suffix expressions) without scanning each of them. This also removes the dependency on has_any_private, which was not set when the first private member in source order was decorated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/lower/lower_decorators.rs`:
- Around line 2163-2195: The extracted_accessor_map rewrite is missing for
constructor_inject_stmts and static_private_add_blocks; update the same
post-extraction block that rewrites other moved containers to also rewrite
these: call p.rewrite_private_accesses_in_stmts(&mut constructor_inject_stmts,
&extracted_accessor_map) for constructor_inject_stmts, and for
static_private_add_blocks iterate mutably (or call the appropriate API) to
rewrite initializers/stmt slices inside each block (using
p.rewrite_private_accesses_in_expr / p.rewrite_private_accesses_in_stmts as
needed) so any references to auto-accessors are transformed just like the other
containers (keep using the existing p.rewrite_* helpers and the
extracted_accessor_map).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a67c63f-53ad-46c6-a7d0-a5b40ae5f902
📒 Files selected for processing (2)
src/js_parser/lower/lower_decorators.rstest/bundler/transpiler/es-decorators.test.ts
Record private identifiers resolved inside class static blocks and static auto-accessor initializers while they are being visited, and drop the AST-walking pre-scan from the decorator lowering. Accessor storage entries are merged into the single rewrite map between the retained-code and moved-code rewrites, so each container is processed once.
|
Reworked in 7497ef6 per the review:
|
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:208-211— 🟣 Pre-existing, in scope: theEFunction/EArrowarms here (and the newSFunctionarm at line 288, plus the matching rewrite arms at lines 997-1004 / 1129) only recurse intobody.stmtsand never walkargs[i].default. So a private reference that appears in a default parameter value of a function/arrow inside an extracted static block — e.g.class Foo { accessor a = 1; #m() {} static { const check = (o = #m in new Foo()) => o; check(); } }— is neither detected by the pre-scan (solower_all_privateis not forced) nor rewritten (so even when lowering is forced, the raw#msurvives in the moved code), and the output keeps#moutside the class → unparseable. Same shape as theSFunction-body gap fixed in 29d1b42, just one level deeper; fix is to also iterateargsand recurse into eacharg.defaultin both functions. Fine as a follow-up.Extended reasoning...
What the bug is
Both the new scan (
expr_references_private_name/stmts_reference_private_name) and the rewrite (rewrite_private_accesses_in_expr/_stmts) handleEFunction,EArrow, andSFunctionby recursing intobody.stmtsonly.G::Arg(src/ast/g.rs:340) carries a separatedefault: Option<ExprNodeIndex>field, and neither traversal iteratesfunc.args/e.argsto visit it. So a private-name reference that appears in a default parameter value —(o = #m in new Foo()) => o,function f(o = this.#m) {}— is invisible to both passes.The PR's doc comment on the scan says it "mirrors
rewrite_private_accesses_in_expr/_stmts", and indeed it inherits this gap from the rewrite, which already had it onmain. The same applies to defaults inside destructuring binding patterns inSLocal(const { v = #m in x } = obj— onlydecl.valueis walked, not the binding pattern's default values).Step-by-step proof — scan miss
class Foo { accessor a = 1; #m() {} static { const check = (o = #m in new Foo()) => o; console.log(check()); } }
accessor atriggerslower_impl.#m()→has_any_private = true; no decorators →has_any_decorated = false. Line 1504 leaveslower_all_private = false.- The static-block pre-scan (line 1513) walks the block:
SLocal→decl.value→EArrow. TheEArrowarm at line 211 callsstmts_reference_private_nameone.body.stmtsonly — the body is the single expression-statemento, which has no private.e.args[0].default(#m in new Foo()) is never visited. TheSExprforconsole.log(check())also has no private. Scan returnsfalse→lower_all_privatestaysfalse. #m()is therefore kept as a native private method on the class.- The static block is extracted out of the class body. Phase 5 is skipped (
private_lowered_mapis empty) and theextracted_accessor_maprewrite doesn't know about#m(it's a method, not an auto-accessor). - Output contains
(o = #m in new Foo()) => ooutside the class →SyntaxError: Unexpected #m.
Step-by-step proof — rewrite miss
class Foo { @dec m() {} #helper() {} static { const f = (o = #helper in new Foo()) => o; f(); } }
@dec m()+#helper()→ line 1504 setslower_all_private = true.#helperis fully lowered intoprivate_lowered_mapand removed from the class.- Phase 5 walks
extracted_static_blocks→SLocal→decl.value→EArrow. TheEArrowarm at line 1001 callsrewrite_private_accesses_in_stmtsone.body.stmtsonly — the bodyohas nothing to rewrite.e.args[0].default(#helper in new Foo()) is never visited. - The extracted block is emitted after the class with the raw
#helperstill in the default parameter → unparseable.
Why existing code doesn't prevent it
The
EFunctionarm (lines 208-210 scan / 997-1000 rewrite),EArrowarm (line 211 / 1001-1004), andSFunctionarm (line 288 / 1129-1132) all descend intobody.stmtsand nothing else.G::Arg.defaultis a separate field not reachable from the body. Default-parameter values are part of the function's lexical scope (and so can legally reference the enclosing class's private names), but they live on the parameter list, not in the body statement array.Why this is pre-existing
The rewrite arms at lines 997-1004 are unchanged context lines; on
main(which has no scan and the same rewrite gap) both repros above already produce unparseable output. So this is not a regression introduced by this PR. It is, however, exactly the bug class this PR closes (private names escaping the class via extracted static blocks), and commit 29d1b42 in this PR just added the analogousSFunctionarm to both functions for the function-declaration gap raised earlier in review — the parameter-default gap is the same shape one level deeper.Impact
Narrow trigger: requires decorator/auto-accessor lowering to fire, plus a function/arrow inside a moved container whose only private-name reference is in a default parameter value (or a destructuring-binding default). Unusual, but it's the same fuzzer-discoverable round-trip-invariant violation the PR is fixing elsewhere.
How to fix
Have the
EFunction/EArrow/SFunctionarms in bothexpr_references_private_name/stmts_reference_private_nameandrewrite_private_accesses_in_expr/_stmtsalso iterateargsand recurse into eacharg.default, e.g.:js_ast::ExprData::EArrow(e) => { e.args.slice().iter().any(|a| a.default.is_some_and(|d| expr_references_private_name(&d, names))) || stmts_reference_private_name(e.body.stmts.slice(), names) }
(and the analogous mutable iteration in the rewrite). The same treatment applies to destructuring-binding defaults in
SLocalif you want to be exhaustive. Fine as a follow-up.
|
CI status for 7497ef6 (build 58074): 278 jobs passed (including the Windows and macOS lanes that were red on earlier runs); the single failure is |
Reset the depth counter when entering a nested class so ordinary members of a class declared inside another class's static block are not treated as static-init code, and keep only the recorded references that resolve to the visited class's own private names so captures of an enclosing class's privates do not force lowering on that class.
|
Closing in favor of #40833, which rewrites the standard decorator lowering to follow esbuild's |
What does this PR do?
Fixes a transpiler bug (found by fuzzing the round-trip invariant) where the standard-decorator / auto-accessor lowering emits a private name outside the class body, so the printed output does not parse — plus the related case where private references inside moved field initializers are not rewritten.
Fixes #28118
Repro (fuzzer input)
Before, this printed:
#a in _is only legal inside the class that declares#a, so the output fails to reparse (Unexpected #a). The same happens at runtime for files like:for class-decorator-only classes (
@dec class Foo { static { this.#a() } #a() {} }), and for the case in #28118:Cause
lower_implinsrc/js_parser/lower/lower_decorators.rsmoves code out of its original position in the class (static blocks always run after decoration; lowered private field initializers move into__privateAddcalls in the constructor, into static__privateAddblocks, or after the class), but:accessorfield or only a class decorator keeps#anative while its static blocks are still moved out with raw#areferences, and__privateAddblocks, suffix expressions (static auto-accessor initializers), and the initializers of kept public fields.Fix
__privateIn/__privateGet/__privateMethodcalls (same output shape already produced when a decorated property is present). Classes whose static blocks/initializers don't touch private names are unaffected, and no extra AST passes are added.#x in objthere rewrite correctly while code retained in the class keeps native access (update/compound assignment on the accessor keeps working). Each container is rewritten once.__privateAddblocks, suffix expressions, and kept field initializers, which fixesthis.#fieldnot rewritten in class field initializers when class has@decorated accessor#28118 (#callback = () => this.#name) and the analogous static-field/static-accessor initializer cases.Known remaining gaps (pre-existing, called out by review):
this.#field += 1inside a static block of a decorated class) still prints as__privateGet(...) += 1; handling that needs__privateSet-based rewrites.pub = this.#name) now prints as valid syntax but still throws at instantiation, because native field initializers run before the constructor body where__privateAddlands; fully fixing that requires moving kept public instance fields into the constructor in source order.How did you verify your code works?
Added tests in
test/bundler/transpiler/es-decorators.test.ts:Bun.Transpilerreparse check using the exact fuzzer inputthis.#fieldnot rewritten in class field initializers when class has@decorated accessor#28118 repro (TS, standard decorators), static private field + static auto-accessor initializers referencing a private, and a reparse check for the moved-initializer shapesMost of the new tests fail without the src change and all pass with it. Also ran
es-decorators-esbuild.test.ts(147 pass),decorators.test.ts,decorator-metadata.test.ts,transpiler.test.js, andbundler/esbuild/lower.test.tswith the debug build — no regressions.