Skip to content

fix(runtime): per-evaluation inherited static wins over last-wins registry props (#6552) - #6556

Merged
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/6552-renamed-class-export-namespace
Jul 18, 2026
Merged

fix(runtime): per-evaluation inherited static wins over last-wins registry props (#6552)#6556
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/6552-renamed-class-export-namespace

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Fixes #6552.

Symptom

test_gap_renamed_class_export_namespace.ts (the #1758/#1812 regression guard) fails on current main: every renamed class export read via import * as M resolves to the same inherited static — the direct export's DirectKeyword — instead of each alias's own origin class.

expected (node):                          got (perry, before):
(1) M.Number.ast._tag: NumberKeyword     (1) M.Number.ast._tag: DirectKeyword
(2) M.Widget.ast._tag: WidgetKeyword     (2) M.Widget.ast._tag: DirectKeyword

This re-breaks effect Schema decode (the #1785 shape: S.Number = class Number$ extends make(numberKeyword) {} re-exported as Number, then decodeUnknownSync(S.Number) reads S.Number.ast).

Root cause

Not the namespace/alias resolution the test header points at — that still resolves M.Number to the correct Number$ class-ref. The collapse is in the inherited-static read.

The three subclasses (Number$, Widget$, DirectCls) all extend make(...), where make returns the same class-expression template SchemaClass — one shared compile-time class-id. Each make() call's static ast overwrites CLASS_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 over CLASS_DYNAMIC_PROPS in get_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's CLASS_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.js SignInError.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):

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

  • Bug Fixes
    • Fixed inherited static property resolution for dynamically evaluated classes to use the current per-evaluation prototype precedence before falling back to registered dynamic values.
    • Corrected repeated class-expression evaluation behavior, including handling of explicitly overridden or deleted static fields.
    • Ensured renamed class exports with ast = null return null reliably.
  • Tests
    • Added regression coverage for renamed class exports where static ast is explicitly null.

…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>
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 78b4a75b-ff49-4119-8908-494d43af581e

📥 Commits

Reviewing files that changed from the base of the PR and between e42a3b8 and bdd300a.

📒 Files selected for processing (3)
  • crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs
  • test-files/_helpers/renamed_class_export.ts
  • test-files/test_gap_renamed_class_export_namespace.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs

📝 Walkthrough

Walkthrough

Static inherited data-property resolution now checks each ancestor’s per-evaluation pinned prototype before falling back to CLASS_DYNAMIC_PROPS. A regression fixture and assertion verify that an explicit null static value is returned without falling through to another evaluation.

Changes

Static inheritance resolution

Layer / File(s) Summary
Pinned parent lookup precedence
crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs
Ancestor static data lookup recursively checks pinned prototype objects first, treats non-undefined values including null as authoritative, and consults CLASS_DYNAMIC_PROPS only for non-deleted keys when pinned lookup is absent.
Null static value regression coverage
test-files/_helpers/renamed_class_export.ts, test-files/test_gap_renamed_class_export_namespace.ts
Adds a renamed class with ast initialized to null and asserts namespace access returns null rather than an inherited value from another evaluation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • PerryTS/perry#6443: Adjusts inherited static-field traversal and deleted-key handling in the same runtime lookup path.
  • PerryTS/perry#6449: Introduces related pinned-parent precedence behavior in get_field_by_name.
  • PerryTS/perry#6536: Changes static and inherited property-resolution precedence for per-evaluation class objects.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main runtime fix and matches the regression addressed by the PR.
Description check ✅ Passed The description covers the problem, root cause, fix, and testing, and includes the linked issue.
Linked Issues check ✅ Passed The changes address #6552 by fixing namespace-renamed class export static resolution and preserve the intended behavior.
Out of Scope Changes check ✅ Passed The added test and null-handling follow-up are consistent with the stated regression fix and not unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 241d867 and e42a3b8.

📒 Files selected for processing (1)
  • crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs

Comment thread 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>
@proggeramlug
proggeramlug merged commit a7dd328 into PerryTS:main Jul 18, 2026
2 checks passed
@proggeramlug
proggeramlug deleted the fix/6552-renamed-class-export-namespace branch July 18, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant