Additional operations for manipulating iteration order

[allen-marshall]

This PR adds operations to IndexMap (and IndexSet) that allow an entry to be moved to a specific position in the iteration order, while preserving the order of all other entries. The operation's average computation time is proportional to the distance between the entry's current position and the new position, assuming that probing the hash table takes constant time on average.

The PR requires an upgrade to Rust version 1.26, because it relies on the rotate_left and rotate_right methods of the slice type. If upgrading to 1.26 is deemed inappropriate at this time, it may be necessary to delay merging this PR.

The new operations also may allow for an alternative implementation of ordered_remove (from PR #15), in which the element being removed is first moved to the end of the ordering, then IndexMap::pop() is called. This might be more efficient if the element to be removed is already close to the end. I haven't included that idea in the PR, but it could be considered if the PR is accepted.

Please let me know if you find any problems in the PR that I need to fix. One thing I am unsure about is whether returning Option<()> is or is not an appropriate/idiomatic way of supporting error detection in a method that wouldn't normally return anything.

Use Case

My use case for this PR is that I am planning to create a GUI application where the user can manage a list of items, including reordering the items by dragging and dropping an item in the list. Internally, the items may reference each other by a unique key. Ideally, I want to be able to look up an item efficiently by both its numerical index and its unique key. IndexMap seems like a good fit for this use case, and this PR would make it easier for me to implement the reordering through drag and drop.

[samlich]

Is there anything preventing this from being merged?

I've updated the changes below.

diff --git a/src/lib.rs b/src/lib.rs
index b9758c9..de7208a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -26,7 +26,7 @@
 //!
 //! ### Rust Version
 //!
-//! This version of indexmap requires Rust 1.18 or later, or 1.32+ for
+//! This version of indexmap requires Rust 1.26 or later, or 1.32+ for
 //! development builds, and Rust 1.36+ for using with `alloc` (without `std`),
 //! see below.
 //!
diff --git a/src/map.rs b/src/map.rs
index 1f026fd..8ae5dda 100644
--- a/src/map.rs
+++ b/src/map.rs
@@ -530,6 +530,53 @@ where
         self.core.retain_in_order(keep);
     }
 
+    /// Move an existing entry to a specified position in the map's ordering,
+    /// while maintaining the order of all other entries.
+    ///
+    /// Return `Some(())` on success, `None` on failure. Failure occurs if no
+    /// entry with the specified key exists in the map, or if `new_index` is not
+    /// a valid index for the map.
+    ///
+    /// + `key`: Key of the entry whose position in the map's ordering is to be
+    ///   changed.
+    /// + `new_index`: Numerical index indicating the new position to which the
+    ///   entry should be moved, within the map's ordering.
+    ///
+    /// Computes in **O(|curr_index - new_index| + 1)** time (average), where
+    /// `curr_index` is the numerical index of the entry before the change.
+    pub fn reorder_entry<Q: ?Sized>(&mut self, key: &Q, new_index: usize) -> Option<()>
+    where
+        Q: Hash + Equivalent<K>,
+    {
+        if new_index >= self.len() {
+            return None;
+        }
+
+        let curr_index = self.get_full(key)?.0;
+        self.reorder_entry_index(curr_index, new_index)
+    }
+
+    /// Move an existing entry to a specified position in the map's ordering,
+    /// while maintaining the order of all other entries.
+    ///
+    /// Return `Some(())` on success, `None` on failure. Failure occurs if the
+    /// provided index values are not valid indices for the map.
+    ///
+    /// + `curr_index`: Numerical index indicating the current position of the
+    ///   entry to be moved, within the map's ordering.
+    /// + `new_index`: Numerical index indicating the new position to which the
+    ///   entry should be moved, within the map's ordering.
+    ///
+    /// Computes in **O(|curr_index - new_index| + 1)** time (average).
+    pub fn reorder_entry_index(&mut self, curr_index: usize, new_index: usize) -> Option<()> {
+        if curr_index >= self.len() || new_index >= self.len() {
+            return None;
+        }
+
+        self.core.reorder_entry_found(curr_index, new_index);
+        Some(())
+    }
+
     /// Sort the map’s key-value pairs by the default ordering of the keys.
     ///
     /// See `sort_by` for details.
