rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.48k stars 12.73k forks source link

`std::sync::OnceLock` is `Eq` but not `Hash` #131959

Open fowles opened 3 weeks ago

fowles commented 3 weeks ago

The standard library type std::sync::OnceLock is Eq but not Hash. From a theoretical and practical perspective, it should probably be both or neither.

The argument for neither is that the result of Eq can change because of a different thread as soon as it is returned. Similarly, the value of Hash can change between comparison of the Hash and a follow up comparison with Eq. Both of these are expected short-comings of types that can be changed by other threads.

The argument for both is that Hash is fundamentally a property related to equality and not rules out usages in containers where there are some external invariants that guarantee the desired behavior.

daboross commented 3 weeks ago

The PartialEq impl for OnceLock just compares the internal value without considering other fields, which I believe should resolve the concern about Eq.

It seems reasonable to implement Hash. I'd like to write a PR for that, and I presume we can do an FCP there if necessary.

Probably worth documenting that equality behavior as well, I imagine!

@rustbot claim

daboross commented 3 weeks ago

Right! Sorry, I realized I misinterpreted your comment about Eq. Of course it changes with different threads setting the values, that's the whole thing with the type.

Looking at implementing this, though, I've realized there's a precedent for this: Cell and RefCell both also implement PartialEq and Eq without Hash.

Might be reasonable to implement Hash for all three, but it gives me second thoughts about making a PR right now for just OnceLock.

@rustbot release-assignment

At risk of favoring the status quo, do you have a use case for needing Hash here, or is it mostly for API completeness?

fowles commented 3 weeks ago

Mostly API completeness. I had a design that involved putting these things in a hashmap for later removal and processing, but ended up shifting away from it anyway.