Hashcode in Decimal violates the hash contract

[lukstei]

The hash implementation in https://github.com/neogenie/fastnum/blob/master/src/decimal/dec/impls/hash.rs violates the Hash contract, because two decimals which are equal can have different hashes, this will lead to issues when using Decimal in HashMapkeys.

This happens can happen when one of the decimals has a different control block value than the other.

Test which replicates the issue:

#[cfg(test)]
mod tests {
    use fastnum::dec128;
    use fastnum::decimal::{Context, RoundingMode, Signals};
    use std::hash::{BuildHasher, BuildHasherDefault, DefaultHasher, Hash, Hasher, RandomState};

    #[test]
    fn test_hash() {
        fn hash<T: Hash>(x: T) -> u64 {
            let mut hasher = DefaultHasher::new();
            x.hash(&mut hasher);
            hasher.finish()
        }

        let price1 = dec128!(1);
        let price2 = dec128!(1.1).floor();
        assert!(!price2.is_op_ok());

        assert_eq!(price1, price2);
        assert_eq!(hash(price1), hash(price1));
        assert_eq!(hash(price1), hash(price2)); // will fail
    }
}

[neogenie]

@lukstei

Thank you for your feedback and participation in the project!

This is actually a great question, and unfortunately, I don't have a ready answer for it. The thing is, in the current implementation, signal flags are not used for comparison — and this is convenient, but strictly speaking, they are different numbers.

How to proceed: should we fix the hashing implementation, making it less strict, which would eliminate the ability to calculate strict checksums. Or should we fix the comparison operation, making it stricter, but this would create certain inconveniences elsewhere?

Let's discuss this issue.

[lukstei]

@neogenie thanks for the answer. In the above example, in my opinion these two numbers should be considered equal, because they represent the same number regardless if one was rounded or not.
So I'd just remove the control block from the hash and leave everything else as is - do you see any downsides to this approach?

[neogenie]

@lukstei
One needs to think about it, but at first glance, such behavior can be considered acceptable. The only thing is that the entire control block should not be removed, as it contains some useful information, such as the sign and the exponent 🤭

[lukstei]

Okay, I'd just remove it from the hash code generation, but keep in general, that should work. Let me prepare a PR.

[neogenie]

@lukstei
As I wrote before, the control block cannot be completely excluded from hashing because it contains quite important information, which is taken into account, among other things, for the comparison operation.
I sketched out a draft. It will most likely be included in the next release, but we still need to think about corner cases, especially with rounding.

[lukstei]

I see. Thanks for taking a look. I had the impression that, based on the tests I saw, same-digit decimals were considered equal, regardless of control block. However, that doesn't seem to be the case.