@@ -1074,6 +1121,20 @@ mod tests {
     use super::*;
     use util::enumerate;
 
+    /// Convert an IndexMap to a vector of (key, value) entries, in the map's
+    /// iteration order. Keys and values are cloned so that the vector can own
+    /// them without modifying the IndexMap.
+    fn map_to_vec<K, V, S>(map: &IndexMap<K, V, S>) -> Vec<(K, V)>
+    where
+        K: Hash + Eq + Clone,
+        V: Clone,
+        S: BuildHasher,
+    {
+        map.iter()
+            .map(|(key_ref, value_ref)| (key_ref.clone(), value_ref.clone()))
+            .collect()
+    }
+
     #[test]
     fn it_works() {
         let mut map = IndexMap::new();
@@ -1281,6 +1342,92 @@ mod tests {
         }
     }
 
+    #[test]
+    fn reorder_entry() {
+        let insert = [0, 4, 2, 12, 8, 7, 11];
+        let mut map = IndexMap::new();
+
+        for &elt in &insert {
+            map.insert(elt, elt * 2);
+        }
+
+        let expected_vec_0 = vec![(0, 0), (4, 8), (2, 4), (12, 24), (8, 16), (7, 14), (11, 22)];
+        assert_eq!(expected_vec_0, map_to_vec(&map));
+
+        // Try moving entries to different positions.
+        map.reorder_entry(&12, 0).unwrap();
+        map.reorder_entry(&12, 4).unwrap();
+        map.reorder_entry(&4, 6).unwrap();
+        map.reorder_entry(&4, 4).unwrap();
+        map.reorder_entry(&0, 5).unwrap();
+        let expected_vec_1 = vec![(2, 4), (8, 16), (12, 24), (4, 8), (7, 14), (0, 0), (11, 22)];
+        assert_eq!(expected_vec_1, map_to_vec(&map));
+
+        // Try moving entries to where they already are, and check that this
+        // doesn't change anything.
+        map.reorder_entry(&12, 2).unwrap();
+        map.reorder_entry(&0, 5).unwrap();
+        map.reorder_entry(&11, 6).unwrap();
+        map.reorder_entry(&7, 4).unwrap();
+        assert_eq!(expected_vec_1, map_to_vec(&map));
+    }
+
+    #[test]
+    fn reorder_entry_index() {
+        let insert = [0, 4, 2, 12, 8, 7, 11];
+        let mut map = IndexMap::new();
+
+        for &elt in &insert {
+            map.insert(elt, elt * 2);
+        }
+
+        let expected_vec_0 = vec![(0, 0), (4, 8), (2, 4), (12, 24), (8, 16), (7, 14), (11, 22)];
+        assert_eq!(expected_vec_0, map_to_vec(&map));
+
+        // Try moving entries to different positions.
+        map.reorder_entry_index(3, 0).unwrap();
+        map.reorder_entry_index(0, 4).unwrap();
+        map.reorder_entry_index(1, 6).unwrap();
+        map.reorder_entry_index(6, 4).unwrap();
+        map.reorder_entry_index(0, 5).unwrap();
+        let expected_vec_1 = vec![(2, 4), (8, 16), (12, 24), (4, 8), (7, 14), (0, 0), (11, 22)];
+        assert_eq!(expected_vec_1, map_to_vec(&map));
+
+        // Try moving entries to where they already are, and check that this
+        // doesn't change anything.
+        map.reorder_entry_index(2, 2).unwrap();
+        map.reorder_entry_index(5, 5).unwrap();
+        map.reorder_entry_index(6, 6).unwrap();
+        map.reorder_entry_index(4, 4).unwrap();
+        assert_eq!(expected_vec_1, map_to_vec(&map));
+    }
+
+    #[test]
+    fn reorder_entry_failure() {
+        let insert = [0, 4, 2, 12, 8, 7, 11];
+        let mut map = IndexMap::new();
+
+        for &elt in &insert {
+            map.insert(elt, elt * 2);
+        }
+
+        // Try calling reorder_entry with nonexistent keys, using both valid and
+        // invalid indices.
+        assert_eq!(map.reorder_entry(&1, 0), None);
+        assert_eq!(map.reorder_entry(&5, 7), None);
+        assert_eq!(map.reorder_entry(&3, 10), None);
+
+        // Try calling reorder_entry with valid keys but invalid indices.
+        assert_eq!(map.reorder_entry(&2, 7), None);
+        assert_eq!(map.reorder_entry(&12, 10), None);
+
+        // Try calling reorder_entry_index with invalid indices.
+        assert_eq!(map.reorder_entry_index(0, 15), None);
+        assert_eq!(map.reorder_entry_index(1, 7), None);
+        assert_eq!(map.reorder_entry_index(7, 1), None);
+        assert_eq!(map.reorder_entry_index(8, 9), None);
+    }
+
     #[test]
     fn partial_eq_and_eq() {
         let mut map_a = IndexMap::new();
diff --git a/src/map_core.rs b/src/map_core.rs
index 2d8a74e..70a1add 100644
--- a/src/map_core.rs
+++ b/src/map_core.rs
@@ -14,7 +14,7 @@ use std::vec::Vec;
 
 use std::cmp::{max, Ordering};
 use std::fmt;
-use std::iter::FromIterator;
+use std::iter::{once, FromIterator};
 use std::marker::PhantomData;
 use std::mem::replace;
 use std::ops::RangeFull;
@@ -693,6 +693,121 @@ impl<K, V> IndexMapCore<K, V> {
         (probe, actual_pos)
     }
 
+    /// Update self.indices to reflect a change in the ordering of the entries
+    /// in self.entries.
+    ///
+    /// + `indices_to_update`: An iterator of indices into self.indices
+    ///   indicating the slots where a corresponding entry in self.entries has
+    ///   changed position. If the iterator returns an index pointing to an
+    ///   empty slot in self.indices, that slot will be skipped and not updated.
+    /// + `new_positions_to_store`: An iterator of indices into self.entries
+    ///   indicating the new positions to which the referenced slots in
+    ///   self.indices should point after the update. Must contain the same
+    ///   number of items as indices_to_update.
+    fn update_indices<I, J>(&mut self, indices_to_update: I, new_positions_to_store: J)
+    where
+        I: Iterator<Item = usize>,
+        J: Iterator<Item = usize>,
+    {
+        dispatch_32_vs_64!(
+            self.update_indices_impl::<I, J>(indices_to_update, new_positions_to_store)
+        )
+    }
+
+    fn update_indices_impl<Sz, I, J>(&mut self, indices_to_update: I, new_positions_to_store: J)
+    where
+        Sz: Size,
+        I: Iterator<Item = usize>,
+        J: Iterator<Item = usize>,
+    {
+        for (index_to_update, new_position_to_store) in
+            indices_to_update.zip(new_positions_to_store)
+        {
+            let pos = &mut self.indices[index_to_update];
+            if !pos.is_none() {
+                pos.set_pos::<Sz>(new_position_to_store);
+            }
+        }
+    }
+
+    /// Probe self.indices to find the slot that points to the specified
+    /// position in self.entries.
+    ///
+    /// Return an index into self.indices indicating the location of the
+    /// matching slot that was found.
+    ///
+    /// + `index`: The position value to look for in self.indices. This method
+    ///   will probe until it finds a slot in self.indices that points to this
+    ///   position in self.entries. Must be a valid index for self.entries.
+    fn probe_to_referenced_pos(&self, index: usize) -> usize {
+        dispatch_32_vs_64!(self.probe_to_referenced_pos_impl(index))
+    }
+
+    fn probe_to_referenced_pos_impl<Sz>(&self, index: usize) -> usize
+    where
+        Sz: Size,
+    {
+        let mut probe = desired_pos(self.mask, self.entries[index].hash);
+        probe_loop!(probe < self.indices.len(), {
+            if let Some((stored_index, _)) = self.indices[probe].resolve::<Sz>() {
+                if stored_index == index {
+                    return probe;
+                }
+            }
+        })
+    }
+
+    /// Move an entry from its current index in the map's ordering to a
+    /// different index, while maintaining the order of all other entries.
+    ///
+    /// + `curr_index`: Numerical index indicating the current position of the
+    ///   entry to be moved, within the map's ordering. Must be a valid index
+    ///   for the map.
+    /// + `new_index`: Numerical index indicating the new position to which the
+    ///   entry should be moved, within the map's ordering. Must be a valid
+    ///   index for the map.
+    pub(crate) fn reorder_entry_found(&mut self, curr_index: usize, new_index: usize) {
+        if curr_index == new_index {
+            return;
+        }
+
+        // Compute information about the indices at which we are moving entries.
+        let moving_to_greater = new_index > curr_index;
+        let (low_index, high_index) = {
+            if moving_to_greater {
+                (curr_index, new_index)
+            } else {
+                (new_index, curr_index)
+            }
+        };
+
+        // Determine what indices in self.indices will need to be updated after
+        // the reordering. This is done before the reordering of self.entries so
+        // that the probing will start at the right hash values.
+        let indices_to_update: Vec<usize> = (low_index..(high_index + 1))
+            .map(|index| self.probe_to_referenced_pos(index))
+            .collect();
+
+        // Change the order in self.entries.
+        {
+            let slice = &mut self.entries[low_index..(high_index + 1)];
+            if moving_to_greater {
+                slice.rotate_left(1);
+            } else {
+                slice.rotate_right(1);
+            }
+        }
+
+        // Update the relevant elements of self.indices to match the new order.
+        if moving_to_greater {
+            let new_positions_to_store = once(high_index).chain(low_index..high_index);
+            self.update_indices(indices_to_update.into_iter(), new_positions_to_store);
+        } else {
+            let new_positions_to_store = ((low_index + 1)..(high_index + 1)).chain(once(low_index));
+            self.update_indices(indices_to_update.into_iter(), new_positions_to_store);
+        }
+    }
+
     /// Remove an entry by shifting all entries that follow it
     pub(crate) fn shift_remove_found(&mut self, probe: usize, found: usize) -> (K, V) {
         dispatch_32_vs_64!(self.shift_remove_found_impl(probe, found))
diff --git a/src/set.rs b/src/set.rs
index 1b90fb0..d361d5d 100644
--- a/src/set.rs
+++ b/src/set.rs
@@ -508,6 +508,42 @@ where
         self.map.retain(move |x, &mut ()| keep(x))
     }
 
+    /// Move an existing value to a specified position in the set's ordering,
+    /// while maintaining the order of all other values in the set.
+    ///
+    /// Return `Some(())` on success, `None` on failure. Failure occurs if the
+    /// specified value is not in the set, or if `new_index` is not a valid
+    /// index for the set.
+    ///
+    /// + `value`: Value whose position in the set's ordering is to be changed.
+    /// + `new_index`: Numerical index indicating the new position to which the
+    ///   value should be moved, within the set's ordering.
+    ///
+    /// Computes in **O(|curr_index - new_index| + 1)** time (average), where
+    /// `curr_index` is the numerical index of the value before the change.
+    pub fn reorder_value<Q: ?Sized>(&mut self, value: &Q, new_index: usize) -> Option<()>
+    where
+        Q: Hash + Equivalent<T>,
+    {
+        self.map.reorder_entry(value, new_index)
+    }
+
+    /// Move an existing value to a specified position in the set's ordering,
+    /// while maintaining the order of all other values in the set.
+    ///
+    /// Return `Some(())` on success, `None` on failure. Failure occurs if the
+    /// provided index values are not valid indices for the set.
+    ///
+    /// + `curr_index`: Numerical index indicating the current position of the
+    ///   value to be moved, within the set's ordering.
+    /// + `new_index`: Numerical index indicating the new position to which the
+    ///   value should be moved, within the set's ordering.
+    ///
+    /// Computes in **O(|curr_index - new_index| + 1)** time (average).
+    pub fn reorder_value_index(&mut self, curr_index: usize, new_index: usize) -> Option<()> {
+        self.map.reorder_entry_index(curr_index, new_index)
+    }
+
     /// Sort the set’s values by their default ordering.
     ///
     /// See `sort_by` for details.
@@ -1153,6 +1189,17 @@ mod tests {
     use super::*;
     use util::enumerate;
 
+    /// Convert an IndexSet to a vector of value entries, in the set's iteration
+    /// order. Values are cloned so that the vector can own them without
+    /// modifying the IndexSet.
+    fn set_to_vec<T, S>(set: &IndexSet<T, S>) -> Vec<T>
+    where
+        T: Hash + Eq + Clone,
+        S: BuildHasher,
+    {
+        set.iter().map(|value_ref| value_ref.clone()).collect()
+    }
+
     #[test]
     fn it_works() {
         let mut set = IndexSet::new();
@@ -1369,6 +1416,90 @@ mod tests {
         }
     }
 
+    #[test]
+    fn reorder_value() {
+        let insert = [0, 4, 2, 12, 8, 7, 11];
+        let mut set = IndexSet::new();
+
+        for &elt in &insert {
+            set.insert(elt);
+        }
+
+        assert_eq!(insert.to_vec(), set_to_vec(&set));
+
+        // Try moving values to different positions.
+        set.reorder_value(&12, 0).unwrap();
+        set.reorder_value(&12, 4).unwrap();
+        set.reorder_value(&4, 6).unwrap();
+        set.reorder_value(&4, 4).unwrap();
+        set.reorder_value(&0, 5).unwrap();
+        let expected_vec = vec![2, 8, 12, 4, 7, 0, 11];
+        assert_eq!(expected_vec, set_to_vec(&set));
+
+        // Try moving values to where they already are, and check that this
+        // doesn't change anything.
+        set.reorder_value(&12, 2).unwrap();
+        set.reorder_value(&0, 5).unwrap();
+        set.reorder_value(&11, 6).unwrap();
+        set.reorder_value(&7, 4).unwrap();
+        assert_eq!(expected_vec, set_to_vec(&set));
+    }
+
+    #[test]
+    fn reorder_value_index() {
+        let insert = [0, 4, 2, 12, 8, 7, 11];
+        let mut set = IndexSet::new();
+
+        for &elt in &insert {
+            set.insert(elt);
+        }
+
+        assert_eq!(insert.to_vec(), set_to_vec(&set));
+
+        // Try moving values to different positions.
+        set.reorder_value_index(3, 0).unwrap();
+        set.reorder_value_index(0, 4).unwrap();
+        set.reorder_value_index(1, 6).unwrap();
+        set.reorder_value_index(6, 4).unwrap();
+        set.reorder_value_index(0, 5).unwrap();
+        let expected_vec = vec![2, 8, 12, 4, 7, 0, 11];
+        assert_eq!(expected_vec, set_to_vec(&set));
+
+        // Try moving values to where they already are, and check that this
+        // doesn't change anything.
+        set.reorder_value_index(2, 2).unwrap();
+        set.reorder_value_index(5, 5).unwrap();
+        set.reorder_value_index(6, 6).unwrap();
+        set.reorder_value_index(4, 4).unwrap();
+        assert_eq!(expected_vec, set_to_vec(&set));
+    }
+
+    #[test]
+    fn reorder_value_failure() {
+        let insert = [0, 4, 2, 12, 8, 7, 11];
+        let mut set = IndexSet::new();
+
+        for &elt in &insert {
+            set.insert(elt);
+        }
+
+        // Try calling reorder_value with nonexistent values, using both valid
+        // and invalid indices.
+        assert_eq!(set.reorder_value(&1, 0), None);
+        assert_eq!(set.reorder_value(&5, 7), None);
+        assert_eq!(set.reorder_value(&3, 10), None);
+
+        // Try calling reorder_value with valid values but invalid indices.
+        assert_eq!(set.reorder_value(&2, 7), None);
+        assert_eq!(set.reorder_value(&12, 10), None);
+
+        // Try calling reorder_value_index with invalid indices.
+        assert_eq!(set.reorder_value_index(0, 15), None);
+        assert_eq!(set.reorder_value_index(1, 7), None);
+        assert_eq!(set.reorder_value_index(7, 1), None);
+        assert_eq!(set.reorder_value_index(8, 9), None);
+    }
+
     #[test]
     fn partial_eq_and_eq() {
         let mut set_a = IndexSet::new();

[bluss]

It makes sense to me!

• Option is good for found/not found. Documentation needs to be in tune with the choice of Option or Result. if it's called Success/Failure in doc, then it must use Result. Maybe it should be reformulated.
• Indexing errors (out of bounds) normally panic, we don't use Result on those errors in this crate
• The name "reorder" doesn't convey "pick up one from where it was and put in a new location" to me. Maybe the methods could have names that use "move" or "put"?

The hashbrown PR will have priority for now, I fear this PR needs to be rebased again after that is merged.

[allen-marshall]

Thanks for the feedback. I am currently struggling with mental health issues that might make it difficult for me to work on this for a while; otherwise I would make the suggested changes myself. If someone else wants to fix the issues so the feature (or something similar) can potentially be merged, feel free.

[EndilWayfare]

I have a very similar use case. I would much rather contribute to this pull request than roll my own datastructure from scratch. If the changes were made and branch merged, how soon might this make it into a public release?

(I'd prefer not to use a git source in my Cargo.toml, but I could live with it since my project will be in pre-pre-alpha for a while)

[cuviper]

Once it's merged, we can publish a release at any time.

[cuviper]

We now have move_index from #176, which serves the same purpose as reorder_entry_index. I suppose we could still add something that does a key lookup first, but that's just a combination of get_index_of and move_index.