Derived Hash for repr(u8) enum submits 8 bytes to Hasher instead of 1

[kryptan]

Automatically derived Hash for an enum marked as repr(u8) will hash 8 bytes when it is only necessary to hash just one. When hashing large value containing many such enums this will lead to 8x slower hashing than necessary.

Example:

use std::hash::{Hash, Hasher};

struct H;

impl Hasher for H {
    fn finish(&self) -> u64 {
        unreachable!()
    }

    fn write(&mut self, bytes: &[u8]) {
        println!("{:?}", bytes);
    }
}

#[repr(u8)]
#[derive(Hash)]
enum E {
    A,
    B,
}

fn main() { 
    E::A.hash(&mut H);
}

prints [0, 0, 0, 0, 0, 0, 0, 0] while I expected it to print [0] just like 0u8.hash(&mut H).

This behavior is the same on recent stable, beta and nightly (1.14, 1.15, 1.16).

---

EDIT: Added second variant to the enum. Previous example with single variant doesn't print anything anymore because of #42709.

[est31]

Can it be changed at this stage? Wouldn't a change break stability promises of the language?

[brson]

I wonder if the performance impact can be quantified in real workloads, or if this mistake shows up elsewhere. I'd guess this is because our enum discriminant intrinsic returns only u64 but not sure.

The impact seems low so we might be able to get away with changing it.

[est31]

The impact seems low so we might be able to get away with changing it.

For most non trivial hashing algorithms, it will most certainly change generated hashes if such an enum was involved.

There can be situations where you want your hashes to be stable, or when you design some on-disk or over-network format and want to keep the output constant because you need to be compatible with earlier compiles of your program.

Don't know of code which does this but it can't really be cratered...

[durka]

That intrinsic may need to change soon to return u128 so we'll have to keep this in mind as a caveat.

[nagisa]

Its plausible to change this. We specifically said in documentation of discriminant function, that the discriminant is only guaranteed to not change if it is observed in code compiled by the same compiler version.

I’m not sure how would we go around implementing the change which picks relevant feed method, but at the very least we do have type of enum “remembered” in Discriminant and could use that to deduce the right thing to use.

[durka]

We said that in the documentation of `mem::discriminant`, but since the discriminant is used for hashing I guess the question is whether hash values are implied to be stable.

…

On Wed, Jan 18, 2017 at 5:28 PM, Simonas Kazlauskas < ***@***.***> wrote: Its plausible to change this. We specifically said in documentation of discriminant <https://doc.rust-lang.org/stable/std/mem/fn.discriminant.html> function, that the discriminant is only guaranteed to not change if it is observed in code compiled by the same compiler version. I’m not sure how would we go around implementing the change which picks relevant feed method, but at the very least we do have type of enum “remembered” in Discriminant and could use that to deduce the right thing to use. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#39137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3nz6LpGoKFUL8eXPbnhNar8LWxhIzks5rTpH9gaJpZM4LmMUG> .

[Mark-Simulacrum]

Reopening -- this was not fixed by #42709. See #42709 (comment).

[durka]

It sounds like the consensus here is it's okay to fix this?

[est31]

If so, we should explicitly document that the values generated by the Hash trait are not stable, and may change between compiler updates for same data.

[steveklabnik]

Visiting for triage. Strangely, this code no longer prints. I'm not sure at all why this is.

[kryptan]

Visiting for triage. Strangely, this code no longer prints. I'm not sure at all why this is.

That's because of #42709. I have updated the code (added second variant to the enum).

[saethlin]

I think this has been done in the meantime. The above code now prints [0].