hashing of general objects was far too simple (fixes #20744) #20767
Conversation
StefanKarpinski
force-pushed
the
sk/hashfix
branch
from
df00880 to
9fe729b
Compare
If we'd used `3h - object_id(x)` previously, the original issue would never have come up since `hash(a, hash(b, hash(c, h)))` wouldn't have produced `3A - 3B + 3C - h` but would instead have given the value `27h -9A - 3B - C`, which is asymmetrical in `a` and `c`. Passing the result to `hash_uint` fixes that issue since that function is non-linear, but defense in depth, right? We may as well do the same operations in such a way that the passed-through value gets the most operations done on it.
|
dunno what our policy on hash-value stability is, but we should probably merge this before #20762 just in case |
|
The segfault on osx that appeared here might be fixed by #20777. |
|
would it have anything to do with changing hash values, or is it preexisting and just coincidence? |
|
No, it's unrelated to hash values. |
We are allowed to change hash values any time we want to – code should not rely on them. In fact, I've considered whether we should randomize them from run to run. |
| @@ -55,7 +55,6 @@ end | |||
|
|
|||
| ## symbol & expression hashing ## | |||
|
|
|||
| hash(x::Symbol, h::UInt) = 3*object_id(x) - h | |||
There was a problem hiding this comment.
@JeffBezanson: is it possible this line was added for performance since the hash(x::ANY, h::UInt) method doesn't specialize because of the ANY hint?
There was a problem hiding this comment.
I imagine it was either a #265-related bug (hash was pretty likely to run into bugs of that sort because of the fallback definition), or an oversight. object_id is also not specialized (and should just end up always inlined too)
There was a problem hiding this comment.
So, in your professional opinion, this should be safe to delete?
There was a problem hiding this comment.
Yes. Looks like Tony already did though :)
is that documented at all? I don't think this is necessarily being followed. |
|
It is not. But randomizing hash values should make it fairly clear 😈 |
No description provided.