refactor: rename CreateMethodProperty to DefineMethodProperty and use it in VM opcodes

Your Name · Your Name · commit 5ea7a2b0e0f4 · 2026-03-13T00:47:11.000+05:30
Address PR review feedback:
- Rename create_method_property to define_method_property per ECMAScript 2026 spec (10.2.8)
- Update return type to JsResult&lt;()&gt; since the spec returns 'unused'
- Use define_method_property in DefineClassMethodByName, DefineClassStaticMethodByName,
  and DefineClassMethodByValue VM opcodes instead of manual PropertyDescriptor construction
- Remove unrelated TypedArray wrapper methods (entries, keys, values, iterator, to_string)
  and associated tests from jstypedarray.rs
- Revert pub(crate) visibility changes in builtin.rs
- Move CopyDataProperties from jsobject.rs to operations.rs
diff --git a/core/engine/src/builtins/typed_array/builtin.rs b/core/engine/src/builtins/typed_array/builtin.rs
@@ -624,7 +624,7 @@ impl BuiltinTypedArray {
     ///  - [ECMAScript reference][spec]
     ///
     /// [spec]: https://tc39.es/ecma262/#sec-%typedarray%.prototype.entries
-    pub(crate) fn entries(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
+    fn entries(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
         // 1. Let O be the this value.
         // 2. Perform ? ValidateTypedArray(O, seq-cst).
         let (ta, _) = TypedArray::validate(this, Ordering::SeqCst)?;
@@ -2546,7 +2546,7 @@ impl BuiltinTypedArray {
     ///  - [ECMAScript reference][spec]
     ///
     /// [spec]: https://tc39.es/ecma262/#sec-%typedarray%.prototype.values
-    pub(crate) fn values(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
+    fn values(this: &JsValue, _: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
         // 1. Let O be the this value.
         // 2. Perform ? ValidateTypedArray(O, seq-cst).
         let (ta, _) = TypedArray::validate(this, Ordering::SeqCst)?;
diff --git a/core/engine/src/object/builtins/jstypedarray.rs b/core/engine/src/object/builtins/jstypedarray.rs
@@ -877,41 +877,6 @@ impl JsTypedArray {
         })
     }
 
-    /// Calls `TypedArray.prototype.entries()`.
-    #[inline]
-    pub fn entries(&self, context: &mut Context) -> JsResult<JsValue> {
-        BuiltinTypedArray::entries(&self.inner.clone().into(), &[], context)
-    }
-
-    /// Calls `TypedArray.prototype.keys()`.
-    #[inline]
-    pub fn keys(&self, context: &mut Context) -> JsResult<JsValue> {
-        BuiltinTypedArray::keys(&self.inner.clone().into(), &[], context)
-    }
-
-    /// Calls `TypedArray.prototype.values()`.
-    #[inline]
-    pub fn values(&self, context: &mut Context) -> JsResult<JsValue> {
-        BuiltinTypedArray::values(&self.inner.clone().into(), &[], context)
-    }
-
-    /// Calls `TypedArray.prototype[@@iterator]()`.
-    #[inline]
-    pub fn iterator(&self, context: &mut Context) -> JsResult<JsValue> {
-        BuiltinTypedArray::values(&self.inner.clone().into(), &[], context)
-    }
-
-    /// Calls `TypedArray.prototype.toString()`.
-    #[inline]
-    pub fn to_string(&self, context: &mut Context) -> JsResult<JsString> {
-        // TypedArray.prototype.toString is the same as Array.prototype.toString
-        let result = crate::builtins::Array::to_string(&self.inner.clone().into(), &[], context)?;
-        result
-            .as_string()
-            .js_expect("Array.prototype.toString always returns string")
-            .map_err(Into::into)
-    }
-
     /// It is a getter that returns the same string as the typed array constructor's name.
     /// It returns `Ok(JsValue::Undefined)` if the this value is not one of the typed array subclasses.
     ///
@@ -1383,87 +1348,3 @@ fn typed_iterators_f32() {
     let vec2 = array.iter(context).collect::<Vec<_>>();
     assert_eq!(vec, vec2);
 }
-
-#[test]
-fn typed_array_to_string() {
-    let context = &mut Context::default();
-    let vec = vec![1u8, 2, 3];
-    let array = JsUint8Array::from_iter(vec, context).unwrap();
-    assert_eq!(array.to_string(context).unwrap(), crate::js_string!("1,2,3"));
-}
-
-#[test]
-fn typed_array_entries() {
-    let context = &mut Context::default();
-    let vec = vec![1u8, 2];
-    let array = JsUint8Array::from_iter(vec, context).unwrap();
-    let entries = array.entries(context).unwrap();
-    let mut entries_vec = Vec::new();
-    let next_str = crate::js_string!("next");
-    loop {
-        let next_fn = entries.as_object().unwrap().get(next_str.clone(), context).unwrap();
-        let result = next_fn.as_object().unwrap().call(&entries, &[], context).unwrap();
-        if result.as_object().unwrap().get(crate::js_string!("done"), context).unwrap().to_boolean() {
-            break;
-        }
-        entries_vec.push(result.as_object().unwrap().get(crate::js_string!("value"), context).unwrap());
-    }
-    assert_eq!(entries_vec.len(), 2);
-}
-
-#[test]
-fn typed_array_keys() {
-    let context = &mut Context::default();
-    let vec = vec![1u8, 2];
-    let array = JsUint8Array::from_iter(vec, context).unwrap();
-    let keys = array.keys(context).unwrap();
-    let mut keys_vec = Vec::new();
-    let next_str = crate::js_string!("next");
-    loop {
-        let next_fn = keys.as_object().unwrap().get(next_str.clone(), context).unwrap();
-        let result = next_fn.as_object().unwrap().call(&keys, &[], context).unwrap();
-        if result.as_object().unwrap().get(crate::js_string!("done"), context).unwrap().to_boolean() {
-            break;
-        }
-        keys_vec.push(result.as_object().unwrap().get(crate::js_string!("value"), context).unwrap());
-    }
-    assert_eq!(keys_vec, vec![JsValue::new(0), JsValue::new(1)]);
-}
-
-#[test]
-fn typed_array_values() {
-    let context = &mut Context::default();
-    let vec = vec![1u8, 2];
-    let array = JsUint8Array::from_iter(vec, context).unwrap();
-    let values = array.values(context).unwrap();
-    let mut values_vec = Vec::new();
-    let next_str = crate::js_string!("next");
-    loop {
-        let next_fn = values.as_object().unwrap().get(next_str.clone(), context).unwrap();
-        let result = next_fn.as_object().unwrap().call(&values, &[], context).unwrap();
-        if result.as_object().unwrap().get(crate::js_string!("done"), context).unwrap().to_boolean() {
-            break;
-        }
-        values_vec.push(result.as_object().unwrap().get(crate::js_string!("value"), context).unwrap());
-    }
-    assert_eq!(values_vec, vec![JsValue::new(1), JsValue::new(2)]);
-}
-
-#[test]
-fn typed_array_iterator() {
-    let context = &mut Context::default();
-    let vec = vec![1u8, 2];
-    let array = JsUint8Array::from_iter(vec, context).unwrap();
-    let values = array.iterator(context).unwrap();
-    let mut values_vec = Vec::new();
-    let next_str = crate::js_string!("next");
-    loop {
-        let next_fn = values.as_object().unwrap().get(next_str.clone(), context).unwrap();
-        let result = next_fn.as_object().unwrap().call(&values, &[], context).unwrap();
-        if result.as_object().unwrap().get(crate::js_string!("done"), context).unwrap().to_boolean() {
-            break;
-        }
-        values_vec.push(result.as_object().unwrap().get(crate::js_string!("value"), context).unwrap());
-    }
-    assert_eq!(values_vec, vec![JsValue::new(1), JsValue::new(2)]);
-}
diff --git a/core/engine/src/object/operations.rs b/core/engine/src/object/operations.rs
@@ -177,33 +177,40 @@ impl JsObject {
         self.__define_own_property__(&key.into(), new_desc.into(), context)
     }
 
-    /// `7.3.5 CreateMethodProperty ( O, P, V )`
+    /// `10.2.8 DefineMethodProperty ( homeObject, key, closure, enumerable )`
+    ///
+    /// Defines a method property on an object with the specified attributes.
     ///
     /// More information:
     ///  - [ECMAScript reference][spec]
     ///
-    /// [spec]: https://tc39.es/ecma262/#sec-createmethodproperty
-    pub(crate) fn create_method_property<K, V>(
+    /// [spec]: https://tc39.es/ecma262/#sec-definemethodproperty
+    pub(crate) fn define_method_property<K, V>(
         &self,
         key: K,
         value: V,
         context: &mut InternalMethodPropertyContext<'_>,
-    ) -> JsResult<bool>
+    ) -> JsResult<()>
     where
         K: Into<PropertyKey>,
         V: Into<JsValue>,
     {
-        // 1. Assert: Type(O) is Object.
-        // 2. Assert: IsPropertyKey(P) is true.
-        // 3. Let newDesc be the PropertyDescriptor { [[Value]]: V, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }.
+        // 1. Assert: homeObject is an ordinary, extensible object with no non-configurable properties.
+        // 2. If key is a Private Name, then
+        //    a. Return PrivateElement { [[Key]]: key, [[Kind]]: method, [[Value]]: closure }.
+        // 3. Else,
+        //    a. Let desc be the PropertyDescriptor { [[Value]]: closure, [[Writable]]: true, [[Enumerable]]: enumerable, [[Configurable]]: true }.
         let new_desc = PropertyDescriptor::builder()
             .value(value)
             .writable(true)
             .enumerable(false)
             .configurable(true);
 
-        // 4. Return ! O.[[DefineOwnProperty]](P, newDesc).
-        self.__define_own_property__(&key.into(), new_desc.into(), context)
+        //    b. Perform ! DefinePropertyOrThrow(homeObject, key, desc).
+        self.__define_own_property__(&key.into(), new_desc.into(), context)?;
+
+        //    c. Return unused.
+        Ok(())
     }
 
     /// Create data property or throw
diff --git a/core/engine/src/vm/opcode/define/class/method.rs b/core/engine/src/vm/opcode/define/class/method.rs
@@ -39,14 +39,9 @@ impl DefineClassStaticMethodByName {
                 .set_home_object(class.clone());
         }
 
-        class.__define_own_property__(
-            &key,
-            PropertyDescriptor::builder()
-                .value(function.clone())
-                .writable(true)
-                .enumerable(false)
-                .configurable(true)
-                .build(),
+        class.define_method_property(
+            key,
+            function.clone(),
             &mut InternalMethodPropertyContext::new(context),
         )?;
         Ok(())
@@ -92,14 +87,9 @@ impl DefineClassMethodByName {
                 .set_home_object(class_proto.clone());
         }
 
-        class_proto.__define_own_property__(
-            &key,
-            PropertyDescriptor::builder()
-                .value(function.clone())
-                .writable(true)
-                .enumerable(false)
-                .configurable(true)
-                .build(),
+        class_proto.define_method_property(
+            key,
+            function.clone(),
             &mut InternalMethodPropertyContext::new(context),
         )?;
         Ok(())
@@ -194,14 +184,9 @@ impl DefineClassMethodByValue {
                 .set_home_object(class_proto.clone());
         }
 
-        class_proto.__define_own_property__(
-            &key,
-            PropertyDescriptor::builder()
-                .value(function.clone())
-                .writable(true)
-                .enumerable(false)
-                .configurable(true)
-                .build(),
+        class_proto.define_method_property(
+            key,
+            function.clone(),
             &mut InternalMethodPropertyContext::new(context),
         )?;
         Ok(())
