fix(runtime): #5844 — Proxy trap dispatch fixes across getOwnPropertyDescriptor, has, set, and setPrototypeOf#5882
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds Proxy support across object prototype operations: Object.create, Object.create_with_props, and Object.setPrototypeOf now accept Proxy values as prototypes; HasProperty prototype-chain walks dispatch to Proxy ChangesProxy-aware object operations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReflectSet as js_reflect_set
participant ProxySet as proxy_set_with_receiver
participant Handler as set trap
participant Target
Caller->>ReflectSet: Reflect.set(target, key, value, receiver)
ReflectSet->>ProxySet: proxy_set_with_receiver(target, key, value, receiver)
ProxySet->>Handler: invoke set trap(target, key, value, receiver)
alt trap exists
Handler-->>ProxySet: truthy/falsy result
else no trap
ProxySet->>Target: reflect_ordinary_set_with_receiver(target, key, value, receiver)
Target-->>ProxySet: result
end
ProxySet-->>ReflectSet: boolean result
sequenceDiagram
participant Caller
participant HasProperty as ordinary_has_property
participant Proxy as js_proxy_has
Caller->>HasProperty: key in object
HasProperty->>HasProperty: walk prototype chain
HasProperty->>Proxy: dispatch js_proxy_has(proxy, key)
Proxy-->>HasProperty: boolean result
HasProperty-->>Caller: truthiness result
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 |
…Descriptor, has, set, and setPrototypeOf
Fixes 12 of 33 test262 built-ins/Proxy failures, zero regressions
(verified against built-ins/Proxy, built-ins/Object, built-ins/Reflect,
language/expressions/in — 219/219/3132/101/15 pass respectively).
Root causes, all variations on "a Proxy is a small registered id, not a
real heap pointer, and several generic object-machinery code paths
either mis-treated it as one or never checked for it at all":
- js_reflect_get_own_property_descriptor: the missing-trap fallback read
the target's descriptor via the raw ObjectHeader path even when the
target was itself a Proxy; now recurses through the Reflect entry
point so a chain of trap-less proxies forwards correctly.
- ordinary_has_property (the `in` operator's prototype-chain walk) and
its array-fast-path counterpart didn't recognize a Proxy sitting in
the recorded `[[Prototype]]` chain, silently returning false (or
reading garbage) instead of dispatching the proxy's `has` trap.
- ordinary_set_with_receiver's prototype-chain walk had the same gap on
the write side, and `js_proxy_set`/`Reflect.set` didn't thread a
distinct `receiver` through to the trap when a Proxy was reached via
a prototype hop rather than as the direct assignment target.
- Object.create(proto) and Object.setPrototypeOf(obj, proto) rejected a
Proxy `proto` argument outright (their "is this an object" checks
don't recognize the small registered id), and Object.create had no
path to record a Proxy prototype at all — it can't be modeled via the
synthetic-class-id machinery used for real prototype objects, so route
it through the same static-prototype side table setPrototypeOf uses.
- Object.setPrototypeOf's cycle-detection walk called
`[[GetPrototypeOf]]` unconditionally while probing the candidate
prototype's chain, including on a Proxy — invoking its trap as an
unrelated side effect of cycle-safety bookkeeping (OrdinarySetPrototypeOf
step 7.b.ii.1 says to stop the walk there instead).
Also fixed, found while debugging the above with a fresh (Proxy-free)
repro: the cycle-detection loop's Floyd's tortoise-and-hare walk only
guarded `tortoise` against an already-null position before advancing;
`hare` (which steps twice per iteration) had no equivalent guard, so a
2-hop-to-null prototype chain re-advanced an already-null `hare` and
threw "Cannot convert undefined or null to object" on a plain, entirely
ordinary `Object.setPrototypeOf({}, {foo: 1})`.
Refs #5844.
proggeramlug
force-pushed
the
fix/5844-proxy-invariants
branch
from
July 3, 2026 06:25
223e32b to
672efab
Compare
…xy/put_value.rs proxy.rs grew to 2004 lines after rebasing onto main (which independently added ~30 lines), tripping the 2000-line file-size CI gate. Relocate the Proxy [[Set]] entry points into the sibling put_value.rs submodule (already the natural home for PutValue/[[Set]] dispatch) rather than trim comments to fit — same pattern as the file's other topical splits. Pure code motion, no behavior change (re-verified: built-ins/Proxy still 219 pass / 21 runtime-fail).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/proxy/put_value.rs`:
- Around line 8-11: Update the stale doc comment in proxy_set_with_receiver to
match the current proxy set semantics: the set trap now receives (target, key,
value, receiver), and the function returns the trap’s coerced boolean result
instead of always echoing value or TAG_TRUE. Keep the comment aligned with the
behavior in put_value.rs so it reflects the actual proxy path and no longer
describes the pre-#2756 implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7cab4e0-06b9-4420-8fd3-6777c38865ae
📒 Files selected for processing (7)
crates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/field_get_set/has_property.rscrates/perry-runtime/src/object/object_ops/define_properties.rscrates/perry-runtime/src/object/object_ops/prototype.rscrates/perry-runtime/src/proxy.rscrates/perry-runtime/src/proxy/put_value.rscrates/perry-runtime/src/proxy/reflect.rs
Comment on lines
+8
to
+11
| /// `proxy[key] = value` — if handler.set exists, call it with | ||
| /// (target, key, value) and return TAG_TRUE (the trap's return value is | ||
| /// ignored by the default test semantics since we echo `value`). Otherwise | ||
| /// forward to the target directly. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment contradicts the new behavior.
This comment predates #2756. proxy_set_with_receiver now passes receiver to the trap ((target, key, value, receiver)) and returns the trap's coerced boolean rather than always echoing value/TAG_TRUE. Update the doc to avoid misleading maintainers.
📝 Suggested doc update
-/// `proxy[key] = value` — if handler.set exists, call it with
-/// (target, key, value) and return TAG_TRUE (the trap's return value is
-/// ignored by the default test semantics since we echo `value`). Otherwise
-/// forward to the target directly.
+/// `proxy[key] = value` with the proxy itself as `Receiver`. Delegates to
+/// `proxy_set_with_receiver`, which invokes the `set` trap with
+/// `(target, key, value, receiver)` and returns its coerced boolean result
+/// (`#2756`), or forwards to the target's `[[Set]]` when no trap exists.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// `proxy[key] = value` — if handler.set exists, call it with | |
| /// (target, key, value) and return TAG_TRUE (the trap's return value is | |
| /// ignored by the default test semantics since we echo `value`). Otherwise | |
| /// forward to the target directly. | |
| /// `proxy[key] = value` with the proxy itself as `Receiver`. Delegates to | |
| /// `proxy_set_with_receiver`, which invokes the `set` trap with | |
| /// `(target, key, value, receiver)` and returns its coerced boolean result | |
| /// (`#2756`), or forwards to the target's `[[Set]]` when no trap exists. |
🤖 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/proxy/put_value.rs` around lines 8 - 11, Update the
stale doc comment in proxy_set_with_receiver to match the current proxy set
semantics: the set trap now receives (target, key, value, receiver), and the
function returns the trap’s coerced boolean result instead of always echoing
value or TAG_TRUE. Keep the comment aligned with the behavior in put_value.rs so
it reflects the actual proxy path and no longer describes the pre-#2756
implementation.
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 12 of the 33 test262
built-ins/Proxyfailures listed in #5844, with zero regressions.Root causes, all variations on "a Proxy is a small registered id, not a real heap pointer, and several generic object-machinery code paths either mis-treated it as one or never checked for it at all":
js_reflect_get_own_property_descriptor: the missing-trap fallback read the target's descriptor via the rawObjectHeaderpath even when the target was itself a Proxy; now recurses through theReflectentry point so a chain of trap-less proxies forwards correctly.ordinary_has_property(theinoperator's prototype-chain walk) and its array-fast-path counterpart didn't recognize a Proxy sitting in the recorded[[Prototype]]chain, silently returningfalse(or reading garbage) instead of dispatching the proxy'shastrap.ordinary_set_with_receiver's prototype-chain walk had the same gap on the write side, andjs_proxy_set/Reflect.setdidn't thread a distinctreceiverthrough to the trap when a Proxy was reached via a prototype hop rather than as the direct assignment target.Object.create(proto)andObject.setPrototypeOf(obj, proto)rejected a Proxyprotoargument outright, andObject.createhad no path to record a Proxy prototype at all (it can't be modeled via the synthetic-class-id machinery used for real prototype objects) — routed it through the same static-prototype side tablesetPrototypeOfuses.Object.setPrototypeOf's cycle-detection walk called[[GetPrototypeOf]]unconditionally while probing the candidate prototype's chain, including on a Proxy — invoking its trap as an unrelated side effect of cycle-safety bookkeeping (OrdinarySetPrototypeOfstep 7.b.ii.1 says to stop the walk there instead).Also fixed (found while debugging the above with a fresh, Proxy-free repro): the cycle-detection loop's Floyd's tortoise-and-hare walk only guarded
tortoiseagainst an already-null position before advancing;hare(which steps twice per iteration) had no equivalent guard, so a 2-hop-to-null prototype chain re-advanced an already-nullhareand threw"Cannot convert undefined or null to object"on a plain, entirely ordinaryObject.setPrototypeOf({}, {foo: 1}).Cases now passing:
has/call-in-prototype.js,has/call-in-prototype-index.js,has/call-object-create.js,revocable/length.js,revocable/name.js,set/call-parameters-prototype.js,set/trap-is-missing-receiver-multiple-calls.js,set/trap-is-missing-receiver-multiple-calls-index.js,set/trap-is-null-receiver.js,set/trap-is-undefined-target-is-proxy.js,setPrototypeOf/call-parameters.js,defineProperty/targetdesc-not-compatible-descriptor-not-configurable-target.js.Remaining 21 failures are distinct root causes (String/Function exotic-descriptor gaps,
instanceof's class-id-chain model not walking a Proxy'sgetPrototypeOftrap, array-element-write fast path bypassing the prototype-chain proxy dispatch,Proxyconstructor.prototype/newTargetchecks, etc.) — left for follow-up per the issue's "pick a coherent subcluster" guidance.Code-only PR per issue instructions — no
Cargo.toml/CLAUDE.mdversion bump, noCHANGELOG.mdentry.Test plan
scripts/test262_subset.py --dir built-ins/Proxy: 219 pass / 21 runtime-fail / 0 diff (was 207/33 before) — all 21 remaining failures were already in the original 33-item list (zero regressions within Proxy)scripts/test262_subset.py --dir built-ins/Object: 3132 pass / 41 runtime-fail (all pre-existing categorical gaps unrelated to this change)scripts/test262_subset.py --dir built-ins/Reflect: 101 pass / 1 runtime-fail (pre-existing, unrelated)scripts/test262_subset.py --dir language/expressions/in: 15 pass / 2 runtime-fail (pre-existing, unrelated to primitives, not Proxy/prototype-chain)cargo fmt --all -- --checkbash scripts/check_file_size.sh🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Reflect.setand related proxy behavior now preserve the correct receiver during nested proxy handling.Bug Fixes
in,Object.create,Object.setPrototypeOf, andReflectoperations.