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

Warn for overwriting HashMap entry #11978

Open
SeseMueller opened this issue Dec 18, 2023 · 1 comment · May be fixed by #12575
Open

Warn for overwriting HashMap entry #11978

SeseMueller opened this issue Dec 18, 2023 · 1 comment · May be fixed by #12575
Labels
A-lint Area: New lints

Comments

@SeseMueller
Copy link

What it does

Inserting on a HashMap on a key that is already contained overwrites the value that was at that location.
If the return value of insert (Option<T>; what was previously there) is not used, that value is lost.
While it is practically impossible to warn for all cases where this might occur, a few might be caught.
I suggest checking for an insert (or get_mut().unwrap()) followed by another insert which throws away the return value, while there weren't any reads on the HashMap in between.

Advantage

  • Catch the loss of a value due to overwriting it
  • Also useful for Hashmaps that map to complex types that may be mutated in-place

Drawbacks

In some situations overwriting the old value is exactly the plan and this warn might have false positives in these kinds of situations.
This also opens the discussion whether HashMap::insert's result should be must use, where the ease of use stands against potential errors like the loss of data as explained above.

Example

    let mut test = HashMap::new();
    test.insert(5, vec![1,2,3]); // Or get_mut if the values is already contained: test.get_mut(5).unwrap().push(4)
    test.insert(5, vec![]); // Return value not used
@SeseMueller SeseMueller added the A-lint Area: New lints label Dec 18, 2023
@Alexendoo
Copy link
Member

HashMap::from([(1, 2), (1, 3)]) would be a good pattern to catch also

@SeseMueller SeseMueller linked a pull request Mar 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants