fix(class): super-in-static resolution + accessor .name reflection (#5345)#5395
Merged
Conversation
…5345) Two ECMAScript class-semantics fixes for the test262 class tail. 1. super in a STATIC method (super/in-static-{getter,methods,setter}, +3): `super.x` and `super.m()` inside a `static` method must resolve against the PARENT's static side (its static getter / static method on the constructor), not the parent prototype/instance vtable. Perry returned `undefined` for `super.x` and threw "value is not a function" for `super.m()`. The receiver in a static method is the class constructor (a ClassRef), so the runtime helpers now branch on that: - js_super_accessor_get: when the receiver is a ClassRef, walk the parent class_id chain for a static getter (CLASS_STATIC_ACCESSORS), then a static data field (CLASS_DYNAMIC_PROPS). - js_super_method_call_dynamic: when `this` is a ClassRef, resolve the parent's static method (lookup_static_method_in_chain) and invoke it with `this` bound to the current class. - codegen: route `super.m()` to the dynamic helper whenever compile-time INSTANCE resolution fails (previously only for dynamic-parent classes), so the static case reaches the runtime resolver instead of a bogus 0.0. 2. accessor .name reflection (fn-name-accessor-{get,set} correctness): `Object.getOwnPropertyDescriptor(C.prototype, "x").get.name` must be "get x" / "set x" with { writable:false, enumerable:false, configurable:true }. The reflected accessor function value left `.name` as "". class_accessor_function_value now sets the prefixed name with the spec attributes (mirroring the Function.prototype.bind name path). test262 language/statements/class/super: 3 pass → 6 pass (+3: in-static-getter, in-static-methods, in-static-setter), 0 regressions. No regressions across subclass/definition/super. (The bundled fn-name-accessor test262 cases still need symbol-keyed accessor reflection, a separate gap, but string-keyed accessor .name is now spec-correct.) 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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRemoves the ChangesStatic super dispatch and accessor reflection
Sequence Diagram(s)sequenceDiagram
participant Engine
participant js_super_method_call_dynamic
participant ClassRegistry
participant js_super_accessor_get
participant CLASS_STATIC_ACCESSORS
rect rgba(70, 130, 180, 0.5)
Note over Engine,ClassRegistry: Static super.m() call
Engine->>js_super_method_call_dynamic: this_value=ClassRef, cid, method_name, args
js_super_method_call_dynamic->>ClassRegistry: lookup parent static method via cid chain
ClassRegistry-->>js_super_method_call_dynamic: method fn ptr
js_super_method_call_dynamic->>js_super_method_call_dynamic: set static_this = ClassRef, pack rest args
js_super_method_call_dynamic-->>Engine: return result (early exit)
end
rect rgba(60, 179, 113, 0.5)
Note over Engine,CLASS_STATIC_ACCESSORS: Static super.x accessor get
Engine->>js_super_accessor_get: receiver=ClassRef, cid, key
js_super_accessor_get->>CLASS_STATIC_ACCESSORS: walk parent chain for static getter
CLASS_STATIC_ACCESSORS-->>js_super_accessor_get: getter fn or miss
alt getter found
js_super_accessor_get->>js_super_accessor_get: set implicit_this = ClassRef, invoke getter
js_super_accessor_get-->>Engine: getter result
else data field found in CLASS_DYNAMIC_PROPS
js_super_accessor_get-->>Engine: data field value
else nothing found
js_super_accessor_get-->>Engine: undefined
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
…e prototype (#5345) `Object.prototype.hasOwnProperty.call(C.prototype, "x")` returned `false` for a class instance getter/setter/method, even though `Object.getOwnPropertyDescriptor(C.prototype, "x")` correctly returns the descriptor — the two disagreed. Class instance accessors and methods live in the class vtable, not the prototype object's keys_array, so `js_object_has_own` missed them. `js_object_has_own` now, for a class-declaration prototype object, also recognizes `constructor`, own instance accessors (`class_own_accessor_ptrs`), and own instance methods (`class_has_own_method`) — mirroring the `getOwnPropertyDescriptor` path. Refs class/definition/{getters,setters}-prop-desc (those tests also exercise accessor deletion for the `configurable` check, a separate gap, so they remain red — but hasOwnProperty is now spec-consistent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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/object/class_registry.rs`:
- Around line 3857-3861: The code incorrectly uses set_builtin_property_attrs to
set the "name" property on a specific closure object, but this function is only
intended for built-in prototype methods and omits critical side-effect flags for
descriptor tracking. Replace the set_builtin_property_attrs call with
set_property_attrs when setting the "name" property on the accessor function's
closure object, matching the same pattern used elsewhere in the codebase for
similar dynamic property descriptors.
🪄 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: af6367fa-1330-4eff-b35b-7dcc45d61624
📒 Files selected for processing (5)
crates/perry-codegen/src/expr/super_method.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/property_key.rs
Comment on lines
+3857
to
+3861
| crate::object::set_builtin_property_attrs( | ||
| closure as usize, | ||
| "name".to_string(), | ||
| crate::object::PropertyAttrs::new(false, false, true), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the difference between set_property_attrs and set_builtin_property_attrs
echo "=== set_property_attrs signature/implementation ==="
rg -n -A10 'pub(\s+\(crate\))?\s+(unsafe\s+)?fn\s+set_property_attrs\s*\(' --type=rust
echo ""
echo "=== set_builtin_property_attrs signature/implementation ==="
rg -n -A10 'pub(\s+\(crate\))?\s+(unsafe\s+)?fn\s+set_builtin_property_attrs\s*\(' --type=rustRepository: PerryTS/perry
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Simpler search for both functions
echo "=== Searching for set_property_attrs definition ==="
rg -n 'fn\s+set_property_attrs' --type=rust -A5
echo ""
echo "=== Searching for set_builtin_property_attrs definition ==="
rg -n 'fn\s+set_builtin_property_attrs' --type=rust -A5
echo ""
echo "=== Searching for where these functions are called ==="
rg -n 'set_property_attrs|set_builtin_property_attrs' --type=rust | head -20Repository: PerryTS/perry
Length of output: 3174
🏁 Script executed:
#!/bin/bash
# Get full implementation of both functions to understand differences
echo "=== Full set_property_attrs implementation ==="
rg -n -A20 'pub\(crate\) fn set_property_attrs\(obj: usize' crates/perry-runtime/src/object/mod.rs
echo ""
echo "=== Full set_builtin_property_attrs implementation ==="
rg -n -A10 'pub\(crate\) fn set_builtin_property_attrs\(obj: usize' crates/perry-runtime/src/object/mod.rs
echo ""
echo "=== Find set_bound_native_closure_name context ==="
rg -n -B5 -A10 'set_property_attrs.*"name"' crates/perry-runtime/src/native_module.rs | head -40Repository: PerryTS/perry
Length of output: 1831
🏁 Script executed:
#!/bin/bash
# Search more broadly for set_property_attrs usage with "name" key
echo "=== Uses of set_property_attrs with name key ==="
rg -n 'set_property_attrs.*"name"' --type=rust
echo ""
echo "=== Class registry file to see the accessor function code ==="
grep -n -A10 -B5 'set_builtin_property_attrs' crates/perry-runtime/src/object/class_registry.rs | head -50
echo ""
echo "=== Check if there's other similar name setting pattern in class_registry ==="
rg -n '\.name.*PropertyAttrs' crates/perry-runtime/src/object/class_registry.rs
echo ""
echo "=== Understand the purpose of PROPERTY_ATTRS_IN_USE and GLOBAL_DESCRIPTORS_IN_USE flags ==="
rg -n 'PROPERTY_ATTRS_IN_USE\|GLOBAL_DESCRIPTORS_IN_USE' crates/perry-runtime/src/object/mod.rs | head -20Repository: PerryTS/perry
Length of output: 2567
🏁 Script executed:
#!/bin/bash
# Let's understand when each function should be used
echo "=== Context around set_property_attrs function definition ==="
rg -n -B10 'pub\(crate\) fn set_property_attrs' crates/perry-runtime/src/object/mod.rs | head -20
echo ""
echo "=== Context around set_builtin_property_attrs function definition ==="
rg -n -B10 'pub\(crate\) fn set_builtin_property_attrs' crates/perry-runtime/src/object/mod.rs | head -30
echo ""
echo "=== The only usage of set_property_attrs with 'name' key ==="
rg -n -B10 -A5 'set_property_attrs.*obj as usize.*"name"' crates/perry-runtime/src/child_process/mod.rsRepository: PerryTS/perry
Length of output: 2533
Use set_property_attrs instead of set_builtin_property_attrs for the accessor function's .name property.
The code uses set_builtin_property_attrs, which is intended for built-in prototype methods (documented in mod.rs:911-920). It omits critical side-effect flags (PROPERTY_ATTRS_IN_USE, GLOBAL_DESCRIPTORS_IN_USE, disable_class_field_inline_guard()) that signal descriptor tracking is in use.
The accessor function's .name property is a dynamic property on a specific closure object, not a built-in prototype method. The only other usage of setting a "name" property descriptor in the codebase (child_process.rs:2024 for Error objects) correctly uses set_property_attrs. Use the same approach here.
🤖 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/object/class_registry.rs` around lines 3857 - 3861,
The code incorrectly uses set_builtin_property_attrs to set the "name" property
on a specific closure object, but this function is only intended for built-in
prototype methods and omits critical side-effect flags for descriptor tracking.
Replace the set_builtin_property_attrs call with set_property_attrs when setting
the "name" property on the accessor function's closure object, matching the same
pattern used elsewhere in the codebase for similar dynamic property descriptors.
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/object/object_ops.rs`:
- Around line 946-955: The prototype-object branch checking
class_id_for_decl_prototype_object is missing a deleted-key guard that exists in
the ClassRef branch. Before returning f64::from_bits(TAG_TRUE) in the
prototype-object branch, add a check to ensure the key is not marked as deleted
by calling class_is_key_deleted(cid, key) and only return true if the key has
not been deleted. This ensures that deleted instance methods and accessors
correctly report as not present when hasOwnProperty is called.
🪄 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: de00aae3-da01-4a49-b66f-aa58cb2ff216
📒 Files selected for processing (1)
crates/perry-runtime/src/object/object_ops.rs
#5345) Completes the class-accessor reflection fixes so `verifyProperty` passes for class getters/setters (test262 class/definition/{getters,setters}-prop-desc, +2): - `delete C.prototype.x` for a class instance accessor/method now records the key in CLASS_DELETED_KEYS (it lives in the class vtable, not the keys_array, so the prior keys-scan delete was a vacuous no-op). hasOwnProperty and getOwnPropertyDescriptor honor the deleted mark, so verifyProperty's `configurable` probe (`delete` then assert-absent) now observes the removal. - hasOwnProperty on a class constructor (ClassRef) now recognizes a static accessor (`static get x()`) as an own property (`class_own_static_accessor_ptrs`, own-only), mirroring getOwnPropertyDescriptor — previously it returned false ("staticX should be an own property"). test262 language/statements/class/definition: 49 → 51 pass (+2: getters-prop-desc, setters-prop-desc), 0 regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/object/delete_rest.rs`:
- Around line 188-198: The early deletion path for class-declaration prototype
objects in the condition block starting with class_id_for_decl_prototype_object
is missing a configurability check before calling class_mark_key_deleted. Add a
check using get_property_attrs to verify the property is configurable, similar
to the configurability checks performed in other deletion branches at lines
87-91, 106-110, 151-155, 169-173, and 229-233. This ensures that properties
redefined as non-configurable via Object.defineProperty cannot be incorrectly
deleted.
🪄 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: f0434d6e-b0f1-421d-9872-66d2784d119e
📒 Files selected for processing (3)
crates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/object_ops.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-runtime/src/object/descriptors.rs
Comment on lines
+188
to
+198
| if let Some(cid) = super::class_registry::class_id_for_decl_prototype_object(obj as usize) { | ||
| if let Some(name) = super::has_own_helpers::str_from_string_header(key) { | ||
| if name != "constructor" | ||
| && (super::class_registry::class_own_accessor_ptrs(cid, name).is_some() | ||
| || super::native_module::class_has_own_method(cid, name)) | ||
| { | ||
| super::class_registry::class_mark_key_deleted(cid, name); | ||
| return 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing configurability check before marking class-declared property as deleted.
The early deletion path for class-declaration prototype objects doesn't check whether the property has been redefined as non-configurable via Object.defineProperty. Other deletion branches in this function (lines 87-91, 106-110, 151-155, 169-173, 229-233) consistently check get_property_attrs before deleting.
If someone does Object.defineProperty(C.prototype, 'x', {configurable: false}) on an existing class accessor, the current code would incorrectly succeed in deletion because:
- No accessor descriptor is added to the side table (only property attrs)
- The check at line 168 returns
None, skipping the configurability guard - This early path marks the key deleted without further checks
🛡️ Proposed fix: add configurability check
if let Some(cid) = super::class_registry::class_id_for_decl_prototype_object(obj as usize) {
if let Some(name) = super::has_own_helpers::str_from_string_header(key) {
if name != "constructor"
&& (super::class_registry::class_own_accessor_ptrs(cid, name).is_some()
|| super::native_module::class_has_own_method(cid, name))
{
+ if let Some(attrs) = get_property_attrs(obj as usize, name) {
+ if !attrs.configurable() {
+ return 0;
+ }
+ }
super::class_registry::class_mark_key_deleted(cid, name);
return 1;
}
}
}📝 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
| if let Some(cid) = super::class_registry::class_id_for_decl_prototype_object(obj as usize) { | |
| if let Some(name) = super::has_own_helpers::str_from_string_header(key) { | |
| if name != "constructor" | |
| && (super::class_registry::class_own_accessor_ptrs(cid, name).is_some() | |
| || super::native_module::class_has_own_method(cid, name)) | |
| { | |
| super::class_registry::class_mark_key_deleted(cid, name); | |
| return 1; | |
| } | |
| } | |
| } | |
| if let Some(cid) = super::class_registry::class_id_for_decl_prototype_object(obj as usize) { | |
| if let Some(name) = super::has_own_helpers::str_from_string_header(key) { | |
| if name != "constructor" | |
| && (super::class_registry::class_own_accessor_ptrs(cid, name).is_some() | |
| || super::native_module::class_has_own_method(cid, name)) | |
| { | |
| if let Some(attrs) = get_property_attrs(obj as usize, name) { | |
| if !attrs.configurable() { | |
| return 0; | |
| } | |
| } | |
| super::class_registry::class_mark_key_deleted(cid, name); | |
| return 1; | |
| } | |
| } | |
| } |
🤖 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/object/delete_rest.rs` around lines 188 - 198, The
early deletion path for class-declaration prototype objects in the condition
block starting with class_id_for_decl_prototype_object is missing a
configurability check before calling class_mark_key_deleted. Add a check using
get_property_attrs to verify the property is configurable, similar to the
configurability checks performed in other deletion branches at lines 87-91,
106-110, 151-155, 169-173, and 229-233. This ensures that properties redefined
as non-configurable via Object.defineProperty cannot be incorrectly deleted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
added a commit
that referenced
this pull request
Jun 19, 2026
…) (#5443) 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: Ralph <ralph@skelpo.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Two ECMAScript class-semantics fixes for the #5345 test262 class tail.
1.
superin astaticmethod (+3 test262 cases)Inside a
staticmethod,super.x/super.m()must resolve against the parent's static side (a static getter, or a static method on the parent constructor) — not the parent prototype/instance vtable. Perry returnedundefinedforsuper.xand threw "value is not a function" forsuper.m().The receiver in a static method is the class constructor (a
ClassRef), so the runtime helpers now branch on that:js_super_accessor_get: ClassRef receiver → walk the parentclass_idchain for a static getter (CLASS_STATIC_ACCESSORS), then a static data field (CLASS_DYNAMIC_PROPS).js_super_method_call_dynamic:thisis a ClassRef → resolve the parent's static method (lookup_static_method_in_chain) and invoke it withthis= the current class.super.m()to the dynamic helper whenever compile-time instance resolution fails (previously only for dynamic-parent classes), so the static case reaches the runtime resolver instead of a bogus0.0.2. accessor
.namereflection (correctness)Object.getOwnPropertyDescriptor(C.prototype, "x").get.namemust be"get x"/"set x"with{writable:false, enumerable:false, configurable:true}. The reflected accessor value left.nameas"".class_accessor_function_valuenow sets the prefixed name with the spec attributes (mirroring theFunction.prototype.bindname path).Verification
test262
language/statements/class/super(node v26): 3 pass → 6 pass (+3) —in-static-getter,in-static-methods,in-static-setter. 0 regressions acrosssuper/subclass/definition.Direct checks:
super.xin a static method → parent static getter value;super.m()in a static method → parent static method (bound to the current class); accessor descriptor.name→"get x"with correct attributes.🤖 Generated with Claude Code
Summary by CodeRabbit
superbehavior in static class contexts for static methods and static properties, including correct handling of rest parameters.get <key>/set <key>and use expected descriptor attributes.Object.hasOwn/hasOwnPropertynow correctly recognize class static accessor properties and declared class prototype objects.delete obj[name]now records deletions for class-declared prototype instance accessors and native class-owned methods.