fix(proxy): #5905 — forward setPrototypeOf/deleteProperty through proxy targets with correct throw semantics#5967
Conversation
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChanges add proxy has/delete handling, strict-mode-aware proxy delete lowering, delete guards for Array length and function prototype, and TypeError propagation for failed Proxy ChangesProxy and delete semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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.
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.rs`:
- Around line 819-831: The prototype-delete guard in the proxy/object delete
paths still treats async functions like plain functions, so `delete
fn.prototype` can incorrectly return false. Update the logic in `proxy.rs` and
mirror the same change in `object/delete_rest.rs` so the `prototype`
special-case also excludes async functions alongside the existing
`closure_is_arrow` and `closure_is_bound_method` checks, using the relevant
closure inspection helpers around the `ClosureHeader` path.
🪄 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: 728ad0a7-fc25-4927-a249-cba6ee3e3336
📒 Files selected for processing (4)
crates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/object/object_ops/define_properties.rscrates/perry-runtime/src/proxy.rs
…xy targets with correct throw semantics
When a Proxy's target is itself a Proxy and the outer trap is missing/undefined/null,
the operation forwards down the chain. Several forwarding paths dropped the spec's
throw-on-false behavior or mis-reported non-configurable exotic properties as deletable:
- Object.setPrototypeOf(proxy, x) discarded the internal [[SetPrototypeOf]] boolean;
a false result (non-extensible target reached through the chain) must throw a
TypeError (Reflect.setPrototypeOf keeps returning the boolean). Fixes
setPrototypeOf/trap-is-{missing,undefined}-target-is-proxy.
- Reflect.deleteProperty / delete never rejected an Array's non-configurable
`length` or a plain function's non-configurable `prototype` — both are exotic
own properties with no descriptor-table entry, so the delete silently reported
success. Reject them in reflect_ordinary_delete_property_key and js_object_delete_field.
- delete proxy.key is lowered to the ProxyDelete HIR node, whose codegen returned
js_proxy_delete's boolean directly and ignored strict mode; a strict-mode delete
that resolves to false must throw. Route it through js_delete_result like the
ordinary member-delete path. Fixes deleteProperty/trap-is-{missing,null,undefined}-target-is-proxy.
built-ins/Proxy 218/240 -> 223/240; no regressions in Reflect/Object/delete slices.
174f531 to
7d52ea4
Compare
|
Rebased on main and split |
Part of #5905 (built-ins/Proxy worklist).
Root cause
When a Proxy's target is itself a Proxy and the outer trap is missing/undefined/null, the operation forwards down the chain. Several forwarding paths dropped the spec's throw-on-false behavior or mis-reported non-configurable exotic properties as deletable.
Fixes
Object.setPrototypeOf(proxy, x)discarded the internal[[SetPrototypeOf]]boolean. Afalseresult (a non-extensible target reached through the chain, or an inner trap returningfalse) must throw aTypeError, whileReflect.setPrototypeOfkeeps returning the boolean. (crates/perry-runtime/src/object/object_ops/define_properties.rs)Reflect.deleteProperty/deletenever rejected an Array's non-configurablelengthor a plain (non-arrow, non-bound) function's non-configurableprototype. Both are exotic own properties with no descriptor-table entry, soobj_value_attrs/get_property_attrsreturnedNoneand the delete silently reported success. Now rejected inreflect_ordinary_delete_property_keyandjs_object_delete_field. (crates/perry-runtime/src/proxy.rs,crates/perry-runtime/src/object/delete_rest.rs)delete proxy.keylowers to theProxyDeleteHIR node, whose codegen returnedjs_proxy_delete's boolean directly and ignored strict mode. A strict-mode delete that resolves tofalsemust throw, exactly like the ordinary member-delete path — now routed throughjs_delete_resultwith the strict flag. (crates/perry-codegen/src/expr/proxy_reflect.rs)Before/after
built-ins/Proxy: 218/240 → 223/240 (parity 90.8% → 92.9%).Newly passing (5):
setPrototypeOf/trap-is-missing-target-is-proxy.jssetPrototypeOf/trap-is-undefined-target-is-proxy.jsdeleteProperty/trap-is-missing-target-is-proxy.jsdeleteProperty/trap-is-null-target-is-proxy.jsdeleteProperty/trap-is-undefined-target-is-proxy.jsZero regressions: verified the
built-ins/Proxyslice (no new failures) plusbuilt-ins/Reflect,built-ins/Object{,/defineProperty,/getPrototypeOf,/setPrototypeOf}, andlanguage/expressions/delete(all unchanged).cargo fmt --all -- --checkandscripts/check_file_size.shpass.Summary by CodeRabbit
deleteto correctly report failure (and thus strict-mode throwing) for special non-configurable properties like Arraylengthand functionprototype.delete, rather than incorrectly succeeding.Object.setPrototypeOfto fail reliably when the operation is not permitted (instead of appearing to succeed).