Drop the old parent's members from a relinked class chain - #11764
Conversation
After Object.setPrototypeOf(C.prototype, X), a method, getter or setter declared in C's old parent class stayed visible on C's instances: typeof inst.m, "m" in inst, inst.m() and super.m() all still answered from the declared parent. #11738 made the generic class-chain read follow the recorded link. The walks over declared members did not: lookup_class_method_in_chain, method_owner_class_id, the `in` vtable fallback and the call dispatcher's parent walks followed get_parent_class_id, fixed at declaration. They now step through instance_chain_parent_class_id, which stops at a class whose prototype a user relinked; the generic read continues on the link, so a relink onto another class's prototype finds that class's methods. The compiler resolves inherited names ahead of time along the declared extends chain: the dispatch tower for untyped receivers and super.m() call the ancestor's body directly. Object.setPrototypeOf already retires every direct-method guard byte and bumps VTABLE_GEN, but only the narrow tower's shape probe read those bytes. The wide tower (past eight implementors) and super.m() now read them too, so a redefined parent method no longer keeps running its old body there either. With the bytes set, super.m() goes to js_super_method_call_dynamic, which reads the home object's recorded link and throws when it carries no callable. The A2 read sites need nothing: the relink restamps C.prototype's shape.
The one_shape_class_relink_methods fixture (node 26.5.1's output) covers reads, in, Reflect, calls at primed and fresh sites, own members, null and class-prototype targets, accessors, super.m(), statics, an intermediate relink, relinking back, and class-typed receivers. The integration test runs it under forced evacuation and requires the class read sites to have primed and hit, plus a wide dispatch tower seeing a redefined parent method.
proggeramlug
changed the title
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe runtime now tracks declared class prototype relinks, limits instance-method lookup to the live instance prototype chain, and invalidates cached dispatch guards. Compiled method and ChangesPrototype relinking and dispatch
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant CompiledSuperCall
participant PrototypeMethodGuard
participant DynamicSuperDispatcher
participant RelinkedPrototypeChain
CompiledSuperCall->>PrototypeMethodGuard: Check invalidation bytes
alt Guard passes
CompiledSuperCall->>CompiledSuperCall: Call statically resolved method
else Guard fails
CompiledSuperCall->>DynamicSuperDispatcher: Pass class ID, method name, receiver, and arguments
DynamicSuperDispatcher->>RelinkedPrototypeChain: Resolve the named property
RelinkedPrototypeChain-->>DynamicSuperDispatcher: Return the property value
DynamicSuperDispatcher->>DynamicSuperDispatcher: Call callable value with receiver and arguments
end
Merge Risk: ⚪ Minimal · up to No merge-blocking issue was identified in the relinked prototype behavior. The separately acknowledged patched-method Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected paths preserve callable checks and receiver identity while correcting stale inherited-method dispatch. No introduced security-control bypass was established. Confidence remains limited by incomplete coverage of concurrent access and adjacent prototype-sensitive operations. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation Issue [ Full details: Out of Scope Changes checkExplanation The runtime, fixture, and test changes support [ Full details: Docstring CoverageExplanation Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 17 files. (2 skipped: 2 unsupported.)
✨ 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 |
proggeramlug
added
the
run-extended-tests
1 participant
After
Object.setPrototypeOf(C.prototype, X), methods, getters and setters from C's old declared parent remained visible through several runtime and compiler dispatch paths. Reads, membership checks, calls andsuper.m()now follow the replacement instance chain, while C's own members and constructor-side static inheritance remain available. Wide dispatch towers also stop running a stale parent body after prototype redefinition.Current head:
6a61c2ceff67f5eebc347ae70ca9a54afc13e0c0, refreshed against main420721032422900c04797dd6ca59155f54eb1295.The refresh additionally fixes a reproduced defect in the initial PR: a getter on the replacement super chain returning a callable Proxy was rejected as non-callable. The original
2687543product fails the eight-case fixture withTypeError: m is not a function; the repaired product matches Node 26.5.1 for callable, default, nested and revoked function proxies, ordinary functions, non-callable values, object-target proxies with apply traps, and getter exceptions. Proxy calls use the existing rooted proxy dispatcher. No published ABI signatures change.Runtime declared-member walks stop at a user-relinked class prototype. Per-name invalidation bytes retire compiler-resolved inherited bodies, including wide towers and direct super calls; existing cache generations retire dispatch caches. The super fallback roots its receiver and arguments across key allocation and getter calls, then refreshes them before calling the returned value. The current-main refresh preserves all GC helper tables and root-holder inventories and routes the landed immutable-global test suite in scoped CI.
Validation at the current head on Linux, Rust nightly 2026-08-20 and LLVM 22.1.8:
Full local script lint completed successfully: all 111 gates passed; the compile tier and two CI-only commands were explicitly skipped. The release product and targeted integration tests above ran separately. After lint, the compiler and both static wrappers were rebuilt together at the same current head and frozen with verified SHA256 hashes; the source tree was clean. Fresh hosted PR acceptance remains required; both whole shadow/native root-dominance jobs have now succeeded as recorded below; this body does not claim merge readiness. The earlier cfa candidate's 110-gate result remains historical; the 111-gate result above is from this current head.
The original class fixture checks primed/hit read sites but uses an obsolete poison variable and does not assert actual moving collection. Its passing result alone is not moving-GC evidence; the separate positive controls above supply that evidence. The author's 42-cell instruction-cost comparison is historical and has not been independently repeated at this head.
Separate remaining defects:
instanceofstill follows declared ancestry; super property reads remain statically resolved; prototype__proto__assignment is not repaired here; assigned prototype methods in the unrelinked super fallback require separate work. This PR does not close #11760.Current-head hosted evidence: the whole shadow root-dominance job 110888864845 succeeded; the whole native statepoints job 110888865324 also completed successfully. Both required whole jobs passed at this exact current head. The GC ratchet job 110888865025 failed only its pinned-baseline check: all 73 failed numeric rows exactly match the independently completed main job 110380943914, including values and allowances. This attributes that failure; it does not make the failed job green.
Other current-head failures remain recorded: the five backend IR assertions match the independently completed main job; macOS native-root linking lacks eight CF/Objective-C provider symbols; Windows retains the old try/catch refusal tripwire; moving-witness CI reports the same five zero-movement cells as main. The provider/tripwire repairs are included in #11756 and require current integrated four-target acceptance. The witness harness repair is undergoing a separate native comparison. These dependencies and remaining hosted checks prevent merge approval.
Summary by CodeRabbit
superlookups consistently, including when the replacement chain contains callable proxies.TypeErrorwhen the resolved value is not callable.