fix(codegen): resolve a native base class by class CHAIN, not by extends name (#6325, #6326, #6336)#6342
fix(codegen): resolve a native base class by class CHAIN, not by extends name (#6325, #6326, #6336)#6342proggeramlug wants to merge 2 commits into
Conversation
…nds name (#6325, #6326, #6336) Perry installs a native base class's surface at super() time — an own-property method bag for EventEmitter, a hidden backing MapHeader/SetHeader for Map/Set, the standard event fields for Event/CustomEvent. The trigger was keyed on the class naming that base LITERALLY in its own `extends` clause, which is the wrong key and lost the base three ways: * #6325 — a class with NO own constructor writes no super() at all, so the init never ran. `class M extends Map {}` produced an instance with no backing collection and therefore no methods at all: `m.set("a", 1)` threw `TypeError: set is not a function`. * #6326 — an INDIRECT subclass names an intermediate USER class, not the base. `class D extends B {}` over `class B extends EventEmitter {}` inherited nothing one hop away: `typeof d.on` was `undefined`. * #6336 — `new (class extends Event {})("tick")` never emitted the class expression's RegisterClassParentDynamic side effect, so the registry parent edge was never wired, `instanceof Event` was false, and every chain walk that identifies a receiver by its base bailed. native_instance_base_in_chain() replaces the literal name test: it walks extends_name through user classes that carry no constructor of their own and reports the native base the chain bottoms out in. It STOPS at any ancestor with a constructor — that ancestor's super() performs the init itself, and running it twice would re-stamp the surface over live state (a fresh listener bag over an emitter whose ctor already registered listeners, a fresh empty backing over a seeded Map). This generalizes the ctor-less walk node_stream_parent_kind already performed for the node:stream bases. Consulted from both places a derived ctor can reach the base: the implicit (no-own-ctor) `new` path in new.rs, and the explicit-super() arm in this_super_call.rs, where a chain that bottoms out in a native base previously found no ctor to inline and silently no-oped. A class expression's parent edge is a SIDE EFFECT of evaluating the expression — lower_class_expr sequences RegisterClassParentDynamic in front of the ClassRef it yields, which is why the const-bound form worked. lower_new_non_ident never went through it, lowering the class straight to a New on a synthetic name, so a class expression constructed in place registered nothing. It now sequences the same registration in front of the construction; Sequence yields its last element, so the new site still sees the instance. Making Map/Set subclassing work makes overriding a collection method reachable for the first time, so the #6316/#6322 ordering discipline has to hold there too. The override already wins (class-vtable method; the backing redirect only fires on a dispatch miss), but super.get(k) returned undefined: EventEmitter and the node:stream classes install their surface as method CLOSURES, so an override displaces a real value js_super_method_call_dynamic can still find — Map/Set have no closures, they redirect the OPERATION onto the hidden backing, so there was nothing to find. super_collection_method runs the base operation on the backing directly, as the LAST fallback in js_super_method_call_dynamic (after the class vtable and the prototype walk), so a genuine user-class parent method always wins. The receiver is rooted across js_map_set/js_set_add: both allocate and both must return the receiver, so a stale bit pattern would hand the override a dead `this`. Fixtures: test_gap_6325_map_set_subclass.ts, test_gap_6326_indirect_native_base.ts, test_gap_6336_class_expr_builtin_parent.ts — each byte-identical to node --experimental-strip-types, each carrying a no-override control and an override-still-wins guard. Full 299-test gap suite A/B'd against a pristine origin/main build (its own perry binary AND its own libperry_runtime.a / libperry_stdlib.a, since this touches the runtime): zero regressions. Validated under PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 with GC diagnostics confirming a real evacuation ran (moved_objects=5727), verifier clean.
📝 WalkthroughWalkthroughNative subclass construction now discovers supported native bases through inheritance chains, registers parents for immediately constructed class expressions, and dispatches ChangesNative subclass initialization
Class-expression parent registration
Map/Set super-method dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Constructor
participant CodegenHelpers
participant NativeRuntime
Constructor->>CodegenHelpers: resolve native base in class chain
CodegenHelpers->>NativeRuntime: emit EventEmitter, Map/Set, or Event initialization
NativeRuntime-->>Constructor: initialized subclass instance
Possibly related issues
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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/expr/this_super_call.rs (1)
183-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate the native-base chain fix into
Expr::SuperCallSpread
super(...arguments)still only special-cases directMap/Set; it skipsnative_instance_base_in_chain, so indirect subclasses likeclass Counter extends B {}whereB extends EventEmitter/Map/Set/Event/CustomEventwon’t get the native surface installed here. Mirror the same chain handling in this arm or narrow the fix’s scope.🤖 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/expr/this_super_call.rs` around lines 183 - 222, Update the Map/Set handling in Expr::SuperCallSpread to use native_instance_base_in_chain rather than checking only the immediate extends_name, so indirect subclasses also receive the native collection initialization. Mirror the existing native-base chain behavior used by Expr::SuperCall while preserving spread-argument extraction and field initializer execution.
🤖 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-codegen/src/expr/this_super_call.rs`:
- Around line 844-878: In the native-base fallback branch of the super-call
lowering flow, change the apply_field_initializers_recursive call from
FieldInitMode::SelfOnly to FieldInitMode::AfterRoot. Keep the existing
current_class_name and post-super timing unchanged so all fields after the
native root, including ctor-less intermediate classes, are initialized.
---
Outside diff comments:
In `@crates/perry-codegen/src/expr/this_super_call.rs`:
- Around line 183-222: Update the Map/Set handling in Expr::SuperCallSpread to
use native_instance_base_in_chain rather than checking only the immediate
extends_name, so indirect subclasses also receive the native collection
initialization. Mirror the existing native-base chain behavior used by
Expr::SuperCall while preserving spread-argument extraction and field
initializer execution.
🪄 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: f8d77711-6767-4e0e-b43a-eed6cb2bd4ef
📒 Files selected for processing (10)
crates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/new_helpers.rscrates/perry-hir/src/lower/expr_new/non_ident.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/map_set_subclass.rstest-files/test_gap_6325_map_set_subclass.tstest-files/test_gap_6326_indirect_native_base.tstest-files/test_gap_6336_class_expr_builtin_parent.ts
The post-super() field pass used FieldInitMode::SelfOnly, which covers only
the leaf. AncestorsOnly (applied up-front) covers only the chain ROOT — and a
native base is the root and has no TS fields. So a ctor-less class in the
middle of the chain had no site that ran its initializers at all.
Two intermediates are needed to see it: with one, the leaf and the
root-adjacent class both happen to be covered.
class A extends EventEmitter { aF = 'a-ok' }
class B extends A { bF = 'b-ok' } // <- silently uninitialized
class C extends B { cF = 'c-ok'; constructor() { super() } }
before: a-ok undefined c-ok
after: a-ok b-ok c-ok (matches node)
AfterRoot keeps chain[1..], i.e. everything below the native root, and is
equivalent to SelfOnly for the two-level case the existing tests cover.
Found by review on #6342.
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/expr/this_super_call.rs (1)
190-222: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle indirect Map/Set bases in
SuperCallSpread. This path only checks the immediateextends_name, soclass C extends B { constructor() { super(...arguments) } }withclass B extends Map {}still falls through tojs_super_construct_apply, which doesn’t install the Map/Set backing for the indirect base.native_instance_base_in_chainneeds to cover this arm too.🤖 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/expr/this_super_call.rs` around lines 190 - 222, Update the Map/Set detection in SuperCallSpread to use native_instance_base_in_chain rather than checking only the immediate extends_name, so indirect subclasses such as B extends Map are recognized. Preserve the existing map_set_kind initialization and field-initializer flow for both direct and indirect Map/Set bases.
🤖 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/expr/this_super_call.rs`:
- Around line 190-222: Update the Map/Set detection in SuperCallSpread to use
native_instance_base_in_chain rather than checking only the immediate
extends_name, so indirect subclasses such as B extends Map are recognized.
Preserve the existing map_set_kind initialization and field-initializer flow for
both direct and indirect Map/Set bases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d896d57-314e-41c8-be83-fb8dad2d4914
📒 Files selected for processing (2)
crates/perry-codegen/src/expr/this_super_call.rstest-files/test_gap_6326_indirect_native_base.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test-files/test_gap_6326_indirect_native_base.ts
Summary
Three issues, one root: perry installs a native base class's surface at
super()time, and the trigger was keyed on the class naming that base literally in its ownextendsclause. A declaration-site name match is the wrong key, and it lost the base in three different ways:class M extends Map {}super(), so the init never ran — the instance had no backing collection and therefore no methods at allclass D extends B {},class B extends EventEmitter {}new (class extends Event {})("tick")RegisterClassParentDynamicside effect, so the registry parent edge was never wired and every chain walk bailedThe fix replaces the literal name test with "the class chain reaches the base" — the same walk
node_stream_parent_kindalready performed for thenode:streambases, generalized — and closes the class-expression registration hole so the chain is walkable in the first place. This is the mechanism PR #6311 established forEventTarget: wire the registry parent edge, then resolve by chain rather than by name.Closes #6325, closes #6326, closes #6336.
What changed
perry-codegen/src/lower_call/new_helpers.rs— the shared mechanism.native_instance_base_in_chain(ctx, class)walksextends_namethrough user classes that carry no constructor of their own and reports the native base the chain bottoms out in (EventEmitter,Map,Set,Event,CustomEvent). It stops at any ancestor that has a constructor: that ancestor'ssuper()performs the init itself, and running it again would re-stamp the surface over live state — a fresh listener bag over an emitter whose ctor already registered listeners, a fresh empty backing over a seeded Map. A user class in the module shadows the builtin name, so a source-levelclass Map {}is never mistaken for the builtin.emit_native_instance_base_initemits the corresponding init and forwards the constructor args, which is exactly what the spec's implicitconstructor(...args) { super(...args) }does — and is hownew Seeded([["k", 9]])seeds its backing andnew (class extends Event {})("tick")gets itstype.The set is deliberately narrow:
Error, thenode:streamclasses,Request/Response,PromiseandArrayeach have their own construction machinery that the walk must not preempt.perry-codegen/src/lower_call/new.rs— the implicit (no-own-ctor)newpath now consults the chain walk instead of testingclass.extends_name == Some("EventEmitter"). This is what makesclass M extends Map {}work at all (#6325) and what carries the surface across a hop (#6326).perry-codegen/src/expr/this_super_call.rs— the explicit-super()arm. When the immediate parent is a real user class but the chain bottoms out in a native base (class Counter extends B { constructor() { super(); … } }), the parent-chain walk found no constructor to inline andsuper()silently no-oped. It now installs the base surface. The direct form is untouched: the existing builtin arms only fire when the immediate parent name is the base, so they never see this shape.perry-hir/src/lower/expr_new/non_ident.rs—new (class extends Base {})(). A class expression'sSubclass → Parentregistry edge is a side effect of evaluating the expression:lower_class_exprsequences aRegisterClassParentDynamicin front of theClassRefit yields, which is why the const-bound form (const K = class extends Event {}) worked. This arm never went throughlower_class_expr— it lowers the class straight to aNewon a synthetic name — so for a class expression constructed in place the registration never ran. It now sequences the same registration in front of the construction. (Sequenceyields its last element, so thenewsite still sees the instance.)perry-runtime/src/object/map_set_subclass.rs+object/class_constructors.rs—super.<m>()into the Map/Set base. Makingclass M extends Map {}work makes overriding a collection method reachable for the first time, so the #6316/#6322 ordering discipline has to hold there too. The override already wins (it is a class-vtable method, and the backing redirect only fires on a dispatch miss), butsuper.get(k)returnedundefined:EventEmitterand thenode:streamclasses install their surface as method closures, so an override displaces a real value thatjs_super_method_call_dynamiccan still find — Map/Set have no closures, they redirect the operation onto a hidden backing collection, so there was nothing to find.super_collection_methodruns the base operation on the backing directly. It is the last fallback injs_super_method_call_dynamic, after the class vtable and the prototype walk, so a genuine user-class parent method always wins. The receiver is rooted acrossjs_map_set/js_set_add(both allocate, and both must return the receiver, so a stale bit pattern there would hand the override a deadthis).Repro matrix vs
node --experimental-strip-typesmainclass M extends Map {}; m.set("a",1)TypeError: set is not a function1 11 1class S extends Set {}; s.add("x")TypeErrortrue 1true 1new Seeded([["k",9]])—extends Map, no ctorTypeError9 19 1class LeafMap extends MidMap extends MapTypeError3 1 true3 1 truesuper.set/super.getTypeError10 20 undefined10 20 undefinedclass D extends B extends EventEmitter:typeof d.onundefinedfunctionfunctionD2 → C2 → B → EventEmitter(deep chain)undefinedfunctionfunctionsuper()undefinedfunctionfunctionsuper.emit()undefinedtruetruenew (class extends Event {})("tick")false undefinedtrue ticktrue tickconst E2 = class extends Event {}; new E2("x")TypeError: Event is not a functiontrue xtrue xnew (class extends EventEmitter {})()undefinedfunctionfunctionnew (class extends Map {})()TypeError1 1 true1 1 trueextends EventEmitter;extends Mapwith an explicit ctor;new (class extends UserBase {})(); EventEmitter override +super.emit(#6316)Validation
Fixtures —
test-files/test_gap_6325_map_set_subclass.ts,test_gap_6326_indirect_native_base.ts,test_gap_6336_class_expr_builtin_parent.ts. Each is byte-identical to Node and each carries both a no-override control and an override-still-wins guard.Gap-suite A/B, 299 tests, two genuinely distinct toolchains. This PR touches
perry-runtime, so each side was built with its ownperrybinary and its ownlibperry_runtime.a/libperry_stdlib.a(via-p perry-runtime-static -p perry-stdlib-static— the rlib-only crates emit no archive), andPERRY_RUNTIME_DIRwas pinned per side. Provenance was proved rather than assumed:nmfindssuper_collection_methodin the fixed archive and not in the baseline one;PERRY_RUNTIME_DIRflips the behavior (override: undefined undefined undefined→override: 10 20 undefined), so the archive selection is demonstrably live.Result: 294 identical / 5 changed → zero regressions. Three of the five are the new fixtures (fail on
main, pass here). The other two are proven run-to-run noise, not build differences — re-running the baseline binary alone reproduces each:test_gap_console_methods— aconsole.timewall-clock line (timer1: 0.012msvs0.034ms); the baseline binary alone prints0.002ms/0.005ms/0.002msacross three runs.test_gap_module_const_local_shadow— already intest-parity/known_failures.json; it prints uninitialized memory (x+1.2e-311), and the baseline binary alone yields a different garbage float on every run.GC. Validated under
PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1, withPERRY_GC_DIAG=1confirming the run was not vacuous:[gc-evac-policy] enabled=true reason=force … moved_objects=5727 moved_bytes=383472 released_original_objects=5727. The stress fixture churns allocations around every Map/Set write, emitter dispatch andsupercall, takes an explicitminor()/collect()each round, and then asserts the backing collections, the emitter bag and the Event fields all survived relocation intact. The evacuation verifier did not trip.cargo fmt --all -- --checkclean;scripts/check_file_size.shclean.Not in scope
EventTarget— the second half of Immediately-constructed class expression loses its builtin parent edge: new (class extends Event/EventTarget {})() is parentless #6336's repro.EventTargethas no reserved class id onmain; that is exactly what the open PR fix(runtime): EventTarget methods must live on the prototype so subclasses inherit them (#6301) #6311 adds, along with the prototype method surface. (Onmaintoday even the const-boundnew K() instanceof EventTargetisfalse, so this is fix(runtime): EventTarget methods must live on the prototype so subclasses inherit them (#6301) #6311's half, not a chain-walk problem.) The class-expression registration hole closed here is the other half, and the two compose: once fix(runtime): EventTarget methods must live on the prototype so subclasses inherit them (#6301) #6311 lands,new (class extends EventTarget {})()resolves through the edge this PR now emits. Verified againstEvent, which predates fix(runtime): EventTarget methods must live on the prototype so subclasses inherit them (#6301) #6311 and reproduces onmainindependently.Mixin factory that directly returns a dynamic-parent class expression loses its binding (regression) #5952 (mixin factory returning a dynamic-parent class expression) is a different root and stays open — confirmed by building it against this branch, where it is unchanged. The factory's class expression goes through
lower_class_expr, which already emits the parent registration, so the hole fixed here isn't the one it falls into. Its bug is inperry-transform's factory specialization:resolve_factory_return_classresolves the trailingClassRefthrough theSequence, so the pass clones the returned class under the binding name and drops both the call and the binding. Fixing it means teaching that pass to skip dynamic-parent factories — unrelated to the native-base wiring here.An adjacent pre-existing gap surfaced while writing the runtime: indirect subclass of a native base inherits nothing (
class D extends B,class B extends EventEmitter) #6326 fixture, and is not introduced by this PR: a baretypeof obj.methodvalue-read sitting next to a declared-field read lets scalar replacement promote the instance to scalars (it never escapes), and a runtime-installed own property is invisible to that promotion, so the read comes backundefined. It reproduces on unmodifiedmainfor a directclass X extends EventEmitter { a = 1 }, so it is orthogonal to the class chain. The fixture exercises the emitter rather than only probing it, and the case is called out in a comment there.Summary by CodeRabbit
Map,Set,EventEmitter,Event,CustomEvent), including ctor-less and mixed constructor chains.superdispatch forMap/Setsubclass overrides to reliably reach base implementations.instanceofand native surface APIs.superbehavior.