-
-
Notifications
You must be signed in to change notification settings - Fork 132
fix(class): super-in-static resolution + accessor .name reflection (#5345) #5395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f539c31
db9cce8
dae833b
43448d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -178,6 +178,24 @@ pub extern "C" fn js_object_delete_field( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // below so hasOwnProperty / Object.keys stop seeing it. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A class-declaration prototype object: instance accessors (`get x()`) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // and methods live in the class vtable, not the keys_array, so the scan | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // below would "succeed vacuously" while the member stayed visible to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // hasOwnProperty / getOwnPropertyDescriptor. Record the key as deleted | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // so those reflective paths agree it is gone (test262 verifyProperty's | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // `configurable` check: `delete obj[name]` then assert the key absent — | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // class/definition/{getters,setters}-prop-desc). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+188
to
+198
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If someone does
🛡️ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let keys = (*obj).keys_array; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if keys.is_null() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // No keys array means no fields to delete, but delete "succeeds" vacuously | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 170
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 3174
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 1831
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 2567
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 2533
Use
set_property_attrsinstead ofset_builtin_property_attrsfor the accessor function's.nameproperty.The code uses
set_builtin_property_attrs, which is intended for built-in prototype methods (documented inmod.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
.nameproperty 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 usesset_property_attrs. Use the same approach here.🤖 Prompt for AI Agents