[Accepted] SE-0206: Hashable Enhancements

[lorentey (Karoy Lorentey)]

These are really interesting points; thank you for raising them.

@GarthSnyder

I find the bits labels in the various type-specific forms of combine(bits:) somewhat strange. Yes, I get that the hasher doesn’t care that what you’re passing in is an Int; it just wants its 32 bits or 64 bits of data. By why is this important to represent in the API? Why are these not just type-specific overloads for combine()?

There is a small semantic issue here -- it isn't clear if we can allow hasher.combine(42 as Int64) and hasher.combine(42 as Int8) to produce different hashes.

They are different types, so I'd argue we can and should be free to do that. However, the standard library defines heterogeneous comparison operators between all fixed-width integer types. This means that (42 as Int8) == (42 as Int64), and having the two values produce different hashes may be confusing. (AnyHashable and its interaction with NSNumber produces additional complications.)

The bits: label for the primitive combine functions makes it clear that the bit width of integer values is significant.

@GarthSnyder

In the current design, if you call combine() on an Int (as the GridPoint example seems to imply that you should) the hasher presumably calls Int.hash(into:), which presumably then turns arounds and re-calls Hasher.combine(bits: Int). Why not just avoid this little dance and accept combine(Int) directly?

The implementation of hash(into:) for integer types is currently quite a bit more complicated than that. I hope to make hasher.combine(foo) mean the same thing as hasher.combine(bits: foo) for all integer types, but it will take more discussion and some extra work.

(For Int, the current implementation does indeed end up calling combine(bits: Int), but this is an accidental consequence of Hasher not implementing the full API proposed here, and we will likely need to change it once we have combine(bits: Int8) working.)

@GarthSnyder

@thomasvl raised the issue of how to hash things order-independently. The official solution of hashing all the parts separately with their own hashers and then XORing the results seems functional and not hard to implement given that this is an uncommon requirement. However, the proposal itself talks at some length about how instantiating a bunch of separate hashers may be potentially nonperformant.

We shouldn't be doing unnecessary work. However, as far as I can see, commutative hashing requires calculating a separate hash value for each element, and spawning a new hasher is simply the correct way to do that.

Hasher.finalize() is not in itself slow; calling it only harms performance when the call can be eliminated.

@GarthSnyder

This case on its own seems like a good argument for making Hasher a protocol.

Can you provide an example how a Hasher protocol would make things faster here? (I'd hesitate to have Set and Dictionary produce weaker hash values.)

@GarthSnyder

It’s not obvious from the proposal that Hasher is, in fact, guaranteed to be order-sensitive, though that’s probably what most people would guess. This should really be an explicit part of the API contract that is mentioned in the header comments.

This is a good point! We changed the name from append to combine late in the drafting process; I think combine is clearly better, but it does not by itself imply strict ordering. We can (and should) make up for this in the documentation. (append is uncomfortably close to Array.append. While Hasher.combine and Array.append are similar operations in an abstract sense, they actually mean completely different things.)

@GarthSnyder

I agree with @hlovatt that the ability to cache hash values is important.

I agree too, and your implementation is the correct way to do it. (I.e., spawning/finalizing a separate hasher to generate the cached hash, then feeding the hash value in hash(into:).)

Note that caching the hash involves mutating operations, so for value types, you cannot do it directly in hash(into:). (This is by design, and the same applied for hashValue.) This is not a real limitation, as you can simply force-generate the cached hash sometime before you start using the value in hashing contexts.

@GarthSnyder

This is getting exceedingly picayune, but it bothers me that the GridPoint example in the header comments shows how to add hashing to a struct that in fact needs no code from you at all; the compiler would handle it just fine on its own

Hm; maybe we should simply turn the example into a demonstration of how the compiler implements hash(into:). It would serve the same purpose,, but it would avoid getting bogged down in questions like why wouldn't you want to hash the color of a GridPoint.