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 7:57 AM PT - Mar 17th, 2026
❌ @robobun, your commit ee771b9 has some failures in 🧪 To try this PR locally: bunx bun-pr 28120That installs a local version of the PR into your bun-28120 --bun |
WalkthroughPhase 5 of decorator lowering now rewrites private-access expressions for newly created properties including their initializers, and extends the rewrite pass to process constructor injection statements, static private-add blocks, suffix expressions, and post-decorating initializer expressions to ensure private names are consistently rewritten. 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.
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.
|
@robobun adopt |
|
✅ All 17 test steps passed across all platforms. The only CI "failure" is |
…s to constructor for correct ordering When lower_all_private is true, non-decorated public instance fields with initializers must be moved into the constructor to preserve evaluation order relative to lowered private fields. Also adds the missing nprop.initializer rewrite in the new_properties Phase 5 loop.
…only Revert the Phase 4 change that moved public instance fields into constructor_inject_stmts — this broke ordering relative to decorated elements initialized in Phase 7. The nprop.initializer rewrite in Phase 5 is still needed for completeness. The public field ordering issue (class field initializer running before constructor body where __privateAdd lives) is a pre-existing problem that requires restructuring the init ordering across Phase 4/7, which is out of scope for this fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/lowerDecorators.zig`:
- Line 1113: The loop over suffix_exprs.items currently only calls
rewritePrivateAccessesInExpr and thus leaves raw `this` in static accessor
initializers; update that loop to also call replace_this(p, expr,
class_name_ref) (or the existing replace_this helper) for each expr
before/alongside rewritePrivateAccessesInExpr so `this` is replaced with the
class_name_ref for static suffix initializers; reference the suffix_exprs.items
iteration, rewritePrivateAccessesInExpr, replace_this, and private_lowered_map
to locate and change the code similarly to how static blocks are handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74b3c2b9-0e55-4d75-ba9d-73be525d788e
📒 Files selected for processing (2)
src/ast/lowerDecorators.zigtest/regression/issue/28118.test.ts
There was a problem hiding this comment.
LGTM — the final diff is a small, mechanical addition of missing private-access rewrite calls in Phase 5, following existing patterns exactly.
Extended reasoning...
Overview
This PR adds 7 lines to src/ast/lowerDecorators.zig in the Phase 5 private-access rewriting block, plus a new regression test file (test/regression/issue/28118.test.ts) with 4 test cases. The Zig changes add missing rewritePrivateAccessesInExpr/rewritePrivateAccessesInStmts calls for: (1) nprop.initializer in the new_properties loop, (2) constructor_inject_stmts, (3) static_private_add_blocks, and (4) suffix_exprs. All follow the identical pattern used by existing rewrite calls immediately above them in the same code block.
Security risks
None. This is a transpiler fix that adds missing AST rewriting passes — no auth, crypto, network, or user-input handling is involved.
Level of scrutiny
Low scrutiny is appropriate. The changes are mechanical: the Phase 5 block 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. The PR simply adds the three data structures that were missed (constructor_inject_stmts, static_private_add_blocks, suffix_exprs) plus the nprop.initializer field that was overlooked in the new_properties loop. The earlier problematic commit (a74e23e, moving public fields to constructor) was cleanly reverted in ee771b9.
Other factors
- The test file covers instance fields, direct references, static fields, and chained references — all exercising the exact bug path.
- CI passes across all platforms per the PR description.
- All 22 existing decorator tests and 147 esbuild decorator tests pass.
- The only bug report for this review is pre-existing: the public-to-private field reference pattern was already broken before this PR (SyntaxError), and remains a known limitation with a different error (TypeError). This PR correctly fixes its stated target (private-to-private references in initializers).
- My previous review comments about Phase 4/Phase 7 ordering (inline comments on the reverted code) are no longer applicable since that code was removed in ee771b9.
|
@alii This PR is a complete fix, not half of a bigger change. Here's a breakdown: Phase 4 (property loop) sorts class properties into various lists. For non-decorated private fields, it places
The Phase 5 rewrites Phase 7 appends additional items to So the data flow is:
No double-rewriting risk either — |
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 several data structures:constructor_inject_stmts,static_private_add_blocks,suffix_exprs, andnprop.initializerin thenew_propertiesloop.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)