fix(runtime): a subclass override of a native base method must win over the native surface (#6316)#6322
Conversation
…er the native surface (#6316) perry models EventEmitter and every node:stream class by stamping the base's method surface onto the INSTANCE as ordinary own properties (install_methods_on_existing_object). Own properties legitimately shadow class methods, so the #620 own-property-override probe selected the native closure in preference to a user's `class Bus extends EventEmitter { emit() {...} }` override: inheritance ran backwards and the override never executed. No error, no warning - the worst failure mode. Install a base method under its plain name only when the receiver's class chain does not declare it. When it does, stash the native closure under the reserved `__perry_native_super__<name>` key instead: invisible to ordinary property lookup, so the class method wins, but reachable from js_super_method_call_dynamic so `super.emit(...)` still lands on the real base. That super path was independently dead: a native base is not a perry class, so `class Bus extends EventEmitter` has no CLASS_REGISTRY parent edge and js_super_method_call_dynamic fell off its `parent_cid == 0` guard returning undefined. It now falls back to the displaced base method, both there and after a real parent chain bottoms out in a native base.
…fixture
Covers, byte-for-byte against node --experimental-strip-types:
- EventEmitter: the issue's exact repro (emit override + super.emit)
- several overrides at once (on + emit), each forwarding to the base
- a NON-overridden base method on an overriding subclass still reaches
the base (listenerCount / removeAllListeners)
- no-override control: a plain subclass is unaffected
- explicit constructor + instance state (the super() install path)
- the displaced base method does not leak as an own enumerable key
- node:stream Readable.push and Writable.write overrides + super
An INDIRECT native base (class D extends B, class B extends EventEmitter)
is deliberately not covered: it never runs the subclass init at all, with
or without an override, which is a separate pre-existing gap.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ 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 |
…nds name (#6325, #6326, #6336) (#6342) * fix(codegen): resolve a native base class by class CHAIN, not by extends 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. * fix(codegen): init ctor-less intermediate classes after a native base 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. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Fixes #6316.
Root cause — there are TWO, and they are independent
The issue asked whether the dispatch bypass and the
super.<m>()failure are oneroot cause or two. Two. They happen to coincide on
EventEmitter, which iswhy the repro looks like a single bug.
1. The override bypass — a native base's methods are OWN PROPERTIES
perry models
EventEmitterand everynode:streamclass by stamping the base'smethod surface directly onto the instance:
js_event_emitter_subclass_init/js_node_stream_*_subclass_initcallinstall_methods_on_existing_object, whichdoes
js_object_set_field_by_name(obj, "emit", <native closure>). Nothing liveson a prototype.
Own properties legitimately shadow class methods, and perry implements that
faithfully: the #620 own-property-override probe
(
perry-codegen/src/lower_call/method_override.rs) emits a runtimejs_object_get_own_field_or_undef(recv, "emit")before every static class-methoddispatch, and takes the own-property branch when it hits.
So for
class Bus extends EventEmitter { emit() {…} }, the probe found thenative base's
emitsitting on the instance and called it in preference toBus.prototype.emit. Inheritance ran backwards. The mechanism designed tohonour
this.method = fnwas being fed the base class's own prototype surface.The override never ran, with no error and no warning — a wrong answer rather than
a crash, the worst failure mode.
2.
super.<m>()into a native base was independently deadjs_super_method_call_dynamicresolves fromget_parent_class_id(child_class_id). A native base is not a perry class —EventEmitterowns no class id and noCLASS_REGISTRYentry — so aclass Bus extends EventEmitterhas no parent edge at all. The helper fellstraight off its
parent_cid == 0guard and returnedundefined.This one is not specific to
EventEmitter: it is whysuper.push()on anArraysubclass silently no-ops andsuper.toString()on anErrorsubclassyields
undefined(see the matrix below).The fix
Install a base method under its plain name only when the receiver's class chain
does not declare it (
method_owner_class_id— the class registry is populatedat module init, long before any
new). When the subclass does override it, thenative closure is stashed under the reserved
__perry_native_super__<name>keyinstead:
own-property probe now misses, exactly as it should;
js_super_method_call_dynamic, sosuper.emit(…)landson the real base implementation. The fallback runs last, after the vtable
and prototype-registry lookups, so a genuine user-class parent method always
wins — the same "native is the fallback, the class chain wins" ordering
discipline PR fix(runtime): EventTarget methods must live on the prototype so subclasses inherit them (#6301) #6311 established for
EventTarget.addListenerkeeps aliasing the baseoneven when the subclass overrideson,matching Node (
EventEmitter.prototype.addListener === EventEmitter.prototype.onrefers to the base function).
The reserved key is hidden from
Object.keys/for…in/JSON.stringifyviais_internal_runtime_key_bytes. That is a strict move toward Node: thedisplaced method previously sat on the instance under its plain name, so
Object.keys(bus)wrongly reportedemitas an own key.install_methods_on_existing_objectalso now roots the receiver and the NaN-boxedthisin aRuntimeHandleScope— key interning andjs_closure_allocbothallocate and can GC-move the receiver, and the loop stores through it repeatedly.
Affected-base matrix
Measured on a clean build of the base commit, each case = override +
supercall,diffed against
node --experimental-strip-types.super.<m>()reaches base?EventEmitternode:stream(Readable,Writable, …)EventTargetArraysuper.push()no-opsErrorsuper.toString()→undefinedPromisesuper.thennot a functionMap/SetOnly
EventEmitterand the stream classes stamp their surface onto the instance,so cause 1 is exactly those. Cause 2 spans every native base.
Two adjacent pre-existing bugs surfaced while measuring; both are unchanged by
this PR (verified byte-identical to the base build) and both deserve their own
issues rather than being folded in here:
class X extends Map/extends Setis broken with no override at all —class M extends Map {}; new M().set("a", 1)already throwsTypeError: set is not a functionon main. Theirsuperfailures in the tableare a symptom of that, not of override ordering.
class D extends B,class B extends EventEmitterinherits nothing, overrideor not, because the init only fires when
extends_nameis literallyEventEmitter. This is theEventEmittermirror of the chain-walk fix(runtime): EventTarget methods must live on the prototype so subclasses inherit them (#6301) #6311 addedfor
EventTarget. The fixture documents the exclusion explicitly.Before / after
Testing
test-files/test_gap_6316_native_base_override.ts— byte-identical tonode --experimental-strip-types. Covers: the issue's repro; several overridesat once (
on+emit) each forwarding to the base; a non-overridden basemethod on an overriding subclass still reaching the base; a no-override
control; an explicit constructor + instance state; own-key hygiene; and
Readable.push/Writable.writeoverrides +super.compiler, swapping only the runtime
.a(baseline vs fixed) so the comparisonisolates this change. Three tests differed: this PR's new fixture, plus
test_gap_console_methodsandtest_gap_module_const_local_shadow— bothproven flaky on the baseline binary alone (
console.timewall-clock values;a pre-existing uninitialized-denormal print that varies every run).
cargo test -p perry-runtime— 1264 passed, 0 failed.cargo fmt --all -- --check, file-size gate, GC store-site inventory, and theaddr-class audit all pass.
Relationship to PR #6311
Independent — no file overlap, and this branch is based on
origin/main, not on#6311's branch. #6311 gives
EventTargeta method surface it never had and placesit last so an override wins; this PR removes a surface that was wrongly placed
first. They implement the same ordering rule (the user's class chain wins,
native is the fallback) from opposite directions and should merge cleanly in
either order.