Skip to content
New issue

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

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

Already on GitHub? # to your account

Violating the set invariant with HashSet::get_or_insert_with #399

Open
JustForFun88 opened this issue Feb 15, 2023 · 2 comments · May be fixed by #400
Open

Violating the set invariant with HashSet::get_or_insert_with #399

JustForFun88 opened this issue Feb 15, 2023 · 2 comments · May be fixed by #400

Comments

@JustForFun88
Copy link
Contributor

JustForFun88 commented Feb 15, 2023

The implementation of HashSet::get_or_insert_with is not good. This method make it quite easy to break the invariant of set that all elements are repeated in set only once. Let's say this test passes:

#[test]
fn some_invalid_insert() {
    let mut set = HashSet::new();
    set.insert(1);
    set.get_or_insert_with(&2, |_| 1);
    set.get_or_insert_with(&3, |_| 1);
    assert_eq!(vec![&1, &1, &1], set.iter().collect::<Vec<_>>());
}

May be we need document it?

@JustForFun88 JustForFun88 changed the title Violating the set invariant with HashSet::get or_insert_with Violating the set invariant with HashSet::get_or_insert_with Feb 15, 2023
@JustForFun88 JustForFun88 linked a pull request Feb 16, 2023 that will close this issue
@Amanieu
Copy link
Member

Amanieu commented Feb 22, 2023

There is a much longer discussion about this in rust-lang/rust#60896, which is about the equivalent (unstable) method in the standard library. I would prefer to keep the API as it is for now until a decision is made on that issue. One proposed alternative is to drop this function and instead only provide get_or_insert_owned.

@JustForFun88
Copy link
Contributor Author

There is a much longer discussion about this in rust-lang/rust#60896, which is about the equivalent (unstable) method in the standard library. I would prefer to keep the API as it is for now until a decision is made on that issue. One proposed alternative is to drop this function and instead only provide get_or_insert_owned.

Didn't know about the discussion. In any case, I'll leave #400 open for now, pending a final decision.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants