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.

Sign up for GitHub

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

Jump to bottom

ACP: Move std::collections::hash_map::{RandomState, DefaultHasher} to std::hash #267

Closed
clarfonthey opened this issue Sep 9, 2023 · 1 comment
Closed

ACP: Move std::collections::hash_map::{RandomState, DefaultHasher} to std::hash #267

clarfonthey opened this issue Sep 9, 2023 · 1 comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@clarfonthey
Copy link

clarfonthey commented Sep 9, 2023 •
edited
Loading

Proposal

Problem statement

As part of the effort to move HashMap and HashSet (in some form) to liballoc, the actual libstd-specific implementation needs to be separated out from the version that can be added to liballoc.

In general, it's very weird that these are exported in std::collections::hash_map anyway, and to a user who wishes to use them outside these collections, they're not exactly easy to find. Additionally, these are only exported in std::collections::hash_map and not in std::collections::hash_set.

Ideally, these would be front and center in the std::hash module to indicate that they're the recommended way to perform hashing when the std crate is available, i.e. in all cases except those where #[no_std] is enabled.

Motivating examples or use cases

Mostly covered by above. The main use case is simply importing std::hash::RandomState instead of std::collections::hash_map::RandomState, in addition to RandomState and DefaultHasher being more discoverable in std::hash.

It's also worth pointing out that the only other Hasher implementation is SipHasher, which is deprecated. This would offer a genuine "default" choice alongside that one in the same module, rather than being located in a different one.

Solution sketch

std::collections::hash_map::{RandomState, DefaultHasher} should be re-exported in the std::hash module, making this module officially different from the one offered in libcore. In some future version, after a suitable amount of time, the original location can be deprecated.

Eventually, there could potentially be more methods for DefaultHasher that let the user seed the hasher with their own source of randomness in libcore, bridging the gap between the core and std implementations. This would require guaranteeing that the hasher itself doesn't depend on libstd, only RandomState does, although I don't think that would be too controversial.

Alternatives

The only real alternative is not doing this.

Links and related work

"Move HashMap to liballoc" tracking issue: rust-lang/rust#27242

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@clarfonthey clarfonthey added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Sep 9, 2023
@clarfonthey clarfonthey mentioned this issue Sep 9, 2023
Merged
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Oct 4, 2023
@Amanieu
Copy link
Member

Amanieu commented Oct 4, 2023

We discussed this in the libs-api meeting yesterday and are happy to accept this.

@Amanieu Amanieu closed this as completed Oct 4, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 10, 2023
@bors
Auto merge of rust-lang#115694 - clarfonthey:std-hash-private, r=dtolnay
b5545d6 
Add `std::hash::{DefaultHasher, RandomState}` exports (needs FCP)

This implements rust-lang/libs-team#267 to move the libstd hasher types to `std::hash` where they belong, instead of `std::collections::hash_map`.

<details><summary>The below no longer applies, but is kept for clarity.</summary>
This is a small refactor for rust-lang#27242, which moves the definitions of `RandomState` and `DefaultHasher` into `std::hash`, but in a way that won't be noticed in the public API.

I've opened rust-lang/libs-team#267 as a formal ACP to move these directly into the root of `std::hash`, but for now, they're at least separated out from the collections code in a way that will make moving that around easier.

I decided to simply copy the rustdoc for `std::hash` from `core::hash` since I think it would be ideal for the two to diverge longer-term, especially if the ACP is accepted. However, I would be willing to factor them out into a common markdown document if that's preferred.
</details>
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 11, 2023
@bors
Auto merge of rust-lang#115694 - clarfonthey:std-hash-private, r=dtolnay
2c1b65e 
Add `std::hash::{DefaultHasher, RandomState}` exports (needs FCP)

This implements rust-lang/libs-team#267 to move the libstd hasher types to `std::hash` where they belong, instead of `std::collections::hash_map`.

<details><summary>The below no longer applies, but is kept for clarity.</summary>
This is a small refactor for rust-lang#27242, which moves the definitions of `RandomState` and `DefaultHasher` into `std::hash`, but in a way that won't be noticed in the public API.

I've opened rust-lang/libs-team#267 as a formal ACP to move these directly into the root of `std::hash`, but for now, they're at least separated out from the collections code in a way that will make moving that around easier.

I decided to simply copy the rustdoc for `std::hash` from `core::hash` since I think it would be ideal for the two to diverge longer-term, especially if the ACP is accepted. However, I would be willing to factor them out into a common markdown document if that's preferred.
</details>
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Nov 15, 2023
@bors
Auto merge of #115694 - clarfonthey:std-hash-private, r=dtolnay
a82fa5a 
Add `std::hash::{DefaultHasher, RandomState}` exports (needs FCP)

This implements rust-lang/libs-team#267 to move the libstd hasher types to `std::hash` where they belong, instead of `std::collections::hash_map`.

<details><summary>The below no longer applies, but is kept for clarity.</summary>
This is a small refactor for #27242, which moves the definitions of `RandomState` and `DefaultHasher` into `std::hash`, but in a way that won't be noticed in the public API.

I've opened rust-lang/libs-team#267 as a formal ACP to move these directly into the root of `std::hash`, but for now, they're at least separated out from the collections code in a way that will make moving that around easier.

I decided to simply copy the rustdoc for `std::hash` from `core::hash` since I think it would be ideal for the two to diverge longer-term, especially if the ACP is accepted. However, I would be willing to factor them out into a common markdown document if that's preferred.
</details>
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
@bors
Auto merge of #115694 - clarfonthey:std-hash-private, r=dtolnay
9f3143b 
Add `std::hash::{DefaultHasher, RandomState}` exports (needs FCP)

This implements rust-lang/libs-team#267 to move the libstd hasher types to `std::hash` where they belong, instead of `std::collections::hash_map`.

<details><summary>The below no longer applies, but is kept for clarity.</summary>
This is a small refactor for #27242, which moves the definitions of `RandomState` and `DefaultHasher` into `std::hash`, but in a way that won't be noticed in the public API.

I've opened rust-lang/libs-team#267 as a formal ACP to move these directly into the root of `std::hash`, but for now, they're at least separated out from the collections code in a way that will make moving that around easier.

I decided to simply copy the rustdoc for `std::hash` from `core::hash` since I think it would be ideal for the two to diverge longer-term, especially if the ACP is accepted. However, I would be willing to factor them out into a common markdown document if that's preferred.
</details>
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
@bors
Auto merge of #115694 - clarfonthey:std-hash-private, r=dtolnay
b112ce2 
Add `std::hash::{DefaultHasher, RandomState}` exports (needs FCP)

This implements rust-lang/libs-team#267 to move the libstd hasher types to `std::hash` where they belong, instead of `std::collections::hash_map`.

<details><summary>The below no longer applies, but is kept for clarity.</summary>
This is a small refactor for #27242, which moves the definitions of `RandomState` and `DefaultHasher` into `std::hash`, but in a way that won't be noticed in the public API.

I've opened rust-lang/libs-team#267 as a formal ACP to move these directly into the root of `std::hash`, but for now, they're at least separated out from the collections code in a way that will make moving that around easier.

I decided to simply copy the rustdoc for `std::hash` from `core::hash` since I think it would be ideal for the two to diverge longer-term, especially if the ACP is accepted. However, I would be willing to factor them out into a common markdown document if that's preferred.
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Amanieu @clarfonthey