Remove HashSet::get_or_insert_with

[Amanieu]

This method is unsound because it allows inserting a key at the "wrong" position in a HashSet, which could result in it not appearing it future lookups or being inserted multiple times in the set.

Instead, HashSet::get_or_insert and HashSet::get_or_insert_owned should be preferred.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Mark-Simulacrum]

r=me, but I don't think we should say this is unsound? The method is entirely safe code so that implies other problems. It might be a bad idea, but that's a different kind of problem than a soundness issue.

[Amanieu]

The view of the libs-api team is that unsafe code should be able to rely on an incoming HashMap<T> (and other collections) to work correctly (i.e. not have items that appear in iteration but not in lookups) when T is not a user-controlled type.

[cuviper]

I agree that we want that reliability, but the term "unsound" is the other way around:
https://doc.rust-lang.org/reference/behavior-considered-undefined.html

if unsafe code can be misused by safe code to exhibit undefined behavior, it is unsound.

[fogti]

wouldn't it make more sense to panic! if the key doesn't match?

[cuviper]

Sure, adding a check on the new value might be better than throwing this API out altogether...
rust-lang/hashbrown#518

[RustyYato]

@Amanieu

The view of the libs-api team is that unsafe code should be able to rely on an incoming HashMap (and other collections) to work correctly (i.e. not have items that appear in iteration but not in lookups) when T is not a user-controlled type.

Then get_or_insert_owned is in the same boat, since it's trivial to write a malicious implementation.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=1ec481da7b5fce04dc5dd3e0506409fa

So either both get_or_insert_with and get_or_insert_owned are removed or they are both kept. Since it is possible to implement one in terms of the other (I can show that too if you need). (the hashbrown PR does fix thismitigates the damage and make this harder to exploit, but it should also check get_or_insert_owned for the reasons I showed above)

[cuviper]

the hashbrown PR does fix this

Does it? If we're considering malicious impls, then your PartialEq could also abuse static/thread-local state to return true right after the ..._with closure or its to_owned.

[Amanieu]

Closing since get_or_insert_owned has the same problem, this doesn't actually help.