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

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

Open
fowles opened this issue Oct 20, 2024 · 3 comments
Open

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

fowles opened this issue Oct 20, 2024 · 3 comments
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@fowles
Copy link

fowles commented Oct 20, 2024

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.

@fowles fowles added the C-bug Category: This is a bug. label Oct 20, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 20, 2024
@jieyouxu jieyouxu added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 20, 2024
@lolbinarycat lolbinarycat added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 20, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 21, 2024
@daboross
Copy link
Contributor

daboross commented Oct 22, 2024

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

daboross commented Oct 22, 2024

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

fowles commented Oct 22, 2024

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 26, 2024
…ck, r=Mark-Simulacrum

Document `PartialEq` impl for `OnceLock`

Adds documentation to `std::sync::OnceLock`'s `PartialEq` implementation: specifies publicly that `OnceLock`s are compared based on their contents, and nothing else.

Created in response to, but not directly related to, rust-lang#131959.

## ne

This doesn't create and document `PartialEq::ne`. There's precedent for this in [`RefCell`](https://doc.rust-lang.org/std/cell/struct.RefCell.html#impl-PartialEq-for-RefCell%3CT%3E).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 26, 2024
Rollup merge of rust-lang#132019 - daboross:document-partialeq-oncelock, r=Mark-Simulacrum

Document `PartialEq` impl for `OnceLock`

Adds documentation to `std::sync::OnceLock`'s `PartialEq` implementation: specifies publicly that `OnceLock`s are compared based on their contents, and nothing else.

Created in response to, but not directly related to, rust-lang#131959.

## ne

This doesn't create and document `PartialEq::ne`. There's precedent for this in [`RefCell`](https://doc.rust-lang.org/std/cell/struct.RefCell.html#impl-PartialEq-for-RefCell%3CT%3E).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants