Improve get_index_mut documentation wrt mutable keys
Closed this issue · 5 comments
The function get_index_mut gives a mutable reference to the of an entry, but does not update the corresponding hash. This means you can write buggy code like this:
use indexmap::IndexMap;
fn main() {
let mut map = IndexMap::new();
map.insert("foo", 0);
*map.get_index_mut(0).unwrap().0 = "bar";
assert!(!map.contains_key("foo"));
assert!(!map.contains_key("bar"));
assert!(map.keys().collect::<Vec<_>>() == [&"bar"]);
}This does not seem like it is the intended behavior, since e.g. first_mut only gives a non-mutable reference to the key.
If this is the intended behavior, then there should probably be a warning in the documentation that you should not changing the key in ways which alter the Hash or Eq behavior.
There should be a warning in docs. It's intended the mutable keys functionality is mostly hidden away in the MutableKeys trait.
Users can cause similar bugs with internal mutability in any regular hash map, or other ways of implementing buggy equality/hash functionality.
But the get_index_mut function is not hidden behind the MutableKeys trait, but is directly on the IndexMap.
Perhaps there should also be a get_index_mut2 in the MutableKeys trait, and the get_index_mut could then be made to return a non-mutable key?
That sounds good, but on its own it is no reason to release a 2.0 version, it can be on the list in #135 (Edit - oh, it's already on the list) i.e. not likely to be addressed anytime soon.
Let's make this issue about just improving docs.
This does not seem like it is the intended behavior, since e.g.
first_mutonly gives a non-mutable reference to the key.
Yeah, get_index_mut came long before, but we did notice that inconsistency when first_mut/last_mut were added in #160, and we decided to make the new methods match our better intention..