fix(runtime): give WeakRef/FinalizationRegistry a real method surface, and brand-check every folded weak intrinsic - #7953
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe runtime adds branded dispatch and prototype methods for ChangesWeak wrapper runtime behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MethodCall
participant WeakWrapperDispatch
participant BrandCheck
participant WeakWrapperStorage
participant ForeignReceiverMethod
MethodCall->>WeakWrapperDispatch: resolve method and receiver
WeakWrapperDispatch->>BrandCheck: identify receiver brand
alt valid weak wrapper
BrandCheck-->>WeakWrapperDispatch: matching wrapper brand
WeakWrapperDispatch->>WeakWrapperStorage: execute intrinsic operation
else foreign receiver
BrandCheck-->>WeakWrapperDispatch: foreign receiver
WeakWrapperDispatch->>ForeignReceiverMethod: ordinary dynamic dispatch
end
✨ 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 |
…, and brand-check every folded weak intrinsic `WeakRef.prototype.deref` and `FinalizationRegistry.prototype.register`/ `.unregister` had no runtime existence at all — they were purely an HIR fold keyed on a bare local NAME recorded by `pre_scan_weakref_locals`, which walks module statements and function *declarations* only. Every other receiver shape threw `TypeError: deref is not a function`: an array element, an object property, a call result, a `for…of` binding, a function parameter, a `.map` callback, and any binding inside an arrow function, function expression or class method. Two of twenty measured receiver shapes worked. Three additions close it: `try_weak_method_dispatch` gains `CLASS_ID_WEAKREF`/`CLASS_ID_FINALIZATION_REGISTRY` arms; brand-checking prototype thunks are installed on both prototypes; and the by-name value read resolves those thunks so `typeof wr.deref === "function"` and `wr.deref.bind(wr)` work. `Object.prototype.toString` gains the two tags. The same investigation turned up a silent wrong answer (#7948): the fold is scope-blind and its helpers did not brand-check, so one genuine `const r = new WeakRef(x)` anywhere in a module folded EVERY same-named `r.deref()` onto `js_weakref_deref`, which answered `undefined` for a user class instance, an object literal, an array or a parameter — exit code 0. The weak collections had the same hole through initializer shapes the ambiguity poison pass cannot see (`get`/`set`/`has`/`add`/`delete`). All nine folded helpers now brand-check their receiver and hand a foreign one back to ordinary dynamic dispatch, so a mis-fold degrades to the correct slow path. Closes #7947. Closes #7948.
…hod surface + weak-intrinsic brand checks)
…t only the gap suite The gap suite is tag-gated; `cargo-test` is a required per-PR context. Three unit tests pin the two halves: a foreign receiver carrying its OWN deref/get/ has/delete/add must see that method run (asserted by sentinel return value, so the test cannot pass on the pre-fix `undefined`), a genuine wrapper must still take the intrinsic path, and WeakRef.deref must be reachable through try_weak_method_dispatch while a non-own method still falls through to None.
d841be9 to
8f6ece7
Compare
Validation recordRebased onto Correctness
Sabotage verification (fix + tests committed first)Both arms applied at once; they attribute cleanly because the tests assert independent things.
The first row is the load-bearing one: it asserts a foreign receiver's own method actually runs (sentinel return value), so it cannot pass on the pre-fix An earlier end-to-end arm caught it too — with the dispatch arm disabled the gap test dropped from 48 correct lines to an immediate throw, exit 1. These live in Gap suite: all 8 flagged "regressions" are byte-identical on both arms550 tests on macOS: 528 pass / 16 parity_fail / 6 crash. The snapshot gate flagged 8 regressions. A/B'd against a pristine build of the merge-base — none is mine:
All six crashes abort at the same site — On the baseline choice: the A/B baseline is my merge-base |
1 participant
Closes #7947. Closes #7948.
The measured blast radius
WeakRef.prototype.derefworked in 2 of 20 receiver shapes. The issue'sframing ("array element") was one symptom; its claim that
const r = arr[0]; r.deref()works is wrong — that throws too.derefbeforeFinReg.registerbeforeconst x = new X(…)at module top levelconst x = new X(…)in a function DECLARATIONconst x = new X(…)in an ARROW functionconst x = new X(…)in a function EXPRESSIONconst x = new X(…)in a CLASS METHODarr[0].m()o.r.m()mk().m()for (const x of arr)binding.map((x) => x.m())callbacknew X(…).m()inlineMapvaluem.get(k).m()X.prototype.m.call(x)x.m.bind(x)typeof x.mundefinedx.m?.()undefinedWeakMap/WeakSetwere ✅ in every one of those rows before and after — theasymmetry is the whole story.
Root cause
deref/register/unregisterhad no runtime existence at all. They werepurely an HIR fold:
pre_scan_weakref_localsrecords bare local NAMES bound tolet/const x = new WeakRef(…)— walking module statements and the bodies offunction declarations only — and
expr_call/url_date_instance.rsfolds<tracked-name>.deref()toExpr::WeakRefDeref.Anything the fold could not name fell through to dynamic dispatch, where
nothing resolved:
try_weak_method_dispatchearly-returned unless the receiver'sclass_idwas
CLASS_ID_WEAKMAP/CLASS_ID_WEAKSET;install_collection_proto_methodshad noWeakRef/FinalizationRegistryarm, so
WeakRef.prototypecarried noderefproperty at all;get_field_by_name.rshad a WeakMap/WeakSet armbut no wrapper arm.
weakref_locals.rsnames the asymmetry in a comment, as the reason those setsare exempt from the ambiguity poison pass:
★ The silent half (#7948)
The fold is name-keyed and scope-blind, and its helpers did not brand-check.
One genuine
const r = new WeakRef(x)anywhere in a module folded everyr.deref()in that module ontojs_weakref_deref, which read__perry_wr_targetby name off whatever it was handed and answeredundefined. Exit code 0, no diagnostic:weakmap_locals/weakset_locals/proxy_localsare poisoned, but the poisonpass only recognises
new <OtherClass>()and call/await initializers — itcannot see an object literal, an array, or a parameter, so the identical hijack
went through on the far more common
get/set/has/add/delete.Name poisoning can only ever be a partial patch: the pre-scan cannot enumerate
every way a name acquires a non-intrinsic value (parameters and destructuring
bindings are not even declarations it visits). The durable fix is on the other
side — the intrinsic brand-checks its receiver.
What changed
New module
crates/perry-runtime/src/object/weakref_proto_thunks.rs.try_weak_method_dispatch+weak_class_id_from_receivermoved there fromweakref.rs(which was at 1988/2000 lines) as a pure move, then extended.try_weak_method_dispatchgains("deref", CLASS_ID_WEAKREF)and("register" | "unregister", CLASS_ID_FINALIZATION_REGISTRY).WeakRef.prototype.deref(arity 0),FinalizationRegistry.prototype.register(2) /.unregister(1), hookedinto
populate_builtin_prototype_methods. Gives the reflective path,.length, and the specTypeErrorforX.prototype.m.call({}).get_field_by_name.rsresolves those thunk values foran instance read, so
typeof wr.deref === "function"andwr.deref === WeakRef.prototype.deref.js_weakref_deref,js_finreg_register,js_finreg_unregister,js_weakmap_{set,get,has,delete}andjs_weakset_addverify the receiver's reservedclass_idfirst and hand aforeign one to
dispatch_foreign_weak_receiver, which re-entersjs_native_call_method. A mis-fold now degrades to the correct slow pathinstead of a wrong answer. Recursion is impossible:
js_native_call_methodroutes back into these helpers only via
try_weak_method_dispatch, whichrequires the reserved
class_idthe brand check just rejected.Object.prototype.toString—to_string_tag.rsgained the two missingarms (
[object Object]→[object WeakRef]/[object FinalizationRegistry]).Deliberately NOT fixed
class M extends WeakMap {}and the WeakSet /WeakRef / FinalizationRegistry equivalents throw before and after this
change (
value is not a function;Constructor WeakRef requires 'new').Verified identical against a pristine
origin/mainbinary, so the new brandchecks are not the cause. Different mechanism (constructor/prototype
reification —
map_set_subclassexists only for Map/Set), out of scope. Thegap test's header names this as the pinned boundary so nobody reads a green
run as coverage.
not descend into arrow bodies. That is now a performance property rather
than a correctness one: an unnamed receiver takes the dynamic path, and a
mis-named one brand-checks its way back to the right method. Making the scan
scope-aware is the real cleanup and belongs on its own.
Validation
test-files/test_gap_weakref_receiver_shapes_7947.ts— 48 output lines,byte-exact vs node 26.5.1, exit 0. All 14 previously-throwing receiver
shapes, the reflective/value-read path, both
toStringtags, the brandcheck, FinalizationRegistry through three shapes, the six bug(hir): name-keyed weak/proxy intrinsic folds SILENTLY hijack same-named foreign receivers (deref/get/set/has → undefined) #7948
WeakRef/FinReg collision cells and the five WeakMap/WeakSet ones, plus the
WeakMap/WeakSet shapes that already worked (so a refactor of the shared
dispatch cannot silently drop them).
cargo test --release -p perry-runtime: 2211 passed, 0 failed.origin/mainbuild.Summary by CodeRabbit
WeakRefandFinalizationRegistrymethods across direct, reflective, prototype, and value-based access.