niklasf / shakmaty

A Rust library for chess and chess variant rules and operations
https://docs.rs/shakmaty
GNU General Public License v3.0
210 stars 41 forks source link

Add Hash and PartialEq implementations to all variants #69

Closed UE2020 closed 1 year ago

UE2020 commented 1 year ago

I've added Hash & PartialEq implementations for every variant to close #68

niklasf commented 1 year ago

Thanks!

After pondering this a bit more, I decided to change the notion of equality for positions, slightly modifying your patch. I now documented that Setup uses structural equality (5d52fba62575f10190424d51e70d6abd9adfb5ff), but Position uses FIDE rules for repeated positions (da5377df91f444c7b60a38f1863bb4d1ef936179), which excludes the move counters. That's of course a semver breaking change, but ultimately probably more useful.

Omitting legal_ep_square() from Hash was an optimization, because it can be relatively expensive, but provides little entropy for the hash.

UE2020 commented 1 year ago

Omitting legal_ep_square() from Hash was an optimization, because it can be relatively expensive, but provides little entropy for the hash.

This might be unsound since PartialEq/Eq do include the ep square in the equality check. From the Hash trait docs:

When implementing both Hash and Eq, it is important that the following property holds: k1 == k2 -> hash(k1) == hash(k2) In other words, if two keys are equal, their hashes must also be equal. HashMap and HashSet both rely on this behavior.

niklasf commented 1 year ago

It's ok in this direction (due to "->"). It's the same direction that allows random hash collisions.

Conversely, adding fields to Hash but not checking them in Eq would violate the property.