hashbrown icon indicating copy to clipboard operation
hashbrown copied to clipboard

Consider not dropping entries on panicky rehashes

Open Manishearth opened this issue 11 months ago • 2 comments

Context: I maintain elsa, a crate which contains append-only collections that can be appended to with only &self references. Recently @steffahn brought this bug to my attention, where elsa's assumption that "a hashmap that only experiences insertions (not replacements or removals) will never drop values". This is incorrect in the (relatively rare) case Hash panics on subsequent calls. (It's a very interesting edge case, nice catch @steffahn!)

I do have a way to fix this: we can ensure that Hash is only called by our code (by using a wrapper type with a cached hash; a common pattern). There are other potential fixes too.

However, I do think that the behavior that insert() may drop values not being replaced is surprising, and while HashMap makes no promises of its behavior when you write silly Hash and Eq implementations, I do think a different route for the garbage-in-garbage-out behavior is possible: when Hash panics during realloc, stuff the rest of the entries back into the hashmap in order regardless of hash. It will be totally unpredictable on .get() (but we knew that; that's what is expected when you write silly Hash and Eq implementations), but it will end up calling the destructors at the right time. An alternate route would be to just leak the values.

Basically, I don't think the current behavior of going out of our way to clean stuff up is super necessary, and it makes things harder for unsafe code (elsa is a crate that heavily relies on this, but I can imagine other unsafe code writers making similar assumptions in more specific cases).

Thoughts?

Manishearth avatar Jul 16 '23 14:07 Manishearth