Substitute this and inner class name in relocated static initializers during decorator lowering - #31922
Substitute this and inner class name in relocated static initializers during decorator lowering#31922robobun wants to merge 2 commits into
Conversation
…initializers Standard-decorator lowering relocates static field, accessor, and static block initializers out of the class body. The relocated code kept references to the class expression's inner name binding (ReferenceError at module level) and to this (module this instead of the class). Apply the existing RewriteKind::ReplaceThis / ReplaceRef passes at all three relocation sites: extracted static blocks, decorated static field/accessor initializers, and undecorated static auto-accessor initializers. Fixes #31917
|
Warning Review limit reached
More reviews will be available in 7 minutes and 50 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 (2)
Comment |
|
CI status: the only failing test on both runs (builds 60964 and 60993, every red lane) is
so the subprocess exits 3 on every branch. It fails identically with |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…lker The rewrite_expr/rewrite_stmts walker used for relocated static initializers skipped several AST positions, leaving this and inner class name references unsubstituted there: nested classes, arrow and function parameter defaults (including destructuring patterns), computed object keys, await/yield operands, import() arguments, catch bindings, for-in/of heads, and function/class declarations inside static blocks. Add the missing arms. Nested classes and function declarations follow the existing EFunction split: skipped for ReplaceThis (own this binding, except extends clauses and computed keys, which evaluate in the enclosing scope), recursed for ReplaceRef (ref identity keeps shadowing bindings intact).
There was a problem hiding this comment.
Thanks — 98d210d closes every walker gap I flagged (nested class, arg/binding defaults, computed keys, await/yield/import, SFunction/SClass, for-in/of init, catch binding) and each has a runtime test; I didn't find anything wrong in this revision, but the new rewrite_class/rewrite_binding traversal encodes enough JS scoping subtlety that someone who owns the decorator lowering should give it a final look.
Extended reasoning...
Overview
Two-commit PR against src/js_parser/lower/lower_decorators.rs plus ~230 lines of new tests in test/bundler/transpiler/es-decorators.test.ts. Commit c3d3e1d adds the core fix for #31917: capture a ReplaceRef { inner-name → _class } rewrite when a named class expression is hoisted, and apply ReplaceThis + ReplaceRef at the three points where static initializers are relocated out of the class body (decorated static fields/accessors, undecorated static auto-accessors, extracted static blocks). Commit 98d210d — pushed in response to my earlier review — extends the rewrite_expr/rewrite_stmts walker itself: new rewrite_class, rewrite_args, and rewrite_binding helpers, plus arms for EAwait/EYield/EImport, computed EObject keys, SFunction/SClass, for-in/for-of init, and catch bindings.
Security risks
None. Pure transpiler-output correctness; no I/O, auth, permissions, or untrusted-input parsing surface is touched.
Level of scrutiny
Medium-high. This is code generation in the JS transpiler — bugs here mis-compile user programs. The second commit in particular hand-encodes JS scoping rules (e.g. nested-class extends and computed keys evaluate in the enclosing this, while field initializers/method bodies/static blocks bind their own this; ReplaceRef recurses everywhere because it matches by Ref identity so shadowing is already safe). I traced each new arm against the spec semantics and they look right, and every gap I'd previously listed now has a dedicated end-to-end test that runs the lowered output and checks observable behaviour. But ~100 lines of new recursive AST traversal with per-position ReplaceThis-vs-ReplaceRef splits is past my threshold for auto-approval.
Other factors
- My prior inline comment is fully addressed; nothing outstanding from me.
- Bug-hunting pass on the new revision found nothing.
- CI reds are the unrelated
bunx.test.tsAngular-CLI registry break (and onestreams-leakflake), already triaged in-thread; the decorator suites are green. - No CODEOWNERS entry for
src/js_parser/. - Minor residual gap I noticed but don't think is worth blocking on:
rewrite_classdoesn't visit decorator expressions on the nested class or its members — athis/inner-name reference there would still be missed. Extreme edge case; fine as a follow-up alongside the #31921 items.
|
Re the residual gap note (decorator expressions on nested classes not visited by Checking that did surface a different pre-existing miscompilation: the generated |
1 participant
Fixes #31917
Repro
thisin the same position fails too (TypeError: Cannot read from private field), including in class declarations, because the relocated expression evaluatesthisat module level.Cause
Standard-decorator lowering relocates static initializers out of the class body into the module-level chain that builds the class:
Foo(the class expression's inner name binding) only exists inside the class body, andthisis no longer the class. The lowering already hasRewriteKind::ReplaceRef/ReplaceThismachinery for this (extracted static blocks getReplaceThis), but it was not applied to the other relocated pieces, and the walker itself skipped several AST positions.Fix
In
src/js_parser/lower/lower_decorators.rs:Capture a
ReplaceRef { old: inner name, new: _class }rewrite when swapping to the hoisted temp for class expressions, and apply the rewrites at all three relocation sites: extracted static blocks (ReplaceRef was missing), decorated static field/accessor initializers, and undecorated static auto-accessor initializers (both were missing). Class declarations keep their module-level name binding (and class decorators reassign it before static initializers run), so onlyReplaceThisapplies there.Cover the AST positions the
rewrite_expr/rewrite_stmtswalker skipped (flagged in review): nested classes (newrewrite_class, recursed forReplaceRef; forReplaceThisonly the extends clause and computed keys, which evaluate in the enclosing scope), arrow/function parameter defaults including destructuring patterns (rewrite_args/rewrite_binding), computed object keys,await/yieldoperands,import()arguments, catch bindings, for-in/of heads, and function/class declarations inside static blocks.thisinside nested non-arrow functions and shadowing bindings stay untouched (ref identity plus theEFunction-style kind split; covered by tests).Verification
New tests in
test/bundler/transpiler/es-decorators.test.ts(relocated static initializersblock, 15 tests): both issue repros, declaration + anonymous-expressionthis, decorated and undecoratedstatic accessor, static block inner-name reference, class-decorator replacement visibility, one test per walker position family, plus two negative tests (nested-functionthisand a shadowing function name are not rewritten). 13 of the 15 fail before the fix, all pass after. Expected outputs validated against esbuild 0.21.5 lowering run under Node 24.Existing decorator suites (
es-decorators,es-decorators-esbuild,decorators,decorator-metadata,bundler_decorator_metadata) andtranspiler.test.jsall pass.Related pre-existing problems in the accessor-only lowering path (native private names relocated out of class scope, accessor-vs-static-block evaluation order) are not substitution bugs and are tracked in #31921.