inducer / pytato

Lazily evaluated arrays in Python
Other
8 stars 16 forks source link

Equality of objects doesn't imply equality of hashes for certain array types #496

Closed majosm closed 1 month ago

majosm commented 1 month ago

Array types that contain Mappings can, under some circumstances, compare equally but hash unequally. This is due to a combination of two factors:

  1. Equality comparison and hashing are implemented slightly differently for arrays. EqualityComparer compares the keys and values between the two mappings. Hashing uses whatever is generated by attrs.frozen (presumably calling hash() on the whole mapping object?).
  2. Different mapping types (e.g., immutables.Map and immutabledict) implement hashing differently, so two mappings with different types can have the same contents but not hash the same.

This can be avoided by always using the same mapping class for these attributes, but that raises the question of whether the type annotation should continue to be Mapping, or if it should be made more restrictive (and possibly asserted to be a certain type in __init__?). Another option might be to modify the hashing so that it hashes the keys/values instead of the whole mapping object. I'm not sure which way is preferable.

@inducer This is what was causing my CopyMapper duplication issues. The code in main had been updated to use immutabledict in #461, but the concatenation branches of pytato/arraycontext still had some immutables.Maps left over.

cc @matthiasdiener

inducer commented 1 month ago

Thanks for tracking this down!

I'd like the annotation to stay at Mapping, just because I don't want to commit the types to a specific dictionary type. If we did, user code would likely have to be updated if we switched, which seems undesirable.

I agree that we should check mapping types. Maybe something like this would work:

if __debug__:
    def __attrs_post_init__(self):
        # Mapping types need to match across instances, otherwise hashes might disagree.
        assert isinstance(self.bindings, immutabledict)
inducer commented 1 month ago

Could you do a PR?