fix(class): delete prototype method removes its keys_array field (#5441)#5443
Conversation
PR #5395 (#5395) made `delete C.prototype.<member>` on a class-declaration prototype mark the vtable key deleted and early-return, so hasOwnProperty / getOwnPropertyDescriptor would agree the member is gone. That is correct for instance ACCESSORS (`get x()`), which live only in the class vtable — there is no keys_array entry to remove. Instance METHODS are different: `install_class_decl_prototype_method_fields` plants each method as a REAL keys_array data field on the prototype object (writable:true, enumerable:false, configurable:true). For a method the early return marked the vtable key deleted but left the real field in place, so hasOwnProperty kept finding it via `own_key_present`. test262's `verifyProperty` configurable probe (`delete obj[name]` then assert the key absent) therefore failed with "m descriptor should be configurable" — ~760 generated language/{statements,expressions}/class tests, which dropped language parity 94.5% → 89.3%. Fix: drop the early return so the class-prototype delete falls through to the keys_array scan. For accessors the scan is a vacuous success (no entry); for methods it removes the real field. The vtable key is still marked deleted, so both member kinds become invisible to the reflective paths. Validation (test262 radar, node v26): - language/statements/class: parity 94.0%, 0 "descriptor should be configurable" - language/expressions/class: parity 95.6%, 0 "descriptor should be configurable" - accessor prop-desc tests (#5395's targets) still pass; no regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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)
📝 WalkthroughWalkthroughIn ChangesClass Prototype Field Deletion Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
Fixes #5441 — the ~760 generated
language/{statements,expressions}/classtest262 cases that regressed toUncaught exception: m descriptor should be configurable(language parity 94.5% → 89.3%).Root cause
The regression came from #5395. That PR made
delete C.prototype.<member>on a class-declaration prototype mark the vtable key deleted and early-return, sohasOwnProperty/getOwnPropertyDescriptorwould agree the member is gone.That is correct for instance accessors (
get x()) — they live only in the class vtable, with nokeys_arrayentry to remove (the cases #5395 actually tested).Instance methods are different:
install_class_decl_prototype_method_fieldsplants each method as a realkeys_arraydata field on the prototype object (writable:true, enumerable:false, configurable:true). For a method the early return marked the vtable key deleted but left the real field in place, sohasOwnPropertykept finding it viaown_key_present. test262'sverifyPropertyconfigurable probe (delete obj[name]then assert the key absent) therefore failed withm descriptor should be configurable.This is why isolated
getOwnPropertyDescriptorlooked fine but the fullverifyPropertypath (used by theflags: [generated]class tests) failed.Fix
Drop the early
return 1so the class-prototype delete falls through to the existingkeys_arrayscan:One-file change in
crates/perry-runtime/src/object/delete_rest.rs.Validation (test262 radar, node v26)
descriptor should be configurablefailureslanguage/statements/classlanguage/expressions/classnew-no-sc-line-method-rs-privatename-identifier-alt.js, which also checkswritable:true) now passes.definition/{getters,setters}-prop-desctests (fix(class): super-in-static resolution + accessor .name reflection (#5345) #5395's targets) still pass — no regression.cargo test -p perry-runtime: 1063 passed; the 2 failures are pre-existing/environmental (the known stream test-isolation flake and a cwd-dependent URL test that trips inside a worktree path), not related to this change.Class-tail tracker: #5345.
🤖 Generated with Claude Code
Summary by CodeRabbit