fix(transpiler): rewrite private field refs in class field initializers with decorated accessor#28120
fix(transpiler): rewrite private field refs in class field initializers with decorated accessor#28120
Conversation
…rs with decorated accessor When a class has a `@decorated accessor`, all private fields are lowered to WeakMap-based helpers. The private access rewriting (Phase 5) was missing `constructor_inject_stmts`, `static_private_add_blocks`, and `suffix_exprs`, so `this.#field` references inside non-decorated field initializers were left unrewritten, causing a SyntaxError at runtime. Closes #28118 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 2:42 PM PT - Mar 14th, 2026
❌ @autofix-ci[bot], your commit 31a2b10 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28120That installs a local version of the PR into your bun-28120 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe fix extends private-access rewriting in decorator lowering to additional locations: constructor inject statements, static block statements, and suffix expressions. This resolves private field references in class field initializers when decorated accessors are present. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
There was a problem hiding this comment.
LGTM — small, well-tested fix that adds missing private access rewrite calls following the same established patterns.
Extended reasoning...
Overview
This PR adds 6 lines to src/ast/lowerDecorators.zig in Phase 5 ("Rewrite private accesses") to call rewritePrivateAccessesInStmts/rewritePrivateAccessesInExpr on three data structures that were previously missed: constructor_inject_stmts, static_private_add_blocks, and suffix_exprs. A new test file with 4 regression tests is also added.
Security risks
None. This is a transpiler/AST transformation fix with no security-sensitive surface.
Level of scrutiny
Low scrutiny needed. The change is mechanical — it adds calls to existing rewrite functions following the exact same patterns already present for other data structures in the same Phase 5 block. The surrounding code already rewrites new_properties, instance_init_entries, static_init_entries, extracted_static_blocks, static_non_field_elements, instance_non_field_elements, static_field_decorate, instance_field_decorate, pre_eval_stmts, and prefix_stmts. This PR simply adds the three that were missed.
Other factors
The one bug report found is explicitly tagged as pre-existing (the nprop.initializer gap in the new_properties loop at lines 1088-1092), which predates this PR and is not modified by it. The PR includes 4 well-structured regression tests covering instance fields, direct references, static fields, and chained references. The PR description confirms all 22 existing decorator tests and all 147 esbuild decorator tests still pass.
| rewritePrivateAccessesInStmts(p, constructor_inject_stmts.items, &private_lowered_map); | ||
| for (static_private_add_blocks.items) |spab| { | ||
| if (spab.class_static_block) |sb| |
There was a problem hiding this comment.
🟣 Pre-existing bug: The Phase 5 new_properties rewrite loop (line 1088-1092) rewrites nprop.value and nprop.class_static_block but misses nprop.initializer. For class fields, the initial value is stored in initializer (not value, which is null per G.zig:103), so a non-decorated public field like foo = this.#name in a class with lowered privates would have its this.#name reference left unrewritten, causing a SyntaxError at runtime. Fix: add if (nprop.initializer) |*ini| rewritePrivateAccessesInExpr(p, ini, &private_lowered_map); to the loop.
Extended reasoning...
What the bug is
In Phase 5 ("Rewrite private accesses"), the new_properties loop at lines 1088-1092 only rewrites two fields of each property:
for (new_properties.items) |*nprop| {
if (nprop.value) |*v| rewritePrivateAccessesInExpr(p, v, &private_lowered_map);
if (nprop.class_static_block) |sb|
rewritePrivateAccessesInStmts(p, sb.stmts.slice(), &private_lowered_map);
}It checks nprop.value and nprop.class_static_block, but does NOT check nprop.initializer.
Why this matters for class fields
Per G.zig lines 90-104, the Property struct uses initializer for class field initial values (e.g., class Foo { a = 1 }), while value is explicitly documented as "omitted for class fields" (line 103) and will be null. This means for any class field in new_properties, the loop checks nprop.value (which is null) and skips the actual initial value expression entirely.
Concrete triggering scenario
Consider this code:
class MyClass {
@dec accessor label: string = "";
#name = "hello";
foo = this.#name; // public, non-decorated field
}Because @dec accessor label triggers lower_all_private = true, all private fields including #name get lowered to WeakMap-based storage. The private field #name is removed from the class body and replaced with __privateAdd(this, _name, "hello") in the constructor.
Meanwhile, the public field foo is non-decorated, non-private, and non-auto-accessor, so at line 894 it gets appended directly to new_properties via new_properties.append(prop.*). Its initializer is this.#name, and its value is null.
When Phase 5 runs the new_properties loop, nprop.value is null so the rewrite is skipped. nprop.class_static_block is also null. The nprop.initializer containing this.#name is never visited, so this.#name remains in the emitted output even though #name no longer exists as a private field in the class. This produces a SyntaxError: Cannot reference undeclared private names at runtime.
Why this is pre-existing
This PR adds rewrite calls for constructor_inject_stmts, static_private_add_blocks, and suffix_exprs (lines 1107-1112), which is a different fix for a different set of missing rewrites. The new_properties loop at lines 1088-1092 predates this PR and is not modified by it. The missing initializer check is a pre-existing gap in the same Phase 5 block.
Fix
Add if (nprop.initializer) |*ini| rewritePrivateAccessesInExpr(p, ini, &private_lowered_map); inside the new_properties loop, alongside the existing value and class_static_block checks.
There was a problem hiding this comment.
LGTM — small, mechanical fix adding missing rewrite calls that follow the exact same pattern as adjacent code, with good regression tests.
Extended reasoning...
Overview
This PR adds 6 lines to src/ast/lowerDecorators.zig Phase 5 to apply rewritePrivateAccessesInStmts/rewritePrivateAccessesInExpr to three data structures that were previously missed: constructor_inject_stmts, static_private_add_blocks, and suffix_exprs. A new test file with 4 regression tests covers instance fields, direct references, static fields, and chained references.
Security risks
None. The change is purely in the transpiler/AST lowering layer and does not touch authentication, network, crypto, or permissions code.
Level of scrutiny
Low scrutiny is appropriate. The fix is minimal (6 lines), follows the exact same pattern as the surrounding rewrite calls (e.g., the existing loops over instance_init_entries, static_init_entries, extracted_static_blocks, etc.), and the intent is clear from both the code and the linked issue. No design decisions are involved.
Other factors
The only bug report is pre-existing (the new_properties loop missing nprop.initializer), which I already flagged in my previous inline comment and is unrelated to this PR's fix. CI build failures reference scripts/build/ci.ts infrastructure issues, not this change. CodeRabbit also found no actionable issues. The test plan is thorough, verifying the fix against 4 distinct scenarios plus confirming all 22 existing decorator tests and 147 esbuild decorator tests still pass.
| rewritePrivateAccessesInStmts(p, constructor_inject_stmts.items, &private_lowered_map); | ||
| for (static_private_add_blocks.items) |spab| { | ||
| if (spab.class_static_block) |sb| | ||
| rewritePrivateAccessesInStmts(p, sb.stmts.slice(), &private_lowered_map); | ||
| } | ||
| for (suffix_exprs.items) |*expr| rewritePrivateAccessesInExpr(p, expr, &private_lowered_map); |
There was a problem hiding this comment.
🟣 Pre-existing: rewritePrivateAccessesInExpr has several AST coverage gaps that produce runtime errors when private fields are lowered. The most impactful: compound assignments (this.#count += 1) and unary inc/dec (this.#count++) only match bin_assign, so they fall through to the generic handler which transforms the LHS to __privateGet(this, _count) += 1 — a ReferenceError. Optional chaining (obj?.#field) discards the optional_chain field, throwing TypeError on null instead of returning undefined. Additional gaps: function/arrow parameter defaults, computed object literal keys, nested class extends clauses, and tagged template literal tags with private methods are all not visited or lose semantics. None of these are introduced by this PR — the rewriter function predates it and is unchanged.
Extended reasoning...
What the bugs are
The rewritePrivateAccessesInExpr function (starting at line 359) is responsible for rewriting all private field references (this.#field) to runtime helper calls (__privateGet/__privateSet) when lower_all_private = true. It has multiple gaps in AST node coverage that have existed since the function was written, all predating this PR.
Most impactful: Compound assignments and unary inc/dec
The e_binary handler at line 371 only checks for e.op == .bin_assign (plain =). Compound assignment operators like +=, -=, *=, ??=, etc. have different opcodes (e.g., bin_add_assign) and do not match this check. They fall through to lines 391-392, which recursively process both e.left and e.right. The recursive call on e.left hits the e_index handler (line 360-368) which transforms this.#count into __privateGet(this, _count). The result is __privateGet(this, _count) += 1 — a ReferenceError because you cannot assign to a function call result.
Similarly, unary ++/-- at line 422 just recurses on e.value, which gets transformed to __privateGet(this, _count)++ — also a ReferenceError.
Step-by-step proof for this.#count += 1:
- Class has
@dec accessor labeltriggeringlower_all_private = true #countis lowered to WeakMap_count- Method body
this.#count += 1entersrewritePrivateAccessesInExpr e_binaryhandler:e.opisbin_add_assign, notbin_assign→ first check failse.opis notbin_in→ second check fails- Falls through to generic:
rewritePrivateAccessesInExpr(p, &e.left, map)at line 391 e.leftisE.Index{target: E.This, index: E.PrivateIdentifier}→ matchese_indexhandlere_indexhandler replacese.leftwith__privateGet(this, _count)- Output:
__privateGet(this, _count) += 1→ ReferenceError at runtime
Correct lowering should be: __privateSet(this, _count, __privateGet(this, _count) + 1)
Optional chaining semantics lost
The e_index handler at line 364 replaces the entire expression with privateGetExpr(p, e.target, info, expr.loc), completely discarding the optional_chain field on E.Index. So this.other?.#name becomes __privateGet(this.other, _name) instead of this.other == null ? void 0 : __privateGet(this.other, _name). When this.other is null, this throws TypeError (WeakMap cannot accept null as key) instead of returning undefined.
Additional gaps
- Function/arrow parameter defaults (lines 451-452): Only process body stmts, not
args[].default. A method likemethod(a = this.#x)leaves the default unrewritten → SyntaxError. - Computed object keys (lines 441-445): Process
prop.valueandprop.initializerbut notprop.keywhenis_computedis true.{ [this.#key]: 1 }leaves the key unrewritten → SyntaxError. - Nested class extends (line 453 /
else => {}):e_classis not handled, soclass extends this.#Base {}leaves the extends clause unrewritten → SyntaxError. - Tagged template this-binding (lines 447-449): Unlike
e_callwhich rewrites to.call(this, ...),e_templatejust recursively rewrites the tag.this.#method\hello`becomes__privateMethod(this, _storage, _fn)`hello`which loses thethis` binding.
Why these are pre-existing
The rewritePrivateAccessesInExpr function and all its handlers predate this PR entirely. This PR adds new call sites for the function (lines 1107-1112 for constructor_inject_stmts, static_private_add_blocks, and suffix_exprs) but does not modify the function itself. The new call sites process machine-generated expressions (runtime helper calls like __privateAdd) that would not contain compound assignments, optional chaining, etc. The bugs are primarily reachable through the pre-existing new_properties loop (line 1088) which processes user-written method bodies.
Fix
The compound assignment fix requires adding cases for all compound assignment operators to produce __privateSet(this, _count, __privateGet(this, _count) OP rhs). Unary inc/dec needs a temp variable pattern. Optional chaining needs a null guard wrapper. The other gaps need additional recursion into the missed AST nodes (computed keys, function args, class extends).
Summary
@decorated accessor, Bun lowers all private#fields to WeakMap-based helpers (__privateAdd/__privateGet). Phase 5 of the decorator lowering performs private access rewriting but was missing three data structures:constructor_inject_stmts,static_private_add_blocks, andsuffix_exprs.this.#fieldreferences inside non-decorated field initializers unrewritten, causingSyntaxError: Cannot reference undeclared private namesat runtime.Closes #28118
Test plan
test/regression/issue/28118.test.tswith 4 test cases covering instance fields, direct references, static fields, and chained referencesUSE_SYSTEM_BUN=1(confirming the bug) and pass withbun bd test(confirming the fix)🤖 Generated with Claude Code