Skip to content

fix(transpiler): rewrite private field refs in class field initializers with decorated accessor#28120

Open
robobun wants to merge 2 commits intomainfrom
claude/fix-28118-private-field-decorator-accessor
Open

fix(transpiler): rewrite private field refs in class field initializers with decorated accessor#28120
robobun wants to merge 2 commits intomainfrom
claude/fix-28118-private-field-decorator-accessor

Conversation

@robobun
Copy link
Collaborator

@robobun robobun commented Mar 14, 2026

Summary

  • When a class has a @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, and suffix_exprs.
  • This left this.#field references inside non-decorated field initializers unrewritten, causing SyntaxError: Cannot reference undeclared private names at runtime.
  • Adds the missing rewrite calls for all three data structures.

Closes #28118

Test plan

  • Added regression test test/regression/issue/28118.test.ts with 4 test cases covering instance fields, direct references, static fields, and chained references
  • Tests fail with USE_SYSTEM_BUN=1 (confirming the bug) and pass with bun bd test (confirming the fix)
  • All 22 existing decorator tests pass
  • All 147 esbuild decorator tests pass

🤖 Generated with Claude Code

…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>
@robobun
Copy link
Collaborator Author

robobun commented Mar 14, 2026

Updated 2:42 PM PT - Mar 14th, 2026

@autofix-ci[bot], your commit 31a2b10 has 2 failures in Build #39659 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 28120

That installs a local version of the PR into your bun-28120 executable, so you can run:

bun-28120 --bun

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4cecc19d-5b52-4b60-ac40-22c235e3bacf

📥 Commits

Reviewing files that changed from the base of the PR and between 10bdb48 and 31a2b10.

📒 Files selected for processing (2)
  • src/ast/lowerDecorators.zig
  • test/regression/issue/28118.test.ts

Walkthrough

The 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

Cohort / File(s) Summary
Decorator lowering implementation
src/ast/lowerDecorators.zig
Extended Phase 5 to apply private-access rewriting to constructor inject statements, static block statements, and suffix expressions for consistent private field handling.
Regression test suite
test/regression/issue/28118.test.ts
Added comprehensive test coverage for issue 28118, validating private field initialization and references in various contexts with decorated accessors.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically describes the main change: fixing private field reference rewriting in class field initializers when decorated accessors are present.
Description check ✅ Passed The PR description covers both required template sections with clear explanation of the problem, fix, testing approach, and issue closure.
Linked Issues check ✅ Passed The code changes directly address all requirements from issue #28118 by adding private access rewriting to three previously-missed data structures (constructor_inject_stmts, static_private_add_blocks, suffix_exprs), fixing the root cause of private field SyntaxErrors in field initializers.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the private field rewriting issue: modifications to lowerDecorators.zig implement the fix, and the test file adds regression coverage for issue #28118.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1107 to +1109
rewritePrivateAccessesInStmts(p, constructor_inject_stmts.items, &private_lowered_map);
for (static_private_add_blocks.items) |spab| {
if (spab.class_static_block) |sb|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1107 to +1112
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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:

  1. Class has @dec accessor label triggering lower_all_private = true
  2. #count is lowered to WeakMap _count
  3. Method body this.#count += 1 enters rewritePrivateAccessesInExpr
  4. e_binary handler: e.op is bin_add_assign, not bin_assign → first check fails
  5. e.op is not bin_in → second check fails
  6. Falls through to generic: rewritePrivateAccessesInExpr(p, &e.left, map) at line 391
  7. e.left is E.Index{target: E.This, index: E.PrivateIdentifier} → matches e_index handler
  8. e_index handler replaces e.left with __privateGet(this, _count)
  9. 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 like method(a = this.#x) leaves the default unrewritten → SyntaxError.
  • Computed object keys (lines 441-445): Process prop.value and prop.initializer but not prop.key when is_computed is true. { [this.#key]: 1 } leaves the key unrewritten → SyntaxError.
  • Nested class extends (line 453 / else => {}): e_class is not handled, so class extends this.#Base {} leaves the extends clause unrewritten → SyntaxError.
  • Tagged template this-binding (lines 447-449): Unlike e_call which rewrites to .call(this, ...), e_template just 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

this.#field not rewritten in class field initializers when class has @decorated accessor

1 participant