Repository: trynova/nova (language: rust)

Issue:
Implement `Object.groupBy`
[Object.groupBy](https://tc39.es/ecma262/#sec-object.groupby) is the only builtin function still missing from Object constructor. The main work needed here is the [GroupBy](https://tc39.es/ecma262/#sec-groupby) abstract operation.


Discussion / hints:

Fix patch:
diff --git a/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs b/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs
index 4d591904c..7e6822335 100644
--- a/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs
+++ b/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs
@@ -5,15 +5,21 @@
 //! ## [7.3 Operations on Objects](https://tc39.es/ecma262/#sec-operations-on-objects)
 
 use super::{
-    testing_and_comparison::{is_callable, same_value},
-    type_conversion::{to_length, to_object},
+    operations_on_iterator_objects::{get_iterator, if_abrupt_close_iterator, iterator_close},
+    testing_and_comparison::{is_callable, require_object_coercible, same_value},
+    type_conversion::{to_length, to_object, to_property_key},
 };
 use crate::{
     ecmascript::{
-        builtins::{array_create, ArgumentsList, Array},
+        abstract_operations::operations_on_iterator_objects::iterator_step_value,
+        builtins::{
+            array_create,
+            keyed_collections::map_objects::map_prototype::canonicalize_keyed_collection_key,
+            ArgumentsList, Array,
+        },
         execution::{agent::ExceptionType, Agent, JsResult, RealmIdentifier},
         types::{
-            Function, InternalMethods, IntoObject, IntoValue, Object, PropertyDescriptor,
+            Function, InternalMethods, IntoObject, IntoValue, Number, Object, PropertyDescriptor,
             PropertyKey, String, Value, BUILTIN_STRING_MEMORY,
         },
     },
@@ -756,3 +762,223 @@ pub(crate) fn get_function_realm(
         _ => Ok(agent.current_realm_id()),
     }
 }
+
+/// ### [7.3.34 AddValueToKeyedGroup ( groups, key, value )](https://tc39.es/ecma262/#sec-add-value-to-keyed-group)
+/// The abstract operation AddValueToKeyedGroup takes arguments groups (a List of Records with fields
+/// [[Key]] (an ECMAScript language value) and [[Elements]] (a List of ECMAScript language values)),
+/// key (an ECMAScript language value), and value (an ECMAScript language value) and returns UNUSED.
+pub(crate) fn add_value_to_keyed_group<K: Copy + Into<Value>>(
+    agent: &mut Agent,
+    groups: &mut Vec<GroupByRecord<K>>,
+    key: K,
+    value: Value,
+) -> JsResult<()> {
+    // 1. For each Record { [[Key]], [[Elements]] } g of groups, do
+    for g in groups.iter_mut() {
+        // a. If SameValue(g.[[Key]], key) is true, then
+        if same_value(agent, g.key, key) {
+            // i. Assert: Exactly one element of groups meets this criterion.
+            // ii. Append value to g.[[Elements]].
+            g.elements.push(value);
+
+            // iii. Return UNUSED.
+            return Ok(());
+        }
+    }
+
+    // 2. Let group be the Record { [[Key]]: key, [[Elements]]: « value » }.
+    let group = GroupByRecord {
+        key,
+        elements: vec![value],
+    };
+
+    // 3. Append group to groups.
+    groups.push(group);
+
+    // 4. Return UNUSED.
+    Ok(())
+}
+
+#[derive(Debug)]
+pub(crate) struct GroupByRecord<K: Copy + Into<Value>> {
+    pub(crate) key: K,
+    pub(crate) elements: Vec<Value>,
+}
+
+/// ### [7.3.35 GroupBy ( items, callback, keyCoercion )](https://tc39.es/ecma262/#sec-groupby)
+///
+/// The abstract operation GroupBy takes arguments items (an ECMAScript language value), callback
+/// (an ECMAScript language value), and keyCoercion (property or collection) and returns either a
+/// normal completion containing a List of Records with fields [[Key]] (an ECMAScript language
+/// value) and [[Elements]] (a List of ECMAScript language values), or a throw completion.
+///
+/// Note: This version is for "property" keyCoercion.
+pub(crate) fn group_by_property(
+    agent: &mut Agent,
+    items: Value,
+    callback_fn: Value,
+) -> JsResult<Vec<GroupByRecord<PropertyKey>>> {
+    // 1. Perform ? RequireObjectCoercible(iterable).
+    require_object_coercible(agent, items)?;
+
+    // 2. If IsCallable(callback) is false, throw a TypeError exception.
+    let Some(callback_fn) = is_callable(callback_fn) else {
+        return Err(agent.throw_exception_with_static_message(
+            ExceptionType::TypeError,
+            "Callback is not callable",
+        ));
+    };
+
+    // 3. Let groups be a new empty List.
+    let mut groups: Vec<GroupByRecord<PropertyKey>> = vec![];
+
+    // 4. Let iteratorRecord be ? GetIterator(iterable).
+    let mut iterator_record = get_iterator(agent, items, false)?;
+
+    // 5. Let k be 0.
+    let mut k = 0;
+
+    // 6. Repeat,
+    loop {
+        // NOTE: The actual max size of an array is u32::MAX
+        // a. If k ≥ 2**53 - 1, then
+        if k >= u32::MAX as usize {
+            // i. Let error be ThrowCompletion(a newly created TypeError object).
+            let error = agent.throw_exception_with_static_message(
+                ExceptionType::TypeError,
+                "Maximum array size of 2**53-1 exceeded",
+            );
+
+            // ii. Return ? IteratorClose(iteratorRecord, error).
+            return iterator_close(agent, &iterator_record, Err(error));
+        }
+
+        // b. Let next be ? IteratorStepValue(iteratorRecord).
+        let next = iterator_step_value(agent, &mut iterator_record)?;
+
+        // c. If next is DONE, then
+        //   i. Return groups.
+        let Some(next) = next else {
+            return Ok(groups);
+        };
+
+        // d. Let value be next.
+        let value = next;
+
+        let sk = SmallInteger::try_from(k as u64).unwrap();
+        // 𝔽(k)
+        let fk = Number::from(sk).into_value();
+
+        // e. Let key be Completion(Call(callback, undefined, « value, 𝔽(k) »)).
+        let key = call_function(
+            agent,
+            callback_fn,
+            Value::Undefined,
+            Some(ArgumentsList(&[value, fk])),
+        );
+
+        // f. IfAbruptCloseIterator(key, iteratorRecord).
+        let key = if_abrupt_close_iterator(agent, key, &iterator_record)?;
+
+        // g. If keyCoercion is property, then
+        // i. Set key to Completion(ToPropertyKey(key)).
+        let key = to_property_key(agent, key);
+
+        // ii. IfAbruptCloseIterator(key, iteratorRecord).
+        let key = if_abrupt_close_iterator(agent, key, &iterator_record)?;
+
+        // i. Perform AddValueToKeyedGroup(groups, key, value).
+        add_value_to_keyed_group(agent, &mut groups, key, value)?;
+
+        // j. Set k to k + 1.
+        k += 1;
+    }
+}
+
+/// ### [7.3.35 GroupBy ( items, callback, keyCoercion )](https://tc39.es/ecma262/#sec-groupby)
+///
+/// The abstract operation GroupBy takes arguments items (an ECMAScript language value), callback
+/// (an ECMAScript language value), and keyCoercion (property or collection) and returns either a
+/// normal completion containing a List of Records with fields [[Key]] (an ECMAScript language
+/// value) and [[Elements]] (a List of ECMAScript language values), or a throw completion.
+///
+/// Note: This version is for "collection" keyCoercion.
+pub(crate) fn group_by_collection(
+    agent: &mut Agent,
+    items: Value,
+    callback_fn: Value,
+) -> JsResult<Vec<GroupByRecord<Value>>> {
+    // 1. Perform ? RequireObjectCoercible(iterable).
+    require_object_coercible(agent, items)?;
+
+    // 2. If IsCallable(callback) is false, throw a TypeError exception.
+    let Some(callback_fn) = is_callable(callback_fn) else {
+        return Err(agent.throw_exception_with_static_message(
+            ExceptionType::TypeError,
+            "Callback is not callable",
+        ));
+    };
+
+    // 3. Let groups be a new empty List.
+    let mut groups: Vec<GroupByRecord<Value>> = vec![];
+
+    // 4. Let iteratorRecord be ? GetIterator(iterable).
+    let mut iterator_record = get_iterator(agent, items, false)?;
+
+    // 5. Let k be 0.
+    let mut k = 0;
+
+    // 6. Repeat,
+    loop {
+        // NOTE: The actual max size of an array is u32::MAX
+        // a. If k ≥ 2**53 - 1, then
+        if k >= u32::MAX as usize {
+            // i. Let error be ThrowCompletion(a newly created TypeError object).
+            let error = agent.throw_exception_with_static_message(
+                ExceptionType::TypeError,
+                "Maximum array size of 2**53-1 exceeded",
+            );
+
+            // ii. Return ? IteratorClose(iteratorRecord, error).
+            return iterator_close(agent, &iterator_record, Err(error));
+        }
+
+        // b. Let next be ? IteratorStepValue(iteratorRecord).
+        let next = iterator_step_value(agent, &mut iterator_record)?;
+
+        // c. If next is DONE, then
+        //   i. Return groups.
+        let Some(next) = next else {
+            return Ok(groups);
+        };
+
+        // d. Let value be next.
+        let value = next;
+
+        let sk = SmallInteger::try_from(k as u64).unwrap();
+        // 𝔽(k)
+        let fk = Number::from(sk).into_value();
+
+        // e. Let key be Completion(Call(callback, undefined, « value, 𝔽(k) »)).
+        let key = call_function(
+            agent,
+            callback_fn,
+            Value::Undefined,
+            Some(ArgumentsList(&[value, fk])),
+        );
+
+        // f. IfAbruptCloseIterator(key, iteratorRecord).
+        let key = if_abrupt_close_iterator(agent, key, &iterator_record)?;
+
+        // h. Else,
+        // i. Assert: keyCoercion is collection.
+        // ii. Set key to CanonicalizeKeyedCollectionKey(key).
+        let key = canonicalize_keyed_collection_key(agent, key);
+
+        // i. Perform AddValueToKeyedGroup(groups, key, value).
+        add_value_to_keyed_group(agent, &mut groups, key, value)?;
+
+        // j. Set k to k + 1.
+        k += 1;
+    }
+}
diff --git a/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs b/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs
index ae470638e..9bbfe0503 100644
--- a/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs
+++ b/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs
@@ -9,8 +9,9 @@ use crate::{
                 get_iterator, if_abrupt_close_iterator, iterator_close, iterator_step_value,
             },
             operations_on_objects::{
-                create_array_from_list, define_property_or_throw, enumerable_own_properties,
-                enumerable_properties_kind, get, get_method, has_own_property,
+                create_array_from_list, create_data_property_or_throw, define_property_or_throw,
+                enumerable_own_properties, enumerable_properties_kind, get, get_method,
+                group_by_property, has_own_property,
                 integrity::{Frozen, Sealed},
                 set, set_integrity_level, test_integrity_level,
             },
@@ -617,6 +618,7 @@ impl ObjectConstructor {
         Ok(create_array_from_list(agent, &keys).into_value())
     }
 
+    /// ### [20.1.2.12 Object.getPrototypeOf ( O )](https://tc39.es/ecma262/#sec-object.getprototypeof)
     fn get_prototype_of(
         agent: &mut Agent,
         _this_value: Value,
@@ -627,12 +629,33 @@ impl ObjectConstructor {
             .map(|proto| proto.map_or(Value::Null, |proto| proto.into_value()))
     }
 
+    // ### [20.1.2.13 Object.groupBy ( items, callback )](https://tc39.es/ecma262/#sec-object.groupby)
     fn group_by(
-        _agent: &mut Agent,
+        agent: &mut Agent,
         _this_value: Value,
-        _arguments: ArgumentsList,
+        arguments: ArgumentsList,
     ) -> JsResult<Value> {
-        todo!()
+        let items = arguments.get(0);
+        let callback_fn = arguments.get(1);
+
+        // 1. Let groups be ? GroupBy(items, callback, property).
+        let groups = group_by_property(agent, items, callback_fn)?;
+
+        // 2. Let obj be OrdinaryObjectCreate(null).
+        let object =
+            ordinary_object_create_with_intrinsics(agent, Some(ProtoIntrinsics::Object), None);
+
+        // 3. For each Record { [[Key]], [[Elements]] } g of groups, do
+        for g in groups {
+            // a. Let elements be CreateArrayFromList(g.[[Elements]]).
+            let elements = create_array_from_list(agent, &g.elements).into_value();
+
+            // b. Perform ! CreateDataPropertyOrThrow(obj, g.[[Key]], elements).
+            create_data_property_or_throw(agent, object, g.key, elements)?;
+        }
+
+        // 4. Return obj.
+        Ok(object.into_value())
     }
 
     fn has_own(agent: &mut Agent, _this_value: Value, arguments: ArgumentsList) -> JsResult<Value> {


Test patch:
diff --git a/tests/expectations.json b/tests/expectations.json
index 180ef90df..0e646633a 100644
--- a/tests/expectations.json
+++ b/tests/expectations.json
@@ -3421,18 +3421,8 @@
   "built-ins/Object/getOwnPropertySymbols/proxy-invariant-not-extensible-extra-string-key.js": "CRASH",
   "built-ins/Object/getPrototypeOf/15.2.3.2-2-25.js": "CRASH",
   "built-ins/Object/getPrototypeOf/15.2.3.2-2-26.js": "CRASH",
-  "built-ins/Object/groupBy/callback-arg.js": "CRASH",
-  "built-ins/Object/groupBy/callback-throws.js": "CRASH",
-  "built-ins/Object/groupBy/emptyList.js": "CRASH",
-  "built-ins/Object/groupBy/evenOdd.js": "CRASH",
-  "built-ins/Object/groupBy/groupLength.js": "CRASH",
-  "built-ins/Object/groupBy/invalid-callback.js": "CRASH",
-  "built-ins/Object/groupBy/invalid-iterable.js": "CRASH",
-  "built-ins/Object/groupBy/invalid-property-key.js": "CRASH",
-  "built-ins/Object/groupBy/iterator-next-throws.js": "CRASH",
   "built-ins/Object/groupBy/null-prototype.js": "CRASH",
   "built-ins/Object/groupBy/string.js": "CRASH",
-  "built-ins/Object/groupBy/toPropertyKey.js": "CRASH",
   "built-ins/Object/internals/DefineOwnProperty/consistent-value-regexp-dollar1.js": "CRASH",
   "built-ins/Object/isFrozen/proxy-no-ownkeys-returned-keys-order.js": "CRASH",
   "built-ins/Object/isSealed/proxy-no-ownkeys-returned-keys-order.js": "CRASH",
diff --git a/tests/metrics.json b/tests/metrics.json
index d66930685..3f010c0f4 100644
--- a/tests/metrics.json
+++ b/tests/metrics.json
@@ -1,8 +1,8 @@
 {
   "results": {
-    "crash": 22275,
-    "fail": 5741,
-    "pass": 17003,
+    "crash": 22261,
+    "fail": 5745,
+    "pass": 17013,
     "skip": 31,
     "timeout": 1,
     "unresolved": 0