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

No non-deprecated hasher in libcore #37071

Closed
SimonSapin opened this issue Oct 10, 2016 · 11 comments
Closed

No non-deprecated hasher in libcore #37071

SimonSapin opened this issue Oct 10, 2016 · 11 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Since #36815, a #![no_std] crate that uses core::hash::SipHasher gets a deprecation warning that says "use DefaultHasher instead". But DefaultHasher is in std::collections::hash_map and not available in libcore.

All the Sip code is still in libcore, but there is no public non-deprecated API to use it or any other hasher.

Should DefaultHasher be moved to core::hash? (std::collections::hash_map would have a pub use re-export.)

CC @alexcrichton, @sfackler

@sfackler
Copy link
Member

sfackler commented Oct 10, 2016

We can't move DefaultHasher since it requires access to the system RNG.

EDIT: Nope, I'm an idiot, that's RandomState. Seems reasonable to move.

@SimonSapin
Copy link
Contributor Author

I tried implementing this and got:

error[E0450]: cannot invoke tuple struct constructor with private fields
    --> ../src/libstd/collections/hash/map.rs:2081:9
     |
2081 |         DefaultHasher(SipHasher13::new_with_keys(self.k0, self.k1))
     |         ^^^^^^^^^^^^^ cannot construct with a private field

and I don’t see an easy way to fix this.

But really, the problem here is that we want apparently want to deprecate the new_with_keys functionality, but we still want the default hash map to use it.

I find this inconsistent. Hashing is the foundation of hash maps, and we provide both for users of std/core, except for that one bit of functionality. I’d like to propose un-deprecating new_with_keys.

@sfackler
Copy link
Member

We can work around that in a hacky way by having a perma-unstable, doc(hidden) constructor.

As far as public APIs go, we can add a constructor which takes a &[u8] and derives a key from it. We don't want to set the underlying key size in stone, but we can probably document its current size and guarantee that if you provide the right number of bytes, we'll do the fast thing. I'm looking into doing something similar to allow the RandomState seed to be set.

@alexcrichton
Copy link
Member

I would prefer to not have perma-unstable constructors where possible, including this. I also don't think std::hash::DefaultHasher is appropriate, because it's just the default for hash maps not std::hash which is far more general.

@SimonSapin
Copy link
Contributor Author

To sum up, I see two issues:

  • At least one non-deprecated hasher should be available in core. It doesn’t have to be named "default".
  • Having some way to parameterize a hash function (as new_with_keys does) is useful, as evidenced by RandomState and phf using it. std (and maybe core) should provide some non-deprecated API for doing this.

@burdges
Copy link

burdges commented Oct 25, 2016

I understand the desire to make the default hasher choice opaque, but obviously many applications depend upon access to a specific algorithm and new_with_keys. In particular, there is no way to serialize and repopulate a probabilistic data structure like a Bloom filter, Cuckoo table, etc., so they must be written to disk along with the hash keys used to define them, and you'd likely just mmap them anyways. Are crates just supposed to fork the SipHasher code or something?

@sfackler
Copy link
Member

It seems pretty reasonable to me to stick a SipHash implementation in a crate on crates.io.

@jimblandy
Copy link
Contributor

Agreeing with @SimonSapin and @burdges:

When writing white/gray-box tests it's very nice to be able to fix a particular hashing function and keys. Less important but still nice is being able to put examples in docs and have them actually behave as promised even as Rust evolves. Rust is usually very good about helping me keep executions deterministic when I need them to be so, but having to refer to an external crate just to get a hasher whose meaning stays fixed over time is not so helpful.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 26, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 15, 2017

I would lean toward using a no_std hasher from crates.io. I don't believe it is important for core to contain at least one non-deprecated implementation of Hasher.

@SimonSapin
Copy link
Contributor Author

Is there a #![no_std] hasher in the rust-lang or rust-lang-nursery org that the docs could recommend?

@SimonSapin
Copy link
Contributor Author

https://crates.io/crates/siphasher is not in rust-lang{,-nursery} but has the same code that is/was in libcore, and is no_std. I still believe it would be good for libcore to export one hasher, but that can be an RFC for another time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. 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

9 participants