fix(runtime): link a native-base subclass prototype to the real builtin prototype - #10614
proggeramlug wants to merge 4 commits into
Conversation
…in prototype
class Sub extends EventEmitter {} left Sub.prototype's [[Prototype]] on
Object.prototype instead of EventEmitter.prototype. class_decl_prototype_value
resolves a registered parent class id by recursing into itself, which bails
for a RESERVED native-builtin parent id (builtin_parent_reserved_class_id in
perry-codegen wires this edge for a native base with no declared-class
registration), silently falling through to the Object.prototype default.
Resolve EventEmitter/EventEmitterAsyncResource's real, closure-identity-keyed
prototype object through the same js_function_prototype_value_for_read path
the existing runtime-function-valued-parent branch already uses, so
Object.getPrototypeOf(Sub.prototype) === EventEmitter.prototype holds by
identity. Fixes #10599.
…s-parent id builtin_parent_reserved_class_id (perry-codegen) already gained an "EventEmitter" => 0xFFFF0076 entry (#10592), but not its AsyncResource variant: class Sub extends EventEmitterAsyncResource {} left get_parent_class_id(Sub) unresolved entirely (no parent-edge call is ever emitted), so the perry-runtime getPrototypeOf-identity fallback for reserved native-builtin parents (#10599) never runs for it -- the same gap #10592 closed for plain EventEmitter, one id over.
Covers: direct subclass with field+ctor, fieldless no-ctor subclass, two-level (indirect) subclass, unnamed and named-via-indirection class expressions, EventEmitterAsyncResource, in/for-in walking the same chain, own-enumeration non-regression, dispatch still works, an instanceof-still-holds control (guards #10592), and a class-extends-Array control (dedicated ArrayHeader path, unaffected by this fix). Verified: fails on the runtime fix alone (state.rs reverted, standalone) with every getPrototypeOf/instanceof/`in` assertion false where Node says true; passes with both the runtime fix and both codegen table entries. Two pre-existing, unrelated gaps hit while writing this were deliberately left uncovered (documented inline, not fixed here): EventEmitterAsyncResource.prototype's own [[Prototype]] does not chain to EventEmitter.prototype (a native-to-native link, not a user `extends` subclass), and Object.keys(new Sub()) leaks EventEmitter's prototype methods as literal own enumerable instance properties instead of Node's real _events/_eventsCount/_maxListeners own fields (CLAUDE.md "Native base-class subclassing -- a native base's surface is installed at super() time"). Both reproduce identically with the fix reverted, so neither is caused by it.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Landed via merge train #10652 (v0.5.1596). All source commits preserve authorship; merged main matches the validated train exactly. |
Summary
class Sub extends EventEmitter {}leftObject.getPrototypeOf(Sub.prototype) !== EventEmitter.prototype(andsame for
EventEmitterAsyncResource).instanceof, prototype-chain identity forin/for...in, and dispatchall work off other mechanisms; this is specifically the
[[Prototype]]object-identity link.This PR depends on #10592 (open, not yet merged) and is based on its branch
(
fix/10478-10479-instanceof-value-kinds), notmain. Reason found during validation, below.Root cause
Two independent gaps, both required for the fix to actually take effect:
Runtime (
crates/perry-runtime/src/object/class_registry/state.rs, already fixed in this branch's historyby a prior pass).
class_decl_prototype_valueresolves a registered parent class id by recursing intoitself, which bails for a RESERVED native-builtin parent id (
class_name_for_idreturnsNone, sinceEventEmitterhas nojs_register_class_nameregistration of its own). The lookup silently fell through tothe
Object.prototypedefault. Fixed by resolving the real, closure-identity-keyed prototype throughjs_function_prototype_value_for_read— the same helper the existing runtime-function-valued-parent branchalready uses.
Codegen (
crates/perry-codegen/src/expr/instance_misc1.rs, this PR's own commit). The runtime fix aboveonly runs if
get_parent_class_id(Sub)actually resolves to the reserved id in the first place — thatrequires
builtin_parent_reserved_class_id("EventEmitter")to returnSome(0xFFFF0076)so codegen emits thejs_register_class_parentcall. That entry does not exist onmain; fix(runtime): resolve instanceof and Object.create constructor per receiver value kind #10592 adds it (forinstanceof'ssake). Its
EventEmitterAsyncResourcecounterpart is not added by fix(runtime): resolve instanceof and Object.create constructor per receiver value kind #10592 either — this PR adds it(
"EventEmitterAsyncResource" => 0xFFFF0077), the one piece genuinely new here.Found by validation, not assumed: with only the runtime fix and
main's currentbuiltin_parent_reserved_class_idtable (no #10592),get_parent_class_idnever resolves at all forEventEmitter/EventEmitterAsyncResource— the runtime fix is dead code. Confirmed by building againstmaindirectly first (gap test failed identically with or without the runtime fix), then rebasing onto #10592's
branch, where the runtime fix's own effect became visible.
Tests
test-files/test_gap_10599_eventemitter_prototype_identity.ts. Covers: direct subclass with field + ctor,fieldless no-ctor subclass, two-level (indirect) subclass, unnamed and named-via-indirection class expressions,
EventEmitterAsyncResource,in/for...inwalking the same chain, own-enumeration non-regression on thesubclass's own field, dispatch (
on/emit) still works, an instanceof-still-holds control(
new Sub() instanceof EventEmitter: true, guarding against a #10592 regression), and aclass ArraySub extends Array {}control (dedicatedArrayHeaderpath, should be unaffected).Fails without the fix, confirmed two ways:
getPrototypeOf/instanceof/inassertion in the test read
falsewhere Node readstrue.independent check: same result.
node --experimental-strip-types(manual diffAND
PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10599_eventemitter_prototype_identity→PASS, 100% parity).Two pre-existing, unrelated bugs found while writing this test — deliberately left OUT of it (both verified
present identically with the runtime fix reverted, so neither is caused by this change):
Object.getPrototypeOf(EventEmitterAsyncResource.prototype) === EventEmitter.prototypeisfalsein Perry.This is
EventEmitterAsyncResource's own internal chain toEventEmitter.prototype(a native-to-native linkset up wherever its
.prototypefirst materializes), not a userextendssubclass — outside this fix'smechanism (
class_decl_prototype_valuenever runs for it; it has no declared-class registration).Object.keys(new Sub())returns["tag","on","once","prependListener",...](everyEventEmitter.prototypemethod as a literal OWNenumerable property) instead of Node's
["_events","_eventsCount","_maxListeners","tag"]. Perry's native-basesuper()handling installs the native surface directly onto the instance as own properties (CLAUDE.md"Known-weak areas: Native base-class subclassing — a native base's surface is installed at
super()time")and never sets the real internal fields. An own-property-enumeration defect, orthogonal to the
[[Prototype]]chain identity this PR fixes.
python3 scripts/check_test_registration.py: OK, 332 files checked against 4 registries, none newly dark.Validation
RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --tests: 3982 passed, 4 ignored, 2 pre-existingfailures unrelated to this change —
gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_checkand
gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds. Both assertdebug_assert!-gated behavior by name and design (their own doc comments say so);--releasecompilesdebug_assert!out (see CLAUDE.md's own callout on this), so they fail under--releaseon any commit,independent of this change. Not touched by this diff (GC internals, unrelated file).
cargo test --release -p perry-codegen --tests: all green, 0 failures (checked everytest result: okline)./opt/node-v26.5.1-linux-x64, matching.node-version): byte-identical, harnessPASS, 100% parity.check_test_registration.py: OK (above).run_lint_gates.sh(owner directive: ignore CI/lint for this PR —lint's "Publicbenchmark evidence freshness" step is known-red on every open PR right now, pre-existing and unrelated), full
gap suite (owner directive to not chase CI), perf A/B (this resolves a prototype object once at
class-registration time — a one-time, per-class-declaration cost, not a per-operation hot path — so no
measurable regression is expected; not measured), CodeRabbit review pass, package-level repro (issue does not
name a specific npm package).
What I did NOT verify
run_lint_gates.sh/ CI (owner directive for this PR).itself is still in review.
Fixes #10599