Replace FxHashMap with IndexMap in object properties (#1547)

* Replace `FxHashMap` with `IndexMap` in object properties

* Add test for property order

* Remove unneeded sort from `JSON.stringify`

boa/src/builtins/json/mod.rs

@@ -13,8 +13,6 @@
 //! [json]: https://www.json.org/json-en.html
 //! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON
 
-use std::collections::HashSet;
-
 use crate::{
     builtins::{
         string::{is_leading_surrogate, is_trailing_surrogate},
@@ -172,7 +170,7 @@ impl Json {
                 // ii. If isArray is true, then
                 if replacer_obj.is_array() {
                     // 1. Set PropertyList to a new empty List.
-                    let mut property_set = HashSet::new();
+                    let mut property_set = indexmap::IndexSet::new();
 
                     // 2. Let len be ? LengthOfArrayLike(replacer).
                     let len = replacer_obj.length_of_array_like(context)?;
@@ -477,7 +475,7 @@ impl Json {
         state.indent = JsString::concat(&state.indent, &state.gap);
 
         // 5. If state.[[PropertyList]] is not undefined, then
-        let mut k = if let Some(p) = &state.property_list {
+        let k = if let Some(p) = &state.property_list {
             // a. Let K be state.[[PropertyList]].
             p.clone()
         // 6. Else,
@@ -488,9 +486,6 @@ impl Json {
             keys.iter().map(|v| v.to_string(context).unwrap()).collect()
         };
 
-        // Sort the property key list, because the internal property hashmap of objects does not sort the properties.
-        k.sort();
-
         // 7. Let partial be a new empty List.
         let mut partial = Vec::new();
 

boa/src/object/property_map.rs

@@ -1,18 +1,38 @@
 use super::{PropertyDescriptor, PropertyKey};
 use crate::{
-    gc::{Finalize, Trace},
+    gc::{custom_trace, Finalize, Trace},
     JsString, JsSymbol,
 };
-use rustc_hash::FxHashMap;
-use std::{collections::hash_map, iter::FusedIterator};
+use indexmap::IndexMap;
+use rustc_hash::{FxHashMap, FxHasher};
+use std::{collections::hash_map, hash::BuildHasherDefault, iter::FusedIterator};
+
+/// Wrapper around indexmap::IndexMap for usage in PropertyMap
+#[derive(Debug, Finalize)]
+struct OrderedHashMap<K: Trace>(IndexMap<K, PropertyDescriptor, BuildHasherDefault<FxHasher>>);
+
+impl<K: Trace> Default for OrderedHashMap<K> {
+    fn default() -> Self {
+        Self(IndexMap::with_hasher(BuildHasherDefault::default()))
+    }
+}
+
+unsafe impl<K: Trace> Trace for OrderedHashMap<K> {
+    custom_trace!(this, {
+        for (k, v) in this.0.iter() {
+            mark(k);
+            mark(v);
+        }
+    });
+}
 
 #[derive(Default, Debug, Trace, Finalize)]
 pub struct PropertyMap {
     indexed_properties: FxHashMap<u32, PropertyDescriptor>,
     /// Properties
-    string_properties: FxHashMap<JsString, PropertyDescriptor>,
+    string_properties: OrderedHashMap<JsString>,
     /// Symbol Properties
-    symbol_properties: FxHashMap<JsSymbol, PropertyDescriptor>,
+    symbol_properties: OrderedHashMap<JsSymbol>,
 }
 
 impl PropertyMap {
@@ -22,8 +42,8 @@ impl PropertyMap {
     pub fn get(&self, key: &PropertyKey) -> Option<&PropertyDescriptor> {
         match key {
             PropertyKey::Index(index) => self.indexed_properties.get(index),
-            PropertyKey::String(string) => self.string_properties.get(string),
-            PropertyKey::Symbol(symbol) => self.symbol_properties.get(symbol),
+            PropertyKey::String(string) => self.string_properties.0.get(string),
+            PropertyKey::Symbol(symbol) => self.symbol_properties.0.get(symbol),
         }
     }
 
@@ -34,16 +54,20 @@ impl PropertyMap {
     ) -> Option<PropertyDescriptor> {
         match &key {
             PropertyKey::Index(index) => self.indexed_properties.insert(*index, property),
-            PropertyKey::String(string) => self.string_properties.insert(string.clone(), property),
-            PropertyKey::Symbol(symbol) => self.symbol_properties.insert(symbol.clone(), property),
+            PropertyKey::String(string) => {
+                self.string_properties.0.insert(string.clone(), property)
+            }
+            PropertyKey::Symbol(symbol) => {
+                self.symbol_properties.0.insert(symbol.clone(), property)
+            }
         }
     }
 
     pub fn remove(&mut self, key: &PropertyKey) -> Option<PropertyDescriptor> {
         match key {
             PropertyKey::Index(index) => self.indexed_properties.remove(index),
-            PropertyKey::String(string) => self.string_properties.remove(string),
-            PropertyKey::Symbol(symbol) => self.symbol_properties.remove(symbol),
+            PropertyKey::String(string) => self.string_properties.0.shift_remove(string),
+            PropertyKey::Symbol(symbol) => self.symbol_properties.0.shift_remove(symbol),
         }
     }
 
@@ -54,8 +78,8 @@ impl PropertyMap {
     pub fn iter(&self) -> Iter<'_> {
         Iter {
             indexed_properties: self.indexed_properties.iter(),
-            string_properties: self.string_properties.iter(),
-            symbol_properties: self.symbol_properties.iter(),
+            string_properties: self.string_properties.0.iter(),
+            symbol_properties: self.symbol_properties.0.iter(),
         }
     }
 
@@ -81,23 +105,23 @@ impl PropertyMap {
     /// This iterator does not recurse down the prototype chain.
     #[inline]
     pub fn symbol_properties(&self) -> SymbolProperties<'_> {
-        SymbolProperties(self.symbol_properties.iter())
+        SymbolProperties(self.symbol_properties.0.iter())
     }
 
     /// An iterator visiting all symbol keys in arbitrary order. The iterator element type is `&'a RcSymbol`.
     ///
     /// This iterator does not recurse down the prototype chain.
     #[inline]
     pub fn symbol_property_keys(&self) -> SymbolPropertyKeys<'_> {
-        SymbolPropertyKeys(self.symbol_properties.keys())
+        SymbolPropertyKeys(self.symbol_properties.0.keys())
     }
 
     /// An iterator visiting all symbol values in arbitrary order. The iterator element type is `&'a Property`.
     ///
     /// This iterator does not recurse down the prototype chain.
     #[inline]
     pub fn symbol_property_values(&self) -> SymbolPropertyValues<'_> {
-        SymbolPropertyValues(self.symbol_properties.values())
+        SymbolPropertyValues(self.symbol_properties.0.values())
     }
 
     /// An iterator visiting all indexed key-value pairs in arbitrary order. The iterator element type is `(&'a u32, &'a Property)`.
@@ -129,31 +153,31 @@ impl PropertyMap {
     /// This iterator does not recurse down the prototype chain.
     #[inline]
     pub fn string_properties(&self) -> StringProperties<'_> {
-        StringProperties(self.string_properties.iter())
+        StringProperties(self.string_properties.0.iter())
     }
 
     /// An iterator visiting all string keys in arbitrary order. The iterator element type is `&'a RcString`.
     ///
     /// This iterator does not recurse down the prototype chain.
     #[inline]
     pub fn string_property_keys(&self) -> StringPropertyKeys<'_> {
-        StringPropertyKeys(self.string_properties.keys())
+        StringPropertyKeys(self.string_properties.0.keys())
     }
 
     /// An iterator visiting all string values in arbitrary order. The iterator element type is `&'a Property`.
     ///
     /// This iterator does not recurse down the prototype chain.
     #[inline]
     pub fn string_property_values(&self) -> StringPropertyValues<'_> {
-        StringPropertyValues(self.string_properties.values())
+        StringPropertyValues(self.string_properties.0.values())
     }
 
     #[inline]
     pub fn contains_key(&self, key: &PropertyKey) -> bool {
         match key {
             PropertyKey::Index(index) => self.indexed_properties.contains_key(index),
-            PropertyKey::String(string) => self.string_properties.contains_key(string),
-            PropertyKey::Symbol(symbol) => self.symbol_properties.contains_key(symbol),
+            PropertyKey::String(string) => self.string_properties.0.contains_key(string),
+            PropertyKey::Symbol(symbol) => self.symbol_properties.0.contains_key(symbol),
         }
     }
 }
@@ -162,8 +186,8 @@ impl PropertyMap {
 #[derive(Debug, Clone)]
 pub struct Iter<'a> {
     indexed_properties: hash_map::Iter<'a, u32, PropertyDescriptor>,
-    string_properties: hash_map::Iter<'a, JsString, PropertyDescriptor>,
-    symbol_properties: hash_map::Iter<'a, JsSymbol, PropertyDescriptor>,
+    string_properties: indexmap::map::Iter<'a, JsString, PropertyDescriptor>,
+    symbol_properties: indexmap::map::Iter<'a, JsSymbol, PropertyDescriptor>,
 }
 
 impl<'a> Iterator for Iter<'a> {
@@ -233,7 +257,7 @@ impl FusedIterator for Values<'_> {}
 
 /// An iterator over the `Symbol` property entries of an `Object`
 #[derive(Debug, Clone)]
-pub struct SymbolProperties<'a>(hash_map::Iter<'a, JsSymbol, PropertyDescriptor>);
+pub struct SymbolProperties<'a>(indexmap::map::Iter<'a, JsSymbol, PropertyDescriptor>);
 
 impl<'a> Iterator for SymbolProperties<'a> {
     type Item = (&'a JsSymbol, &'a PropertyDescriptor);
@@ -260,7 +284,7 @@ impl FusedIterator for SymbolProperties<'_> {}
 
 /// An iterator over the keys (`RcSymbol`) of an `Object`.
 #[derive(Debug, Clone)]
-pub struct SymbolPropertyKeys<'a>(hash_map::Keys<'a, JsSymbol, PropertyDescriptor>);
+pub struct SymbolPropertyKeys<'a>(indexmap::map::Keys<'a, JsSymbol, PropertyDescriptor>);
 
 impl<'a> Iterator for SymbolPropertyKeys<'a> {
     type Item = &'a JsSymbol;
@@ -287,7 +311,7 @@ impl FusedIterator for SymbolPropertyKeys<'_> {}
 
 /// An iterator over the `Symbol` values (`Property`) of an `Object`.
 #[derive(Debug, Clone)]
-pub struct SymbolPropertyValues<'a>(hash_map::Values<'a, JsSymbol, PropertyDescriptor>);
+pub struct SymbolPropertyValues<'a>(indexmap::map::Values<'a, JsSymbol, PropertyDescriptor>);
 
 impl<'a> Iterator for SymbolPropertyValues<'a> {
     type Item = &'a PropertyDescriptor;
@@ -395,7 +419,7 @@ impl FusedIterator for IndexPropertyValues<'_> {}
 
 /// An iterator over the `String` property entries of an `Object`
 #[derive(Debug, Clone)]
-pub struct StringProperties<'a>(hash_map::Iter<'a, JsString, PropertyDescriptor>);
+pub struct StringProperties<'a>(indexmap::map::Iter<'a, JsString, PropertyDescriptor>);
 
 impl<'a> Iterator for StringProperties<'a> {
     type Item = (&'a JsString, &'a PropertyDescriptor);
@@ -422,7 +446,7 @@ impl FusedIterator for StringProperties<'_> {}
 
 /// An iterator over the string keys (`RcString`) of an `Object`.
 #[derive(Debug, Clone)]
-pub struct StringPropertyKeys<'a>(hash_map::Keys<'a, JsString, PropertyDescriptor>);
+pub struct StringPropertyKeys<'a>(indexmap::map::Keys<'a, JsString, PropertyDescriptor>);
 
 impl<'a> Iterator for StringPropertyKeys<'a> {
     type Item = &'a JsString;
@@ -449,7 +473,7 @@ impl FusedIterator for StringPropertyKeys<'_> {}
 
 /// An iterator over the string values (`Property`) of an `Object`.
 #[derive(Debug, Clone)]
-pub struct StringPropertyValues<'a>(hash_map::Values<'a, JsString, PropertyDescriptor>);
+pub struct StringPropertyValues<'a>(indexmap::map::Values<'a, JsString, PropertyDescriptor>);
 
 impl<'a> Iterator for StringPropertyValues<'a> {
     type Item = &'a PropertyDescriptor;

boa/src/object/tests.rs

@@ -1,4 +1,4 @@
-use crate::exec;
+use crate::{check_output, exec, TestAction};
 
 #[test]
 fn ordinary_has_instance_nonobject_prototype() {
@@ -17,3 +17,30 @@ fn ordinary_has_instance_nonobject_prototype() {
         "\"TypeError: function has non-object prototype in instanceof check\""
     );
 }
+
+#[test]
+fn object_properties_return_order() {
+    let scenario = r#"
+        var o = {
+            p1: 'v1',
+            p2: 'v2',
+            p3: 'v3',
+        };
+        o.p4 = 'v4';
+        o[2] = 'iv2';
+        o[0] = 'iv0';
+        o[1] = 'iv1';
+        delete o.p1;
+        delete o.p3;
+        o.p1 = 'v1';
+        "#;
+
+    check_output(&[
+        TestAction::Execute(scenario),
+        TestAction::TestEq("Object.keys(o)", r#"[ "0", "1", "2", "p2", "p4", "p1" ]"#),
+        TestAction::TestEq(
+            "Object.values(o)",
+            r#"[ "iv0", "iv1", "iv2", "v2", "v4", "v1" ]"#,
+        ),
+    ]);
+}