fix(runtime): per-evaluation inherited static wins over last-wins registry props (#6552) - #6556
Conversation
…istry props (PerryTS#6552) A subclass of a class-EXPRESSION value evaluated more than once (`function make(a){return class{static ast=a}}`, then `class Number$ extends make(x) {}` / `class Widget$ extends make(y) {}`) records THIS evaluation's parent object as its static prototype (`class_prototype_object`, PerryTS#1788). But PerryTS#6443's static-DATA-field inheritance walk reads the parent's `CLASS_DYNAMIC_PROPS`, which are keyed by the class-expression TEMPLATE id — shared, last-wins across every evaluation — and it ran BEFORE the per-evaluation prototype-object walk. So every renamed effect-Schema class read via `import * as M` (`M.Number.ast`/`M.Widget.ast`) collapsed to the LAST `make(...)`'s static `ast` (the direct export's `DirectKeyword`), re-breaking effect Schema decode (test_gap_renamed_class_export_namespace, the PerryTS#1758/PerryTS#1812 guard). Interleave the two walks: at each level of the class-object proto chain, consult the class's pinned per-evaluation parent object BEFORE the parent's registry props. The pinned object is authoritative (it carries this evaluation's own edge); the registry read stays the fallback for a plain declaration parent (PerryTS#6443: Auth.js `SignInError.kind`), which has no pinned object. Depth order is preserved, so a closer registry static still shadows a deeper per-evaluation one. test_gap_renamed_class_export_namespace is byte-identical to node again across renamed (global-colliding + not) and direct exports; PerryTS#6443 static-field inheritance + deleted-key fallthrough, PerryTS#6438 per-eval statics, effect factory static dispatch, and the rest of the class-expr/static suite (18/18) stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStatic inherited data-property resolution now checks each ancestor’s per-evaluation pinned prototype before falling back to ChangesStatic inheritance resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs`:
- Around line 1061-1065: Update the inherited-field lookup in
js_object_get_field_by_name so a property found on proto is returned when its
value is null; only undefined should continue the fallback to shared registry
data or higher ancestors. Preserve the existing prototype traversal and return
behavior for defined values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d95f10c-c785-4ec4-9fc7-12b4769dc969
📒 Files selected for processing (1)
crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs
…rough (PerryTS#6552 review) CodeRabbit review follow-up. The interleaved per-evaluation proto-object read guarded on `!is_undefined() && !is_null()`, so a static explicitly set to `null` on one evaluation (`class NullAst$ extends make(null) {}`) was treated as "absent" and fell through to the parent's last-wins `CLASS_DYNAMIC_PROPS`, returning a SIBLING evaluation's `ast` instead of `null` (verified: perry read `{_tag:'OBJ'}` where node reads `null`). `null` is a present, authoritative value — only `undefined` means "absent here". Guard on `!is_undefined()` alone so a present `null` is returned and only a true miss continues to the registry / a higher ancestor. Extends the gap test + helper with a renamed null-static export (`M.NullAst.ast === null`), placed before the last non-null `make(...)` so `null` is a genuine discriminator; byte-identical to node. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #6552.
Symptom
test_gap_renamed_class_export_namespace.ts(the #1758/#1812 regression guard) fails on current main: every renamed class export read viaimport * as Mresolves to the same inherited static — the direct export'sDirectKeyword— instead of each alias's own origin class.This re-breaks effect Schema decode (the #1785 shape:
S.Number = class Number$ extends make(numberKeyword) {}re-exported asNumber, thendecodeUnknownSync(S.Number)readsS.Number.ast).Root cause
Not the namespace/alias resolution the test header points at — that still resolves
M.Numberto the correctNumber$class-ref. The collapse is in the inherited-static read.The three subclasses (
Number$,Widget$,DirectCls) allextend make(...), wheremakereturns the same class-expression templateSchemaClass— one shared compile-time class-id. Eachmake()call'sstatic astoverwritesCLASS_DYNAMIC_PROPS[template_id]["ast"], so the registry entry is last-wins (DirectKeyword).fb04be0f— "fix(runtime): inherit static data fields from a parent class (#6443)" — added a parent-class-id-chain walk overCLASS_DYNAMIC_PROPSinget_field_by_name.rs, and placed it before the older #1788 per-evaluation prototype-object walk (resolve_proto_chain_field). For a subclass of a multiply-evaluated class expression, that registry read fires first and returns the last-wins value, preempting each subclass's pinned per-evaluation parent object.(The issue guessed the #6537/#6529 window; the actual regressor is #6443, which lands in the #6504-era the issue verified as already-broken.)
Fix
crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs: interleave the two walks. At each level of the class-object proto chain, consult the class's pinned per-evaluation parent object (class_prototype_object) before the parent'sCLASS_DYNAMIC_PROPS. The pinned object carries this evaluation's own edge, so it is authoritative; the registry read stays the fallback for a plain declaration parent (#6443: Auth.jsSignInError.kind), which has no pinned object. Depth order is preserved, so a closer registry static still shadows a deeper per-evaluation one.The proto-object read is not new to this path — the #1788 arm right below already reads
class_prototype_object(class_id)the same way; this only reorders it relative to the registry read, level by level.Testing
Byte-compared against
node --experimental-strip-types(Node 26.5):test_gap_renamed_class_export_namespace— now identical (renamed global-colliding + non-colliding + direct exports).test_gap_static_field_inheritance(fix(runtime): inherit static data fields from a parent class #6443's own test, incl.delete Mid.foofallthrough),test_gap_class_expr_perf_eval_statics(effect: HttpApi server dies at startup with "TypeError: undefined is not iterable" (logger/fork already work) #6438), andtest_issue_effect_factory_static_dispatch.Per contributor convention, no version bump / CHANGELOG entry — leaving the metadata for the maintainer to fold in at merge.
Out of scope:
test_gap_zlib_3285_params, noted in the issue as a likely separate regression from the same window — not touched here.Summary by CodeRabbit
ast = nullreturnnullreliably.astis explicitlynull.