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

Implement IndexMut for HashMap #527

Closed
wants to merge 1 commit into from

Conversation

ToMe25
Copy link
Contributor

@ToMe25 ToMe25 commented May 30, 2024

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
Copy link
Contributor Author

ToMe25 commented May 30, 2024

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
Copy link
Member

Amanieu commented May 30, 2024

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
Copy link
Contributor Author

ToMe25 commented May 30, 2024

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
Copy link
Member

Amanieu commented May 30, 2024

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.

@Amanieu Amanieu closed this May 30, 2024
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

Successfully merging this pull request may close these issues.

2 participants