Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve get_index_mut documentation wrt mutable keys #174

Closed
TethysSvensson opened this issue Feb 21, 2021 · 5 comments
Closed

Improve get_index_mut documentation wrt mutable keys #174

TethysSvensson opened this issue Feb 21, 2021 · 5 comments

Comments

@TethysSvensson
Copy link

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.

@bluss
Copy link
Member

bluss commented Feb 21, 2021

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.

@TethysSvensson
Copy link
Author

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?

@bluss
Copy link
Member

bluss commented Feb 21, 2021

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.

@TethysSvensson TethysSvensson changed the title get_index_mut allows changing the key Improve get_index_mut documentation wrt mutable keys Feb 21, 2021
@cuviper
Copy link
Member

cuviper commented Feb 23, 2021

This does not seem like it is the intended behavior, since e.g. first_mut only 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..

@cuviper
Copy link
Member

cuviper commented Jun 23, 2023

This was fixed in #219 to return (&K, &mut V), and MutableKeys::get_index_mut2 for the "dangerous" behavior. I'll be releasing 2.0 soon!

@cuviper cuviper closed this as completed Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants