fix(runtime): #5834 — WeakMap/WeakSet constructor + instance-identity test262 fixes - #5851
Conversation
… test262 fixes Fixes the built-ins/WeakMap (11) and built-ins/WeakSet (7) test262 failures from the #5834 worklist: - WeakMap/WeakSet constructors never read/called the `set`/`add` adder through a real property Get, and drained the whole iterable eagerly instead of stepping it lazily. Rewrote both to mirror `js_map_from_iterable`/`js_set_from_iterable`: fetch the adder only when `iterable` is non-null/undefined (matching the spec's step order so a poisoned accessor doesn't fire for `new WeakMap()`), honor an accessor descriptor on `set`/`add` via a new `builtin_prototype_adder` helper, and drive the iterable with lazy per-item stepping so an abrupt Get/adder call closes the iterator before rethrowing. - `try_weak_method_dispatch`'s arity-gated arms skipped validation for under-arity calls (e.g. `s.add()`), so `WeakSet.prototype.add` never ran its CanBeHeldWeakly check on a missing argument. Now always dispatches, padding missing args with `undefined`. - `Object.getPrototypeOf`/`.constructor` had no arm for WeakMap/WeakSet instances (a reserved, non-declared-class `class_id`), so both fell through to the receiver itself / `undefined`. - `x instanceof WeakMap`/`WeakSet` had no runtime probe for real instances (documented as a known gap) — added one, plus the dynamic `x instanceof ctorVar` counterpart. Verified via scripts/test262_subset.py: built-ins/WeakMap and built-ins/WeakSet now both at 100% parity (0 regressions). No version bump/CHANGELOG per the #5834 worklist's code-only-PR instructions. Refs #5834.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRuntime changes add reserved class-id handling for WeakMap/WeakSet across instanceof checks, constructor property resolution, prototype lookup, method dispatch, and iterable initialization. A new builtin_prototype_adder helper resolves accessor properties, and codegen comments are clarified. ChangesWeakMap/WeakSet reserved-id handling
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/weakref.rs (1)
802-834: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate weak-method dispatch by receiver class
crates/perry-runtime/src/weakref.rs:802-833—try_weak_method_dispatchstill routes only onmethod_name, so aWeakMapreceiver can hit"add"/aWeakSetreceiver can hit"set"etc. and call the wrong helper instead of throwing the incompatible-receiverTypeError. Add a class/method pairing check before dispatching.🤖 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-runtime/src/weakref.rs` around lines 802 - 834, `try_weak_method_dispatch` is dispatching solely by `method_name`, which can let a WeakMap receiver reach WeakSet helpers (and vice versa) instead of rejecting incompatible calls. Update the dispatch logic in `try_weak_method_dispatch` to verify the receiver’s `class_id` matches the requested weak method before calling `js_weakmap_set`, `js_weakset_add`, `js_weakmap_get`, `js_weakmap_has`, or `js_weakmap_delete`, and return `None` for mismatched class/method pairs so the normal incompatible-receiver `TypeError` path is preserved.
🤖 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-runtime/src/weakref.rs`:
- Around line 802-834: `try_weak_method_dispatch` is dispatching solely by
`method_name`, which can let a WeakMap receiver reach WeakSet helpers (and vice
versa) instead of rejecting incompatible calls. Update the dispatch logic in
`try_weak_method_dispatch` to verify the receiver’s `class_id` matches the
requested weak method before calling `js_weakmap_set`, `js_weakset_add`,
`js_weakmap_get`, `js_weakmap_has`, or `js_weakmap_delete`, and return `None`
for mismatched class/method pairs so the normal incompatible-receiver
`TypeError` path is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1fb4d93-1f66-453a-9acb-51a6078616f5
📒 Files selected for processing (6)
crates/perry-codegen/src/expr/instance_misc1.rscrates/perry-runtime/src/collection_iter.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/object_ops/prototype.rscrates/perry-runtime/src/weakref.rs
Per CodeRabbit review on PR #5851: try_weak_method_dispatch routed purely on method_name, so a WeakMap receiver could reach "add" (WeakSet-only) and vice versa, instead of falling through to the ordinary property lookup that correctly resolves the missing method and throws TypeError: ... is not a function. Match on (method_name, class_id) so "set"/"get" require CLASS_ID_WEAKMAP and "add" requires CLASS_ID_WEAKSET; "has"/"delete" stay shared. Any other pairing (including unknown method names) now returns None instead of a silent undefined, letting the caller fall through to normal dispatch. Refs #5834.
Summary
Fixes the
built-ins/WeakMap(11) andbuilt-ins/WeakSet(7) test262 failures from the #5834 worklist — a coherent single-root subcluster: WeakMap/WeakSet constructor semantics and instance identity.js_weakmap_init_iterable/js_weakset_init_iterablepreviously drained the whole iterable eagerly via a native helper and never actually read/called theset/addadder through a real propertyGet. Rewrote both to mirrorjs_map_from_iterable/js_set_from_iterable: fetch the adder only wheniterableis non-null/undefined (matching spec step order, so a poisonedWeakMap.prototype.setaccessor getter must not fire fornew WeakMap()/new WeakMap(null)), honor an accessor descriptor onset/addvia a newcollection_iter::builtin_prototype_adderhelper, and drive the iterable with lazy per-item stepping (iterator_next_value) so an abruptGet/adder-call closes the iterator (IteratorClose) before rethrowing.try_weak_method_dispatch's"add"/"set"/etc. arms were gated onargs.len(), so an under-arity call likes.add()fell through to a no-op instead of runningWeakSet.prototype.add'sCanBeHeldWeaklycheck (which must throwTypeErrorforundefined). Now always dispatches, padding missing positions withundefined.Object.getPrototypeOf(new WeakMap())and(new WeakMap()).constructorhad no arm for WeakMap/WeakSet instances (a reserved, non-declared-classclass_id), so both fell through to the receiver itself /undefined. Added arms inprototype.rs'scollection_prototypeclosure andget_field_by_name_tail.rs'sconstructorspecial case.instanceof:x instanceof WeakMap/WeakSethad no runtime probe for real instances — this was a documented, deliberate gap inperry-codegen/src/expr/instance_misc1.rs("no runtime probe for real instances yet"). Added a probe injs_instanceof(compile-time-literal path) plus theglobal_builtin_constructor_class_idcounterpart for the dynamicx instanceof ctorVarform.Test plan
scripts/test262_subset.py --dir built-ins/WeakMap built-ins/WeakSet: 18/18 originally-failing cases now pass; 100% parity (158/158 judged), 0 regressions.cargo fmt --all -- --checkclean.bash scripts/check_file_size.shclean.cargo test --release -p perry-runtime -p perry-codegen: 1107 passed / 1 failed. The 1 failure (object::tests::builtin_prototype_methods_reject_dynamic_new, assertingDate.prototype.toJSON) is unrelated to this change (no Date/global_this code touched) and matches a known pre-existing test-isolation flake in this suite (shared global builtin-population state races under parallel test threads).No version bump / CHANGELOG update, per the #5834 worklist's code-only-PR instructions.
Refs #5834.
🤖 Generated with Claude Code
Summary by CodeRabbit
WeakMap/WeakSethandling forconstructor,prototype, andinstanceofchecks, including correct resolution for internally-reserved class-id cases.getPrototypeOfbehavior sogetPrototypeOf(new WeakMap())/getPrototypeOf(new WeakSet())no longer falls back to returning the receiver itself.WeakMap/WeakSetmethod dispatch and iterable initialization for missing arguments, lazy consumption, and proper iterator cleanup on abrupt failures.