fix(runtime): generic specializations share the generic's prototype object (#7757, prototype half) - #7762
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe runtime maps monomorphized class IDs to generic origins for prototype lookup and materialization. A regression test checks specialized instances, nested inheritance, ChangesGeneric prototype identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-files/test_gap_generic_specialization_prototype_identity_7757.ts (1)
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for another nested specialization.
The test creates only
Wrap<number>. It does not verify that another nested specialization or an unspecializedWrapuses the sameWrap.prototype. Add these cases to cover the generic-origin redirect across multiple nested class IDs.Proposed test additions
const w = new Wrap<number>(); +const w2 = new Wrap<string>(); +const w3 = new Wrap(); console.log("nested proto === Wrap.prototype:", Object.getPrototypeOf(w) === Wrap.prototype); console.log("nested chain:", Object.getPrototypeOf(Object.getPrototypeOf(w)) === Gen.prototype); +console.log("nested specializations share:", Object.getPrototypeOf(w) === Object.getPrototypeOf(w2)); +console.log("nested unspecialized shares:", Object.getPrototypeOf(w) === Object.getPrototypeOf(w3));🤖 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 `@test-files/test_gap_generic_specialization_prototype_identity_7757.ts` around lines 28 - 30, Extend the test around the existing Wrap<number> instance to also instantiate another nested specialization and an unspecialized Wrap, then assert each instance’s prototype is Wrap.prototype and its prototype chain reaches Gen.prototype. Keep the existing checks and use distinct nested class IDs to cover the generic-origin redirect across multiple cases.
🤖 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 `@test-files/test_gap_generic_specialization_prototype_identity_7757.ts`:
- Around line 28-30: Extend the test around the existing Wrap<number> instance
to also instantiate another nested specialization and an unspecialized Wrap,
then assert each instance’s prototype is Wrap.prototype and its prototype chain
reaches Gen.prototype. Keep the existing checks and use distinct nested class
IDs to cover the generic-origin redirect across multiple cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fdf2f0e-87eb-4d11-a394-8c2fdb6538cf
📒 Files selected for processing (4)
changelog.d/7762-generic-specialization-prototype-identity.mdcrates/perry-runtime/src/object/class_registry/prototype_objects.rscrates/perry-runtime/src/object/class_registry/state.rstest-files/test_gap_generic_specialization_prototype_identity_7757.ts
78e8846 to
885a63c
Compare
Merging as v0.5.1448A/B'd on my host — The contradiction framing is the right diagnosis. Both registries was necessary, not belt-and-braces. The non-regression I most wanted to see is in the table: The two findings on
|
…structor and prototype entries (#7826) * fix(runtime): a generic specialization answers with its generic's constructor and prototype entries `new Gen<number>()` is monomorphized into a separate class (`Gen$num`, `monomorph::mangle::generate_specialized_name`) with its own class id, and the instance is stamped with THAT id. TypeScript erases type arguments, so at runtime there is exactly one `Gen` — the specializations are an implementation detail that was leaking through the id-keyed surfaces. #7575 fixed `instanceof`, #7632 fixed `constructor.name`, #7762 fixed the two prototype-object registries. Two holes remained, both keyed on the raw id: 1. THE CONSTRUCTOR VALUE. `class_object_props`'s instance arm synthesized the class ref straight from `(*obj).class_id`, so `a.constructor !== Gen` and `a.constructor !== b.constructor`. #7632 made this WORSE before better: both report the name `Gen`, so two values printed identically and compared unequal. 2. THE PROPERTY LOOKUP CHAIN — the one #7762's prototype-object aliasing did not reach, and the more damaging of the pair. `lookup_prototype_method` walked the PARENT chain from the specialization's id, so a patch on `Gen.prototype` was invisible on a specialized instance: `Gen.prototype.tag = "G"` then `a.tag` gave `undefined` while `Object.getPrototypeOf(a) === Gen.prototype` reported `true`. The two edges disagreed about the same object. Both take the origin edge the other three surfaces already take. In the chain walk the generic is tried BEFORE the parent, because it is an alias rather than an ancestor — a specialization's parent chain is its generic's parent chain, so hopping to the parent first would walk past `Gen` and never come back. That also keeps the walk line-count-neutral, which `construct.rs` requires: it sits exactly at the 2000-line cap. METHOD DISPATCH IS DELIBERATELY NOT ALIASED. It runs off the per-class-id vtable, so each specialization keeps its own monomorphized bodies — the same boundary #7762 drew, and the reason this is not a `CLASS_REGISTRY` parent edge (that chain also resolves `super()` construction and would re-run the wrong constructor). `test-files/test_gap_generic_specialization_constructor_identity_7757.ts` is byte-identical to node. It pins the edge in the negative direction too: two different generics stay distinct, a specialized SUBCLASS reports the subclass rather than the base, and declared methods still dispatch per specialization. Fixes #7757 * docs(changelog): add fragment for the #7757 specialization identity fix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Partial fix for #7757 — the prototype identity. The issue stays open for the
.constructoredge; reasoning and what I ruled out at the bottom.What was wrong
Each monomorphized specialization materialized its own prototype object, so:
Those first two cannot both hold, and that contradiction is the actual bug: the answer depended on which class id you asked through. TypeScript erases type arguments, so at runtime there is exactly one
Genand oneGen.prototype; the specializations are an implementation detail ofmonomorph::mangle::generate_specialized_name.Fix
Both prototype registries resolve a specialization through
class_generic_origin— the same edgeinstanceofuses (#7575) and the display name uses (#7632), now applied to the prototype surface.It had to be both:
CLASS_DECL_PROTOTYPE_OBJECTSandCLASS_PROTOTYPE_OBJECTSare the two paths CLAUDE.md's "known-weak areas" flags as having disagreed about the same chain before, and redirecting only the decl one leftgetPrototypeOfand the lookup chain answering differently.Method dispatch is untouched — it runs off the per-class-id vtable, so each specialization keeps its own monomorphized bodies.
Validation
test-files/test_gap_generic_specialization_prototype_identity_7757.ts— PASS through the harness, byte-identical to node 26.5.1. Covers two specializations, the unspecialized instance, and a nestedclass Wrap<T> extends Gen<T>chain, plus theinstanceofa Map/Set SUBCLASS is false (m instanceof MyMap); only the native base edge survives #7575 / constructor.name of a generic-class instance reports the mangled specialization (Gen$num), not Gen #7632 halves still holding.cargo test -p perry-runtime --lib: 2023 passed, 0 failed.cargo fmt --all --check,scripts/check_file_size.sh: clean.Why
.constructoris not in herea.constructor === Genis stillfalse. It does not resolve through either prototype registry, so this change cannot reach it.I applied the same redirect to
instance_constructor_value— the obvious site, and the oneget_field_by_name_tailcalls for exactly this key — and it changed nothing. Two findings worth having on the record so the next attempt doesn't repeat them:object_static_prototype()appears to materialize on first call. The tail guards theinstance_constructor_valuecall onobject_static_prototype(obj).is_none(), and adding a probe that reads it flips the branch under test. I burned two instrumented builds on artifacts from this before spotting it; anything measuring this path needs to avoid touching that accessor.class Empty {}givese.constructor === undefined(node:Empty). That is not a generics bug at all, which suggests.constructorresolution has a broader gap that monomorphization merely exposes — and that the remaining half of Generic-class specializations are distinct constructor objects: a.constructor !== Gen, and two specializations compare unequal #7757 may be better framed as that gap rather than as a specialization leak.I stopped rather than keep probing blind, and reverted the redirect that provably never runs — leaving a dead guard that looks like a fix is the pattern I flagged on #7542.
No version bump.
Summary by CodeRabbit
Bug Fixes
instanceofbehavior are now consistent across specialized and unspecialized instances.Tests
instanceof, and constructor behavior.