Skip to content

Commit c9348c2

Browse files
epageepage
committed
fix: Proper equality for easy::Value
`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. Fixes toml-rs#194
1 parent c013445 commit c9348c2

File tree

7 files changed

+51
-225
lines changed

7 files changed

+51
-225
lines changed

‎Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ default = []
1919
easy = ["serde"]
2020

2121
[dependencies]
22-
linked-hash-map = "0.5.2"
22+
indexmap = "1.7"
2323
combine = "4.5.2"
2424
vec1 = "1"
2525
itertools = "0.10"

‎src/easy/de/inline_table.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use serde::de::IntoDeserializer;
33
use crate::easy::de::Error;
44

55
pub(crate) struct InlineTableMapAccess {
6-
iter: linked_hash_map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
6+
iter: indexmap::map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
77
value: Option<crate::Item>,
88
}
99

‎src/easy/de/table.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use serde::de::IntoDeserializer;
33
use crate::easy::de::Error;
44

55
pub(crate) struct TableMapAccess {
6-
iter: linked_hash_map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
6+
iter: indexmap::map::IntoIter<crate::repr::InternalString, crate::table::TableKeyValue>,
77
value: Option<crate::Item>,
88
}
99

‎src/easy/map.rs

+11-11
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use std::hash::Hash;
1414
use std::iter::FromIterator;
1515
use std::ops;
1616

17-
use linked_hash_map::{self, LinkedHashMap};
17+
use indexmap::map::IndexMap;
1818
use serde::{de, ser};
1919

2020
use crate::easy::value::Value;
@@ -24,7 +24,7 @@ pub struct Map<K, V> {
2424
map: MapImpl<K, V>,
2525
}
2626

27-
type MapImpl<K, V> = LinkedHashMap<K, V>;
27+
type MapImpl<K, V> = IndexMap<K, V>;
2828

2929
impl Map<String, Value> {
3030
/// Makes a new empty Map.
@@ -39,7 +39,7 @@ impl Map<String, Value> {
3939
#[inline]
4040
pub fn with_capacity(capacity: usize) -> Self {
4141
Map {
42-
map: LinkedHashMap::with_capacity(capacity),
42+
map: IndexMap::with_capacity(capacity),
4343
}
4444
}
4545

@@ -120,7 +120,7 @@ impl Map<String, Value> {
120120
where
121121
S: Into<String>,
122122
{
123-
use linked_hash_map::Entry as EntryImpl;
123+
use indexmap::map::Entry as EntryImpl;
124124

125125
match self.map.entry(key.into()) {
126126
EntryImpl::Vacant(vacant) => Entry::Vacant(VacantEntry { vacant }),
@@ -367,9 +367,9 @@ pub struct OccupiedEntry<'a> {
367367
occupied: OccupiedEntryImpl<'a>,
368368
}
369369

370-
type VacantEntryImpl<'a> = linked_hash_map::VacantEntry<'a, String, Value>;
370+
type VacantEntryImpl<'a> = indexmap::map::VacantEntry<'a, String, Value>;
371371

372-
type OccupiedEntryImpl<'a> = linked_hash_map::OccupiedEntry<'a, String, Value>;
372+
type OccupiedEntryImpl<'a> = indexmap::map::OccupiedEntry<'a, String, Value>;
373373

374374
impl<'a> Entry<'a> {
375375
/// Returns a reference to this entry's key.
@@ -476,7 +476,7 @@ pub struct Iter<'a> {
476476
iter: IterImpl<'a>,
477477
}
478478

479-
type IterImpl<'a> = linked_hash_map::Iter<'a, String, Value>;
479+
type IterImpl<'a> = indexmap::map::Iter<'a, String, Value>;
480480

481481
delegate_iterator!((Iter<'a>) => (&'a String, &'a Value));
482482

@@ -498,7 +498,7 @@ pub struct IterMut<'a> {
498498
iter: IterMutImpl<'a>,
499499
}
500500

501-
type IterMutImpl<'a> = linked_hash_map::IterMut<'a, String, Value>;
501+
type IterMutImpl<'a> = indexmap::map::IterMut<'a, String, Value>;
502502

503503
delegate_iterator!((IterMut<'a>) => (&'a String, &'a mut Value));
504504

@@ -520,7 +520,7 @@ pub struct IntoIter {
520520
iter: IntoIterImpl,
521521
}
522522

523-
type IntoIterImpl = linked_hash_map::IntoIter<String, Value>;
523+
type IntoIterImpl = indexmap::map::IntoIter<String, Value>;
524524

525525
delegate_iterator!((IntoIter) => (String, Value));
526526

@@ -531,7 +531,7 @@ pub struct Keys<'a> {
531531
iter: KeysImpl<'a>,
532532
}
533533

534-
type KeysImpl<'a> = linked_hash_map::Keys<'a, String, Value>;
534+
type KeysImpl<'a> = indexmap::map::Keys<'a, String, Value>;
535535

536536
delegate_iterator!((Keys<'a>) => &'a String);
537537

@@ -542,6 +542,6 @@ pub struct Values<'a> {
542542
iter: ValuesImpl<'a>,
543543
}
544544

545-
type ValuesImpl<'a> = linked_hash_map::Values<'a, String, Value>;
545+
type ValuesImpl<'a> = indexmap::map::Values<'a, String, Value>;
546546

547547
delegate_iterator!((Values<'a>) => &'a Value);

‎src/inline_table.rs

+17-24
Original file line numberDiff line numberDiff line change
@@ -76,22 +76,15 @@ impl InlineTable {
7676

7777
/// Sorts the key/value pairs by key.
7878
pub fn sort_values(&mut self) {
79-
let mut keys: Vec<InternalString> = self
80-
.items
81-
.iter_mut()
82-
.filter_map(|(key, kv)| match &mut kv.value {
79+
// Assuming standard tables have their position set and this won't negatively impact them
80+
self.items.sort_keys();
81+
for kv in self.items.values_mut() {
82+
match &mut kv.value {
8383
Item::Value(Value::InlineTable(table)) if table.is_dotted() => {
8484
table.sort_values();
85-
Some(key)
8685
}
87-
Item::Value(_) => Some(key),
88-
_ => None,
89-
})
90-
.cloned()
91-
.collect();
92-
keys.sort();
93-
for key in keys {
94-
self.items.get_refresh(&key);
86+
_ => {}
87+
}
9588
}
9689
}
9790

@@ -166,7 +159,7 @@ impl InlineTable {
166159
pub fn entry<'a>(&'a mut self, key: &str) -> InlineEntry<'a> {
167160
// Accept a `&str` rather than an owned type to keep `InternalString`, well, internal
168161
match self.items.entry(key.to_owned()) {
169-
linked_hash_map::Entry::Occupied(mut entry) => {
162+
indexmap::map::Entry::Occupied(mut entry) => {
170163
// Ensure it is a `Value` to simplify `InlineOccupiedEntry`'s code.
171164
let mut scratch = Item::None;
172165
std::mem::swap(&mut scratch, &mut entry.get_mut().value);
@@ -181,7 +174,7 @@ impl InlineTable {
181174

182175
InlineEntry::Occupied(InlineOccupiedEntry { entry })
183176
}
184-
linked_hash_map::Entry::Vacant(entry) => {
177+
indexmap::map::Entry::Vacant(entry) => {
185178
InlineEntry::Vacant(InlineVacantEntry { entry, key: None })
186179
}
187180
}
@@ -191,7 +184,7 @@ impl InlineTable {
191184
pub fn entry_format<'a>(&'a mut self, key: &'a Key) -> InlineEntry<'a> {
192185
// Accept a `&Key` to be consistent with `entry`
193186
match self.items.entry(key.get().to_owned()) {
194-
linked_hash_map::Entry::Occupied(mut entry) => {
187+
indexmap::map::Entry::Occupied(mut entry) => {
195188
// Ensure it is a `Value` to simplify `InlineOccupiedEntry`'s code.
196189
let mut scratch = Item::None;
197190
std::mem::swap(&mut scratch, &mut entry.get_mut().value);
@@ -206,7 +199,7 @@ impl InlineTable {
206199

207200
InlineEntry::Occupied(InlineOccupiedEntry { entry })
208201
}
209-
linked_hash_map::Entry::Vacant(entry) => InlineEntry::Vacant(InlineVacantEntry {
202+
indexmap::map::Entry::Vacant(entry) => InlineEntry::Vacant(InlineVacantEntry {
210203
entry,
211204
key: Some(key),
212205
}),
@@ -263,13 +256,13 @@ impl InlineTable {
263256
/// Removes an item given the key.
264257
pub fn remove(&mut self, key: &str) -> Option<Value> {
265258
self.items
266-
.remove(key)
259+
.shift_remove(key)
267260
.and_then(|kv| kv.value.into_value().ok())
268261
}
269262

270263
/// Removes a key from the map, returning the stored key and value if the key was previously in the map.
271264
pub fn remove_entry(&mut self, key: &str) -> Option<(Key, Value)> {
272-
self.items.remove(key).and_then(|kv| {
265+
self.items.shift_remove(key).and_then(|kv| {
273266
let key = kv.key;
274267
kv.value.into_value().ok().map(|value| (key, value))
275268
})
@@ -426,9 +419,9 @@ impl<'a> InlineEntry<'a> {
426419
}
427420
}
428421

429-
/// A view into a single occupied location in a `LinkedHashMap`.
422+
/// A view into a single occupied location in a `IndexMap`.
430423
pub struct InlineOccupiedEntry<'a> {
431-
entry: linked_hash_map::OccupiedEntry<'a, InternalString, TableKeyValue>,
424+
entry: indexmap::map::OccupiedEntry<'a, InternalString, TableKeyValue>,
432425
}
433426

434427
impl<'a> InlineOccupiedEntry<'a> {
@@ -472,13 +465,13 @@ impl<'a> InlineOccupiedEntry<'a> {
472465

473466
/// Takes the value out of the entry, and returns it
474467
pub fn remove(self) -> Value {
475-
self.entry.remove().value.into_value().unwrap()
468+
self.entry.shift_remove().value.into_value().unwrap()
476469
}
477470
}
478471

479-
/// A view into a single empty location in a `LinkedHashMap`.
472+
/// A view into a single empty location in a `IndexMap`.
480473
pub struct InlineVacantEntry<'a> {
481-
entry: linked_hash_map::VacantEntry<'a, InternalString, TableKeyValue>,
474+
entry: indexmap::map::VacantEntry<'a, InternalString, TableKeyValue>,
482475
key: Option<&'a Key>,
483476
}
484477

‎src/table.rs

+19-28
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::iter::FromIterator;
22

3-
use linked_hash_map::LinkedHashMap;
3+
use indexmap::map::IndexMap;
44

55
use crate::key::Key;
66
use crate::repr::{Decor, InternalString};
@@ -104,22 +104,15 @@ impl Table {
104104
///
105105
/// Doesn't affect subtables or subarrays.
106106
pub fn sort_values(&mut self) {
107-
let mut keys: Vec<InternalString> = self
108-
.items
109-
.iter_mut()
110-
.filter_map(|(key, kv)| match &mut kv.value {
107+
// Assuming standard tables have their position set and this won't negatively impact them
108+
self.items.sort_keys();
109+
for kv in self.items.values_mut() {
110+
match &mut kv.value {
111111
Item::Table(table) if table.is_dotted() => {
112112
table.sort_values();
113-
Some(key)
114113
}
115-
Item::Value(_) => Some(key),
116-
_ => None,
117-
})
118-
.cloned()
119-
.collect();
120-
keys.sort();
121-
for key in keys {
122-
self.items.get_refresh(&key);
114+
_ => {}
115+
}
123116
}
124117
}
125118

@@ -230,19 +223,17 @@ impl Table {
230223
pub fn entry<'a>(&'a mut self, key: &str) -> Entry<'a> {
231224
// Accept a `&str` rather than an owned type to keep `InternalString`, well, internal
232225
match self.items.entry(key.to_owned()) {
233-
linked_hash_map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
234-
linked_hash_map::Entry::Vacant(entry) => {
235-
Entry::Vacant(VacantEntry { entry, key: None })
236-
}
226+
indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
227+
indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry { entry, key: None }),
237228
}
238229
}
239230

240231
/// Gets the given key's corresponding entry in the Table for in-place manipulation.
241232
pub fn entry_format<'a>(&'a mut self, key: &'a Key) -> Entry<'a> {
242233
// Accept a `&Key` to be consistent with `entry`
243234
match self.items.entry(key.get().to_owned()) {
244-
linked_hash_map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
245-
linked_hash_map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry {
235+
indexmap::map::Entry::Occupied(entry) => Entry::Occupied(OccupiedEntry { entry }),
236+
indexmap::map::Entry::Vacant(entry) => Entry::Vacant(VacantEntry {
246237
entry,
247238
key: Some(key),
248239
}),
@@ -311,12 +302,12 @@ impl Table {
311302

312303
/// Removes an item given the key.
313304
pub fn remove(&mut self, key: &str) -> Option<Item> {
314-
self.items.remove(key).map(|kv| kv.value)
305+
self.items.shift_remove(key).map(|kv| kv.value)
315306
}
316307

317308
/// Removes a key from the map, returning the stored key and value if the key was previously in the map.
318309
pub fn remove_entry(&mut self, key: &str) -> Option<(Key, Item)> {
319-
self.items.remove(key).map(|kv| (kv.key, kv.value))
310+
self.items.shift_remove(key).map(|kv| (kv.key, kv.value))
320311
}
321312
}
322313

@@ -360,7 +351,7 @@ impl<'s> IntoIterator for &'s Table {
360351
}
361352
}
362353

363-
pub(crate) type KeyValuePairs = LinkedHashMap<InternalString, TableKeyValue>;
354+
pub(crate) type KeyValuePairs = IndexMap<InternalString, TableKeyValue>;
364355

365356
fn decorate_table(table: &mut Table) {
366357
for (key_decor, value) in table
@@ -508,9 +499,9 @@ impl<'a> Entry<'a> {
508499
}
509500
}
510501

511-
/// A view into a single occupied location in a `LinkedHashMap`.
502+
/// A view into a single occupied location in a `IndexMap`.
512503
pub struct OccupiedEntry<'a> {
513-
entry: linked_hash_map::OccupiedEntry<'a, InternalString, TableKeyValue>,
504+
entry: indexmap::map::OccupiedEntry<'a, InternalString, TableKeyValue>,
514505
}
515506

516507
impl<'a> OccupiedEntry<'a> {
@@ -553,13 +544,13 @@ impl<'a> OccupiedEntry<'a> {
553544

554545
/// Takes the value out of the entry, and returns it
555546
pub fn remove(self) -> Item {
556-
self.entry.remove().value
547+
self.entry.shift_remove().value
557548
}
558549
}
559550

560-
/// A view into a single empty location in a `LinkedHashMap`.
551+
/// A view into a single empty location in a `IndexMap`.
561552
pub struct VacantEntry<'a> {
562-
entry: linked_hash_map::VacantEntry<'a, InternalString, TableKeyValue>,
553+
entry: indexmap::map::VacantEntry<'a, InternalString, TableKeyValue>,
563554
key: Option<&'a Key>,
564555
}
565556

0 commit comments

Comments
 (0)