fix(instanceof): #7575 — a monomorphized generic class is an instance of the generic it came from - #7631
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ 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 (3)
📝 WalkthroughWalkthroughThe change records generic class origins in HIR, registers specialization-to-origin links at runtime, and extends ChangesGeneric
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 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 |
72994ab to
02cebe1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-codegen/src/codegen/emission_order_tests.rs (1)
242-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the complete construct is emitted before comparing IR.
The closure check proves only that one registration exists. The tower check proves only that some
@perry_method_symbol exists. This fixture also emits per-class method wrappers with that prefix. An incomplete dispatch tower can therefore pass the determinism test if both compilations produce the same incomplete IR.Use
registered_closure_ids(&first).len() == N as usizeandtower_arm_classes(&first).len() == N as usizefor the two liveness checks.Proposed test-oracle fix
- assert!( - first.contains("call void `@js_register_function_name`("), - "liveness: fixture emitted no function-name registrations" - ); + assert_eq!( + registered_closure_ids(&first).len(), + N as usize, + "liveness: expected one function-name registration per closure" + ); - assert!( - first.contains("`@perry_method_`"), - "liveness: fixture emitted no class methods" - ); + assert_eq!( + tower_arm_classes(&first).len(), + N as usize, + "liveness: expected one dispatch-tower arm per implementing class" + );Also applies to: 426-429
🤖 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/codegen/emission_order_tests.rs` around lines 242 - 245, Strengthen the liveness assertions in the emission-order determinism test: replace the single function-name registration check with registered_closure_ids(&first).len() == N as usize, and replace the broad `@perry_method_` symbol check with tower_arm_classes(&first).len() == N as usize. Apply the same updates to the corresponding assertions in the second location.
🤖 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.
Outside diff comments:
In `@crates/perry-codegen/src/codegen/emission_order_tests.rs`:
- Around line 242-245: Strengthen the liveness assertions in the emission-order
determinism test: replace the single function-name registration check with
registered_closure_ids(&first).len() == N as usize, and replace the broad
`@perry_method_` symbol check with tower_arm_classes(&first).len() == N as usize.
Apply the same updates to the corresponding assertions in the second location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b60e723-d110-49cf-a863-d15e7fd015b6
📒 Files selected for processing (2)
crates/perry-codegen/src/codegen/emission_order_tests.rscrates/perry-codegen/src/codegen/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen/src/codegen/mod.rs
… of the generic
`m instanceof MyMap` was false for a `class MyMap<K, V> extends Map<K, V>`
instance while `m instanceof Map` was true. The issue read this as a Map/Set
subclass / prototype-chain defect. It is neither: the mechanism is
MONOMORPHIZATION, and Map/Set had nothing to do with it.
Perry specializes generic classes. `class Gen<T> {}` plus `new Gen<number>()`
emits a SECOND class named `Gen$num` (monomorph::mangle::generate_specialized_name)
with its own class id, and the instance is stamped with that id — while
`x instanceof Gen` resolves the RHS to the GENERIC's id, which appears nowhere
in the specialization's parent chain. The bisect that pins it:
class Gen<T> extends Base {} new Gen<number>() -> instanceof Gen false
class Gen<T> extends Base {} new Gen() -> instanceof Gen true
class Conc extends Base {} new Conc() -> instanceof Conc true
class GenNoExtends<T> {} new G<number>() -> instanceof G false
The last row has no base class at all, so this was never about `super()`-to-a-
native-base wiring. `class MyMap<K, V> extends Map<K, V>` is simply the
idiomatic spelling, which is why it surfaced there.
HIR now records `Class::specialized_from`; codegen emits one
`js_register_class_generic_origin(spec, generic)` per specialization next to the
parent edges; and `instanceof`'s chain walk (now one shared, depth-bounded
`class_chain_reaches`, used by both the static and the dynamic-RHS path) follows
that edge as well as `extends`.
It is deliberately a SEPARATE edge, not a CLASS_REGISTRY parent edge: that chain
also resolves `super()` construction, static-method lookup and vtable dispatch,
so splicing the generic in between a specialization and its real base would
re-run the wrong constructor.
The Array-side sibling #7603 left unfixed is covered by the same mechanism —
`new GenArr<number>() instanceof GenArr` now holds. `constructor.name` still
reports the mangled `Gen$num`; that is the same root cause on a different
surface and is filed separately rather than folded in here.
Validated locally: new gap test byte-identical to node 26.5.1 and byte-identical
again under PERRY_GC_ZEAL=1 + PERRY_GC_PROTECT_FROMSPACE=1; 4 new runtime unit
tests over the walk (including that the edge stays directional and does not make
sibling specializations match); test_gap_6325 and test_gap_7570 tightened to
assert the subclass edge the issue asked for.
…, and the remaining Class construction sites Expands the gap test past the Map/Set framing the issue was filed under: a generic class over a PLAIN base, over NO base, and over Array all failed identically before the fix, which is what identifies the mechanism as monomorphization rather than native-base wiring. Adds the sibling-specialization negatives so the new edge is shown to be directional rather than a widening. Measured on pristine origin/main: every NON-generic Array-subclass instanceof already held (new MyArr(), new Indirect(), MyArr.from([...])) and only the generic spelling was broken, so the note in test_gap_7541_array_subclass_inherited_statics.ts claiming the non-generic form as a gap was stale; corrected in place. Also threads specialized_from through the remaining Class construction sites (test fixtures and CJS/anon-shape scaffolding) that only --all-targets sees.
02cebe1 to
ea253f0
Compare
Audit before merge — verified, merged as v0.5.1359The corrected diagnosis is right, and I reproduced the bisect that proves
A generic class with no Map anywhere fails identically; a non-generic Map chain Following the origin edge instead of adding a parent edge is the load-bearing Gates: runtime 1,894/0, codegen 694/0, hir 281/0, all five lint scripts + The merge-order lesson here is worth generalisingThis branch was textually conflict-free and still did not compile: #7627 Two things follow, and I'd like both treated as standing practice for the
|
1 participant
Closes #7575.
Root cause: it is not Map/Set, and it is not the prototype chain
The issue reads
m instanceof MyMap === falseas a native-base-subclassing /class-registry-parentage defect, and points at CLAUDE.md's Native base-class
subclassing and Two prototype-resolution paths entries. Both are the wrong
tree. The mechanism is monomorphization.
Perry specializes generic classes.
class Gen<T> {}plusnew Gen<number>()emits a SECOND class named
Gen$num(
crates/perry-hir/src/monomorph/mangle.rs:14,specialize.rs:50) carrying itsown class id, and the instance is stamped with that id — while
x instanceof Genresolves the RHS throughctx.class_idsto the generic'sid (
crates/perry-codegen/src/expr/instance_misc1.rs:346), which appears nowherein the specialization's parent chain.
js_instanceof's walk therefore answersfalsefor the class the user actually wrote, andtruefor the base — exactlythe "only the native base edge survives" symptom.
The bisect that identifies it, measured on pristine
main:The last two rows settle it: a generic class with an ordinary base, and one with
no base at all, fail identically.
class MyMap<K, V> extends Map<K, V>is simplythe idiomatic spelling, which is why it surfaced there.
constructor.nameon thesame instances reports
Gen$num— the same leak on a different surface.The fix
Class::specialized_from(the generic's name), set byspecialize_class.js_register_class_generic_origin(spec, generic)perspecialization in the module-init prelude, next to the existing
js_register_class_parentedges.instanceof's chain walk — now a single shared, depth-boundedclass_chain_reaches, used by both the static and the dynamic-RHS path, whichpreviously had two hand-rolled copies (one of them uncapped) — follows the
origin edge as well as
extends.It is deliberately a separate edge, not a parent edge.
CLASS_REGISTRY'schain also resolves
super()construction(
object/class_constructors.rs:652), static-method lookup and vtable dispatch,so splicing
Genin betweenGen$numand its real base would re-run the wrongconstructor. Only
instanceofmay follow this one, and the runtime module saysso at the declaration.
Did the fix need rooting discipline?
No. The whole change is
u32 -> u32class-id bookkeeping — no heap pointers, nonew cache of a
*mut, so nothing to register withgc_register_mutable_root_scannerand nothing forraw_handle_debt.pyto count(it is unchanged at 998). The gap test is nevertheless byte-identical under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1.The Array-side sibling #7603 left unfixed
One mechanism covers both families, and the note about it was stale. Measured
on pristine
main:So the only broken Array case was the generic spelling, and this PR fixes it. The
comment in
test_gap_7541_array_subclass_inherited_statics.tsthat namedsub instanceof MyArras a pre-existing gap was wrong; it is corrected in placerather than left to mislead the next reader. Nothing is left to file on the
Array side.
Sabotage, both ways
class_generic_originreturnNoneturns two of the four new runtimeunit tests red and turns the gap test red at 8 lines
(
1 unannotated: false true,6 seeded: false true,8 dynamic,9/10 ...).assert the edge is directional and does not widen matches (sibling
specializations must not match each other, a generic is not an instance of its
own specialization), so they would catch the opposite mistake.
Validation (local; CI backlog is deep, so this is the evidence)
test-files/test_gap_7575_map_set_subclass_instanceof.ts— byte-identical tonode --experimental-strip-typeson the pinned 26.5.1, exit 0. Coversinstanceofagainst the subclass, the native base, an unrelated class, amulti-level chain (
class A extends Map {}; class B extends A {}; class C extends B {}), an explicit-super()subclass and a subclass of it, aniterable-seeded instance,
Symbol.hasInstancein both directions (it stilltakes precedence over the chain walk), the dynamic RHS,
instanceofover anuntyped parameter, and the generic-over-plain / generic-over-nothing /
generic-over-Array shapes plus their non-generic controls and sibling
negatives.
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, compiled withPERRY_GC_MOVING_LOOP_POLLS=1.test_gap_6325_map_set_subclass.tsandtest_gap_7570_map_set_declared_base_type.tstightened to assert thesubclass edge, as
instanceofa Map/Set SUBCLASS is false (m instanceof MyMap); only the native base edge survives #7575 asked — both still byte-identical.test_gap_7541,test_gap_7574,test_gap_7563,test_gap_6232,test_gap_4099,test_gap_5592,test_edge_generics,test_generic_class,test_edge_class_advanced,test_edge_complex_patterns,test_edge_interfaces,test_data_pipeline,test_gap_repsel_element_shape_loop_clone,test_gap_intl_rtf_auto_instanceof_6960all byte-identical to node.perry-runtimeunit tests over the walk.cargo test -p perry-runtime1890 passed / 0 failed;
-p perry-hirand-p perry-transformall green.cargo fmt --all -- --check,check_file_size.sh,addr_class_inventory.py,raw_handle_debt.py(998, unchanged),class_id_collisions.py,check_test_registration.py,gc_store_site_inventory.py,workspace_architecture.py,gap_snapshot.py --self-testall pass.Pre-existing failures untouched by this PR, each A/B'd against pristine
origin/mainin this worktree:perry-codegen's integration suites (crates/perry-codegen/tests/*.rs, whichdo not run per-PR) fail the same 23 tests, byte-identical list, on both
arms.
test_harness_class_mixins,test_issue_562_stream_subclassandtest_issue_806_curried_factory_extends_captureproduce byte-identical Perryoutput on both arms.
cargo clippyerrors incrates/perry-ffi/src/jsvalue.rs(approx_constanton
3.14) are pre-existing in a file this PR does not touch.Known, deliberately not folded in
constructor.namestill reports the mangledGen$numrather thanGen. Sameroot cause (monomorphized identity leaking to a user-visible surface), different
surface (the class display-name registry), and it can move error-message text —
so it belongs in its own change with its own parity sweep, not bundled into an
instanceoffix. Filed as #7632.Rebased onto v0.5.1357 (#7627) — clean as text, but it did NOT compile
Rebasing this onto current main produced zero conflicts, and
git merge-tree origin/main <this>exits 0. That verdict was wrong in the waythat matters: the merge does not build.
#7627 added a new
perry_hir::Classliteral incrates/perry-codegen/src/codegen/emission_order_tests.rs, and this PR widensthat struct with
Class::specialized_from. Neither side touches a line theother side touches, so there is nothing for a textual merge to flag — but the
result is
E0063: missing field specialized_from. Fixed here in its own commit.Worth stating plainly because it generalises: textual mergeability is not a
merge check when one branch widens a struct another branch constructs. The
only reliable check is
cargo check --all-targetson the merge result, and--all-targetsis load-bearing — the offending literal is in a#[cfg(test)]fixture, so a plain
cargo checkstays green and the break surfaces later, in ajob that does build tests.
This PR does not touch
expr/instance_misc1.rs,logical_collections.rsormap_set.rs, so it has no overlap with #7627's rooting migration itself.Re-verified after the rebase, not before
test_gap_7575_map_set_subclass_instanceof.tsbyte-identical to node 26.5.1,exit 0; byte-identical again under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, compiledwith
PERRY_GC_MOVING_LOOP_POLLS=1.test_gap_7570,test_gap_6325(both tightened by this PR) andtest_gap_7541all byte-identical.cargo check --all-targetsover the workspace: clean.-p perry-runtime1890 passed / 0failed;
-p perry-codegen --lib694 / 0 (ledger tests included);-p perry-hir281 / 0;-p perry-transform56 / 0.so it was re-run rather than reasoned about): 129/129 sources, 149
.ll,2452 functions / 9846 root stores, 0 violations in dominance mode with
40/40 seeded caught, and 0
--unrooted-allocasviolations..github/workflows/test.ymlgreen, includingraw_handle_debt.pyat 998 (unchanged — this PR isu32 → u32bookkeeping, no heap pointers).
Conflict-free against #7626 (
git merge-treeof the two heads exits 0), and#7626 adds no
Classliteral, so this PR's struct widening cannot break it ineither merge order.
Summary by CodeRabbit
Bug Fixes
instanceofchecks for monomorphized generic classes and their inheritance chains.Map,Set, and other specialized subclasses, including dynamic constructor checks.Symbol.hasInstanceimplementations.Tests
Chores