search

rust-lang / hashbrown

Rust port of Google's SwissTable hash map
https://rust-lang.github.io/hashbrown
Apache License 2.0
2.38k stars 275 forks source link

Implement IndexMut for HashMap #527

Closed ToMe25 closed 3 months ago

ToMe25 commented 3 months ago

Since HashMap already has get_mut and implements Index i believe there is no good reason not to implement IndexMut.

If there is some reason I missed for why this isn't a good idea, please let me know :)

My initial intention was to implement this for the std HashMap directly, but then I saw that past PR's with changes to that got told to implement the changes in hashbrown as well, so I made this PR first.

ToMe25 commented 3 months ago

I'm pretty sure the clippy CI failure wasn't caused by my change.

It seems trivial to fix though, should I make a PR for that?

Amanieu commented 3 months ago

I believe the reason this hasn't been implemented before is because of the surprising behavior for missing keys. In other languages such as Python and C++ you can insert a new entry by using map[key] = value. Making this cause a panic in Rust could be surprising to many users coming from other languages.

Perhaps in the future we can make this work by supporting an assignment version of the Index trait.

ToMe25 commented 3 months ago

I rebased this to allow the CI to run.

I believe the reason this hasn't been implemented before is because of the surprising behavior for missing keys. In other languages such as Python and C++ you can insert a new entry by using map[key] = value. Making this cause a panic in Rust could be surprising to many users coming from other languages.

I personally don't think having a read-only map[key] is less confusing than a mutable but not replaceable map[key]. In fact Vec and VecDeque already implement IndexMut, though LinkedList doesn't for some reason.

It seems like LinkedList doesn't implement IndexMut because it doesn't implement Index.

Well, if IndexMut not being implemented is a conscious decision, feel free to close this PR.

Amanieu commented 3 months ago

I would say that this is a conscious decision: we want to leave the door open for supporting a future IndexAssign functionality. If we were to implement IndexMut today, this would no longer be possible since adding IndexAssign later would be a breaking change.