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 |
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
|
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.