fix: Proper equality for easy::Value (#196)

`LinkedHashMap`s equality checked order when we don't want it to.  It
also isn't maintained.  So we're switching to `IndexMap`

Unfortunately, there are other ordering issues in the relevant test that
makes it hard to get right, so went ahead and removed it.

Unfortunately, I didn't see any change in performance.

Fixes #194

Cargo.toml

@@ -19,7 +19,7 @@ default = []
 easy = ["serde"]
 
 [dependencies]
-linked-hash-map = "0.5.2"
+indexmap = "1.7"
 combine = "4.5.2"
 vec1 = "1"
 itertools = "0.10"

src/easy/de/inline_table.rs

@@ -3,7 +3,7 @@ use serde::de::IntoDeserializer;
 use crate::easy::de::Error;
 
 pub(crate) struct InlineTableMapAccess {
-    iter: linked_hash_map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
+    iter: indexmap::map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
     value: Option<crate::Item>,
 }
 

src/easy/de/table.rs

@@ -3,7 +3,7 @@ use serde::de::IntoDeserializer;
 use crate::easy::de::Error;
 
 pub(crate) struct TableMapAccess {
-    iter: linked_hash_map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
+    iter: indexmap::map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
     value: Option<crate::Item>,
 }
 

src/easy/map.rs

@@ -14,7 +14,7 @@ use std::hash::Hash;
 use std::iter::FromIterator;
 use std::ops;
 
-use linked_hash_map::{self, LinkedHashMap};
+use indexmap::map::IndexMap;
 use serde::{de, ser};
 
 use crate::easy::value::Value;
@@ -24,7 +24,7 @@ pub struct Map<K, V> {
     map: MapImpl<K, V>,
 }
 
-type MapImpl<K, V> = LinkedHashMap<K, V>;
+type MapImpl<K, V> = IndexMap<K, V>;
 
 impl Map<String, Value> {
     /// Makes a new empty Map.
@@ -39,7 +39,7 @@ impl Map<String, Value> {
     #[inline]
     pub fn with_capacity(capacity: usize) -> Self {
         Map {
-            map: LinkedHashMap::with_capacity(capacity),
+            map: IndexMap::with_capacity(capacity),
         }
     }
 
@@ -120,7 +120,7 @@ impl Map<String, Value> {
     where
         S: Into<String>,
     {
-        use linked_hash_map::Entry as EntryImpl;
+        use indexmap::map::Entry as EntryImpl;
 
         match self.map.entry(key.into()) {
             EntryImpl::Vacant(vacant) => Entry::Vacant(VacantEntry { vacant }),
@@ -367,9 +367,9 @@ pub struct OccupiedEntry<'a> {
     occupied: OccupiedEntryImpl<'a>,
 }
 
-type VacantEntryImpl<'a> = linked_hash_map::VacantEntry<'a, String, Value>;
+type VacantEntryImpl<'a> = indexmap::map::VacantEntry<'a, String, Value>;
 
-type OccupiedEntryImpl<'a> = linked_hash_map::OccupiedEntry<'a, String, Value>;
+type OccupiedEntryImpl<'a> = indexmap::map::OccupiedEntry<'a, String, Value>;
 
 impl<'a> Entry<'a> {
     /// Returns a reference to this entry's key.
@@ -476,7 +476,7 @@ pub struct Iter<'a> {
     iter: IterImpl<'a>,
 }
 
-type IterImpl<'a> = linked_hash_map::Iter<'a, String, Value>;
+type IterImpl<'a> = indexmap::map::Iter<'a, String, Value>;
 
 delegate_iterator!((Iter<'a>) => (&'a String, &'a Value));
 
@@ -498,7 +498,7 @@ pub struct IterMut<'a> {
     iter: IterMutImpl<'a>,
 }
 
-type IterMutImpl<'a> = linked_hash_map::IterMut<'a, String, Value>;
+type IterMutImpl<'a> = indexmap::map::IterMut<'a, String, Value>;
 
 delegate_iterator!((IterMut<'a>) => (&'a String, &'a mut Value));
 
@@ -520,7 +520,7 @@ pub struct IntoIter {
     iter: IntoIterImpl,
 }
 
-type IntoIterImpl = linked_hash_map::IntoIter<String, Value>;
+type IntoIterImpl = indexmap::map::IntoIter<String, Value>;
 
 delegate_iterator!((IntoIter) => (String, Value));
 
@@ -531,7 +531,7 @@ pub struct Keys<'a> {
     iter: KeysImpl<'a>,
 }
 
-type KeysImpl<'a> = linked_hash_map::Keys<'a, String, Value>;
+type KeysImpl<'a> = indexmap::map::Keys<'a, String, Value>;
 
 delegate_iterator!((Keys<'a>) => &'a String);
 
@@ -542,6 +542,6 @@ pub struct Values<'a> {
     iter: ValuesImpl<'a>,
 }
 
-type ValuesImpl<'a> = linked_hash_map::Values<'a, String, Value>;
+type ValuesImpl<'a> = indexmap::map::Values<'a, String, Value>;
 
 delegate_iterator!((Values<'a>) => &'a Value);

src/inline_table.rs

@@ -76,22 +76,15 @@ impl InlineTable {
 
     /// Sorts the key/value pairs by key.
     pub fn sort_values(&mut self) {
-        let mut keys: Vec<InternalString> = self
-            .items
-            .iter_mut()
-            .filter_map(|(key, kv)| match &mut kv.value {
+        // Assuming standard tables have their position set and this won't negatively impact them
+        self.items.sort_keys();
+        for kv in self.items.values_mut() {
+            match &mut kv.value {
                 Item::Value(Value::InlineTable(table)) if table.is_dotted() => {
                     table.sort_values();
-                    Some(key)
                 }
-                Item::Value(_) => Some(key),
-                _ => None,
-            })
-            .cloned()
-            .collect();
-        keys.sort();
-        for key in keys {
-            self.items.get_refresh(&key);
+                _ => {}
+            }
         }
     }
 
@@ -166,7 +159,7 @@ impl InlineTable {
     pub fn entry<'a>(&'a mut self, key: &str) -> InlineEntry<'a> {
         // Accept a `&str` rather than an owned type to keep `InternalString`, well, internal
         match self.items.entry(key.to_owned()) {
-            linked_hash_map::Entry::Occupied(mut entry) => {
+            indexmap::map::Entry::Occupied(mut entry) => {
                 // Ensure it is a `Value` to simplify `InlineOccupiedEntry`'s code.
                 let mut scratch = Item::None;
                 std::mem::swap(&mut scratch, &mut entry.get_mut().value);
@@ -181,7 +174,7 @@ impl InlineTable {
 
                 InlineEntry::Occupied(InlineOccupiedEntry { entry })
             }
-            linked_hash_map::Entry::Vacant(entry) => {
+            indexmap::map::Entry::Vacant(entry) => {
                 InlineEntry::Vacant(InlineVacantEntry { entry, key: None })
             }
         }
@@ -191,7 +184,7 @@ impl InlineTable {
     pub fn entry_format<'a>(&'a mut self, key: &'a Key) -> InlineEntry<'a> {
         // Accept a `&Key` to be consistent with `entry`
         match self.items.entry(key.get().to_owned()) {
-            linked_hash_map::Entry::Occupied(mut entry) => {
+            indexmap::map::Entry::Occupied(mut entry) => {
                 // Ensure it is a `Value` to simplify `InlineOccupiedEntry`'s code.
                 let mut scratch = Item::None;
                 std::mem::swap(&mut scratch, &mut entry.get_mut().value);
@@ -206,7 +199,7 @@ impl InlineTable {
 
                 InlineEntry::Occupied(InlineOccupiedEntry { entry })
             }
-            linked_hash_map::Entry::Vacant(entry) => InlineEntry::Vacant(InlineVacantEntry {
+            indexmap::map::Entry::Vacant(entry) => InlineEntry::Vacant(InlineVacantEntry {
                 entry,
                 key: Some(key),
             }),
@@ -263,13 +256,13 @@ impl InlineTable {
     /// Removes an item given the key.
     pub fn remove(&mut self, key: &str) -> Option<Value> {
         self.items
-            .remove(key)
+            .shift_remove(key)
             .and_then(|kv| kv.value.into_value().ok())
     }
 
     /// Removes a key from the map, returning the stored key and value if the key was previously in the map.
     pub fn remove_entry(&mut self, key: &str) -> Option<(Key, Value)> {
-        self.items.remove(key).and_then(|kv| {
+        self.items.shift_remove(key).and_then(|kv| {
             let key = kv.key;
             kv.value.into_value().ok().map(|value| (key, value))
         })
@@ -426,9 +419,9 @@ impl<'a> InlineEntry<'a> {
     }
 }
 
-/// A view into a single occupied location in a `LinkedHashMap`.
+/// A view into a single occupied location in a `IndexMap`.
 pub struct InlineOccupiedEntry<'a> {
-    entry: linked_hash_map::OccupiedEntry<'a, InternalString, TableKeyValue>,
+    entry: indexmap::map::OccupiedEntry<'a, InternalString, TableKeyValue>,
 }
 
 impl<'a> InlineOccupiedEntry<'a> {
@@ -472,13 +465,13 @@ impl<'a> InlineOccupiedEntry<'a> {
 
     /// Takes the value out of the entry, and returns it
     pub fn remove(self) -> Value {
-        self.entry.remove().value.into_value().unwrap()
+        self.entry.shift_remove().value.into_value().unwrap()
     }
 }
 
-/// A view into a single empty location in a `LinkedHashMap`.
+/// A view into a single empty location in a `IndexMap`.
 pub struct InlineVacantEntry<'a> {
-    entry: linked_hash_map::VacantEntry<'a, InternalString, TableKeyValue>,
+    entry: indexmap::map::VacantEntry<'a, InternalString, TableKeyValue>,
     key: Option<&'a Key>,
 }
 

src/table.rs

@@ -1,6 +1,6 @@
 use std::iter::FromIterator;
 
-use linked_hash_map::LinkedHashMap;
+use indexmap::map::IndexMap;
 
 use crate::key::Key;
 use crate::repr::{Decor, InternalString};
@@ -104,22 +104,15 @@ impl Table {
     ///
     /// Doesn't affect subtables or subarrays.
     pub fn sort_values(&mut self) {
-        let mut keys: Vec<InternalString> = self
-            .items
-            .iter_mut()
-            .filter_map(|(key, kv)| match &mut kv.value {
+        // Assuming standard tables have their position set and this won't negatively impact them
+        self.items.sort_keys();
+        for kv in self.items.values_mut() {
+            match &mut kv.value {
                 Item::Table(table) if table.is_dotted() => {
                     table.sort_values();
-                    Some(key)
                 }
-                Item::Value(_) => Some(key),
-                _ => None,
-            })
-            .cloned()
-            .collect();
-        keys.sort();
-        for key in keys {
-            self.items.get_refresh(&key);
+                _ => {}
+            }
         }
     }
 
@@ -230,19 +223,17 @@ impl Table {
     pub fn entry<'a>(&'a mut self, key: &str) -> Entry<'a> {
         // Accept a `&str` rather than an owned type to keep `InternalString`, well, internal
         match self.items.entry(key.to_owned()) {
-            linked_hash_map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
-            linked_hash_map::Entry::Vacant(entry) => {
-                Entry::Vacant(VacantEntry { entry, key: None })
-            }
+            indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
+            indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry { entry, key: None }),
         }
     }
 
     /// Gets the given key's corresponding entry in the Table for in-place manipulation.
     pub fn entry_format<'a>(&'a mut self, key: &'a Key) -> Entry<'a> {
         // Accept a `&Key` to be consistent with `entry`
         match self.items.entry(key.get().to_owned()) {
-            linked_hash_map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
-            linked_hash_map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry {
+            indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
+            indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry {
                 entry,
                 key: Some(key),
             }),
@@ -311,12 +302,12 @@ impl Table {
 
     /// Removes an item given the key.
     pub fn remove(&mut self, key: &str) -> Option<Item> {
-        self.items.remove(key).map(|kv| kv.value)
+        self.items.shift_remove(key).map(|kv| kv.value)
     }
 
     /// Removes a key from the map, returning the stored key and value if the key was previously in the map.
     pub fn remove_entry(&mut self, key: &str) -> Option<(Key, Item)> {
-        self.items.remove(key).map(|kv| (kv.key, kv.value))
+        self.items.shift_remove(key).map(|kv| (kv.key, kv.value))
     }
 }
 
@@ -360,7 +351,7 @@ impl<'s> IntoIterator for &'s Table {
     }
 }
 
-pub(crate) type KeyValuePairs = LinkedHashMap<InternalString, TableKeyValue>;
+pub(crate) type KeyValuePairs = IndexMap<InternalString, TableKeyValue>;
 
 fn decorate_table(table: &mut Table) {
     for (key_decor, value) in table
@@ -508,9 +499,9 @@ impl<'a> Entry<'a> {
     }
 }
 
-/// A view into a single occupied location in a `LinkedHashMap`.
+/// A view into a single occupied location in a `IndexMap`.
 pub struct OccupiedEntry<'a> {
-    entry: linked_hash_map::OccupiedEntry<'a, InternalString, TableKeyValue>,
+    entry: indexmap::map::OccupiedEntry<'a, InternalString, TableKeyValue>,
 }
 
 impl<'a> OccupiedEntry<'a> {
@@ -553,13 +544,13 @@ impl<'a> OccupiedEntry<'a> {
 
     /// Takes the value out of the entry, and returns it
     pub fn remove(self) -> Item {
-        self.entry.remove().value
+        self.entry.shift_remove().value
     }
 }
 
-/// A view into a single empty location in a `LinkedHashMap`.
+/// A view into a single empty location in a `IndexMap`.
 pub struct VacantEntry<'a> {
-    entry: linked_hash_map::VacantEntry<'a, InternalString, TableKeyValue>,
+    entry: indexmap::map::VacantEntry<'a, InternalString, TableKeyValue>,
     key: Option<&'a Key>,
 }