fix(codegen): class-expression statics — run static blocks per evaluation, bind static this to the fresh object - #6269
Conversation
…tion, bind static `this` to the fresh object Two halves of the same #685 / #1787 gap: a class EXPRESSION evaluated inside a function (`return class { … }` factories — effect's `make()`, webpack/turbopack factory modules) lowers to a fresh heap class object per evaluation (`ClassExprFresh`), but its statics were second-class: 1. `static { … }` blocks NEVER ran on the fresh path. The `ClassExprFresh` codegen arm emitted named static fields and `__perry_ctor_caps`, but not the `__perry_static_init_*` invocations (only the shared-template path emitted those), and the module-init fallback can't run them correctly anyway (wrong time, wrong `this`, wrong captures). Fix, in the `ClassExprFresh` arm: - register this evaluation's captured values as the template's CLASS_CAPTURE_VALUES snapshot (`js_class_register_capture_values`) right after building `__perry_ctor_caps` — the compiled static-method bodies resolve enclosing-scope reads through the name-keyed snapshot (`js_param_or_class_capture_value`), not the ctor-caps array; - after the symbol statics, invoke each of the template's `__perry_static_init_*` methods, arming the one-shot static-`this` override (`js_static_this_arm_value(obj)`) before each call so the body's `js_static_this_resolve` prologue binds `this` to THIS fresh object — block writes land as its own properties. Blocks run after the named fields; the source interleaving of fields and blocks is not reproduced on this path (pre-existing limitation). 2. A static METHOD called on the fresh object bound `this` to the shared TEMPLATE. The compile-time static-dispatch tower (devirtualized `make(a).m()` / `const C = make(a); C.m()` via `func_returns_class`) armed the receiver only for plain `ClassRef` receivers; dynamic-value receiver shapes (`Call` / `LocalGet` / `ClassExprFresh`) only set implicit `this` — which the static-method prologue NEVER reads (it consumes the armed override or falls back to the lexical class-ref). So `this.<field>` read the template's static-field global — which the fresh path never writes — and returned `undefined`. Fix: always arm the actual receiver box. For a local holding a plain ClassRef value this arms exactly the prologue's default (no behavior change); for fresh objects it restores the receiver — matching the runtime dispatch tower (`js_class_static_method_call`), which has armed its receiver since the static-private-brand work. Fixes the two long-standing gap-test failures `test_gap_class_expr_static_this` and `test_gap_class_expr_extends_static_call_this` (previously DIFF: every `viaThis()` returned undefined). Class gap regression: 13/13 pass (was 11/13). New gap test `test_gap_class_expr_fresh_static_blocks_this.ts` (blocks per evaluation + captures, `this` identity, const-bound and inline receivers, nested-declaration and top-level-expression controls) — byte-identical to node.
📝 WalkthroughWalkthroughFresh class expressions now register captured outer values, execute template static initializer blocks for each evaluation, and bind static ChangesFresh class static initialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClassExprFresh
participant StaticThisRuntime
participant StaticInitializer
participant FreshClass
ClassExprFresh->>FreshClass: allocate class object
ClassExprFresh->>StaticThisRuntime: arm fresh class as static this
StaticThisRuntime->>StaticInitializer: bind this
ClassExprFresh->>StaticInitializer: invoke static initializer
StaticInitializer->>FreshClass: update static fields
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/expr/static_field_meta.rs (1)
441-473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrdering is correct; capture-snapshot emission duplicates
RegisterClassCaptures.The snapshot registration runs before the
static { … }blocks below (write-right-before-use), and the non-empty/template_cid != 0guards match the upstreamjs_class_register_capture_valuescontract — good.Lines 451-472 are near-identical to the buffer-build + register logic in the
RegisterClassCapturesarm (Lines 155-177). Consider extracting a small helper to keep the two capture-snapshot sites from drifting.♻️ Suggested helper to share the snapshot emission
// free fn in this module fn emit_capture_snapshot(ctx: &mut FnCtx<'_>, cid_str: &str, values: &[String]) { let n = values.len(); if n == 0 { return; } let buf = ctx.func.alloca_entry_array(DOUBLE, n); for (i, v) in values.iter().enumerate() { let slot = ctx .block() .gep(DOUBLE, &buf, &[(crate::types::I64, &i.to_string())]); ctx.block().store(DOUBLE, v, &slot); } let ptr_reg = ctx.block().next_reg(); ctx.block() .emit_raw(format!("{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", ptr_reg, n, buf)); ctx.block().call_void( "js_class_register_capture_values", &[ (crate::types::I32, cid_str), (crate::types::PTR, &ptr_reg), (crate::types::I64, &n.to_string()), ], ); }Then both arms become
emit_capture_snapshot(ctx, &cid_str, &lowered_caps);(guarded ontemplate_cid != 0).🤖 Prompt for 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. In `@crates/perry-codegen/src/expr/static_field_meta.rs` around lines 441 - 473, Extract the duplicated capture-snapshot buffer construction and registration logic from the RegisterClassCaptures arm and the template static-block path into a shared emit_capture_snapshot helper in this module. Have it accept the context, class ID string, and lowered capture values, return early for empty values, and preserve the existing allocation, GEP, store, and js_class_register_capture_values behavior; replace both inline implementations with the helper while retaining the template_cid != 0 guard where applicable.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/src/expr/static_field_meta.rs`:
- Around line 441-473: Extract the duplicated capture-snapshot buffer
construction and registration logic from the RegisterClassCaptures arm and the
template static-block path into a shared emit_capture_snapshot helper in this
module. Have it accept the context, class ID string, and lowered capture values,
return early for empty values, and preserve the existing allocation, GEP, store,
and js_class_register_capture_values behavior; replace both inline
implementations with the helper while retaining the template_cid != 0 guard
where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3708a287-d13c-4432-a9c8-7506201194ff
📒 Files selected for processing (3)
crates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/lower_call/property_get/static_dispatch.rstest-files/test_gap_class_expr_fresh_static_blocks_this.ts
Summary
Two halves of the same #685 / #1787 gap: a class expression evaluated inside a function (
return class { … }factories — effect'smake(), webpack/turbopack factory modules) lowers to a fresh heap class object per evaluation (ClassExprFresh), but its statics were second-class:1.
static { … }blocks never ran on the fresh pathThe
ClassExprFreshcodegen arm emitted named static fields and__perry_ctor_caps, but not the__perry_static_init_*invocations (only the shared-template path emitted those) — and the module-init fallback can't run them correctly anyway (wrong time, wrongthis, wrong captures). Soreturn class { static { this.viaBlock = tag } }factories produced objects whose blocks simply never ran.Fix (in the
ClassExprFresharm,expr/static_field_meta.rs):CLASS_CAPTURE_VALUESsnapshot (js_class_register_capture_values) right after building__perry_ctor_caps— compiled static-method bodies resolve enclosing-scope reads through the name-keyed snapshot (js_param_or_class_capture_value), not the ctor-caps array;__perry_static_init_*methods, arming the one-shot static-thisoverride (js_static_this_arm_value(obj)) before each call, so the body'sjs_static_this_resolveprologue bindsthisto this fresh object — block writes land as its own properties.2. Static methods called on the fresh object bound
thisto the shared templateThe compile-time static-dispatch tower (devirtualized
make(a).m()/const C = make(a); C.m()viafunc_returns_class) armed the receiver only for plainClassRefreceivers; dynamic-value receiver shapes (Call/LocalGet/ClassExprFresh) only set implicitthis— which the static-method prologue never reads (it consumes the armed override or falls back to the lexical class-ref). Sothis.<field>read the template's static-field global — which the fresh path never writes — and returnedundefined.Fix (
lower_call/property_get/static_dispatch.rs): always arm the actual receiver box. For a local holding a plainClassRefvalue this arms exactly the prologue's default (no behavior change); for fresh objects it restores the receiver — matching the runtime dispatch tower (js_class_static_method_call), which has armed its receiver since the static-private-brand work.Validation
test_gap_class_expr_static_thisandtest_gap_class_expr_extends_static_call_this(previously everyviaThis()returnedundefined).test_gap_class_expr_fresh_static_blocks_this.ts— blocks per evaluation with per-evaluation captures,this === receiveridentity, const-bound + inline receivers, nested-declaration and top-level-expression controls — byte-identical to node.cargo fmtclean.Code-only; no version/changelog bump (maintainer folds metadata at merge).
Summary by CodeRabbit
New Features
thisto the newly created class object.Bug Fixes