diff --git a/src/fable-library-py/fable_library/record.py b/src/fable-library-py/fable_library/record.py index 49d33e7ec3..01cf9fc7d1 100644 --- a/src/fable-library-py/fable_library/record.py +++ b/src/fable-library-py/fable_library/record.py @@ -5,7 +5,7 @@ from .bases import ComparableBase, EquatableBase, HashableBase, StringableBase from .core import int32 from .protocols import IComparable -from .util import compare, equals +from .util import combine_hash_codes, compare, equals, structural_hash def record_compare_to(self: Record, other: Record) -> int: @@ -90,8 +90,11 @@ def record_to_string(self: Record) -> str: def record_get_hashcode(self: Record) -> int: + # Hash each field with `structural_hash` rather than hashing a Python tuple, + # so fields that only implement F# `GetHashCode` are hashed by value. slots = type(self).__slots__ - return hash(tuple(getattr(self, fixed_field) for fixed_field in slots)) + hashes = [structural_hash(getattr(self, fixed_field)) for fixed_field in slots] + return int(combine_hash_codes(hashes)) class Record(StringableBase, EquatableBase, ComparableBase, HashableBase, IComparable): @@ -129,6 +132,12 @@ def Equals(self, other: object) -> bool: def GetHashCode(self) -> int32: return int32(record_get_hashcode(self)) + def __hash__(self) -> int: + # EquatableBase declares __eq__, so Python implicitly sets __hash__ to + # None on it, and that None shadows HashableBase.__hash__ in the MRO. + # Redefine it here so records stay hashable through GetHashCode. + return int(self.GetHashCode()) + # ------------------------------------------------------------------------- # IComparable - Comparison (used by ComparableBase) # ------------------------------------------------------------------------- diff --git a/src/fable-library-py/fable_library/union.py b/src/fable-library-py/fable_library/union.py index b92493854d..bd20a4f99e 100644 --- a/src/fable-library-py/fable_library/union.py +++ b/src/fable-library-py/fable_library/union.py @@ -11,7 +11,7 @@ from .bases import ComparableBase, EquatableBase, HashableBase, StringableBase from .core import int32 from .protocols import IComparable -from .util import compare +from .util import combine_hash_codes, compare, equal_arrays, number_hash, structural_hash class Union(StringableBase, EquatableBase, ComparableBase, HashableBase, IComparable): @@ -80,7 +80,9 @@ def Equals(self, other: Any) -> bool: return False if self.tag == other.tag: - return self.fields == other.fields + # Compare fields with F# equality, not Python `==`, so fields that + # only implement `Equals` are compared by value. + return equal_arrays(self.fields, other.fields) return False @@ -89,7 +91,18 @@ def Equals(self, other: Any) -> bool: # ------------------------------------------------------------------------- def GetHashCode(self) -> int32: - return int32(hash((self.tag, *self.fields))) + # Hash the tag and each field structurally instead of hashing a Python + # tuple, which would hash fields that only implement F# `GetHashCode` + # by identity. + hashes = [number_hash(self.tag)] + hashes.extend(structural_hash(x) for x in self.fields) + return combine_hash_codes(hashes) + + def __hash__(self) -> int: + # EquatableBase declares __eq__, so Python implicitly sets __hash__ to + # None on it, and that None shadows HashableBase.__hash__ in the MRO. + # Redefine it here so union values stay hashable through GetHashCode. + return int(self.GetHashCode()) # ------------------------------------------------------------------------- # IComparable - Comparison (used by ComparableBase) @@ -131,13 +144,16 @@ def tagged_union(tag: int): Additionally sets: - cls.tag = tag (numeric case discriminator) - cls.fields property (list of field values for backwards compat) - - cls.__hash__ from HashableBase (dataclass sets it to None when - generating __eq__, which would make union values unhashable) + + The dataclass is created with `eq=False` so that `Union.Equals` (F# + equality) is inherited instead of the generated `__eq__`, which would + compare fields with Python `==`. That also keeps dataclass from setting + `__hash__` to None, so union values stay hashable via `Union.__hash__`. """ def decorator[T](cls: type[T]) -> type[T]: # Apply dataclass internally - dc_cls: Any = dataclass(cls) + dc_cls: Any = dataclass(cls, eq=False) # Set the tag dc_cls.tag = tag @@ -151,11 +167,6 @@ def fields(self) -> Array[Any]: dc_cls.fields = fields - # dataclass() generates __eq__ and therefore sets __hash__ to None. - # Restore the hash protocol so union values stay hashable via - # GetHashCode, e.g. when a union is a field of another union. - dc_cls.__hash__ = HashableBase.__hash__ - return dc_cls return decorator diff --git a/src/fable-library-py/fable_library/util.py b/src/fable-library-py/fable_library/util.py index badbf5fb4e..49b9cc9a45 100644 --- a/src/fable-library-py/fable_library/util.py +++ b/src/fable-library-py/fable_library/util.py @@ -622,15 +622,14 @@ def id(o: Any) -> int: def safe_hash(x: Any) -> int32: - return ( - int32.ZERO - if x is None - else int32(hash(x)) - if is_hashable_py(x) - else x.GetHashCode() - if is_hashable(x) - else number_hash(ObjectRef.id(x)) - ) + """Hash a declared type that may or may not implement GetHashCode. + + F# `GetHashCode` takes precedence over Python's `__hash__`: an F# class + that overrides `Equals`/`GetHashCode` does not opt into the Python + protocols, so checking `__hash__` first would reach the inherited + `object.__hash__` and hash by identity instead. + """ + return identity_hash(x) def string_hash(s: str) -> int32: @@ -666,7 +665,35 @@ def combine_hash_codes(hashes: list[int32]) -> int32: def structural_hash(x: Any) -> int32: - return int32(hash(x)) + """Hash a value using F# structural semantics. + + Like `safe_hash`, F# `GetHashCode` wins over Python's `__hash__` so that + custom equality and hash overrides are honored. Arrays and tuples are + hashed element-wise instead of through Python's `hash`, which would fall + back to identity for their items. + """ + if x is None: + return int32.ZERO + + if isinstance(x, bool): + return int32.ONE if x else int32.ZERO + + if isinstance(x, str): + return string_hash(x) + + if is_hashable(x): + return x.GetHashCode() + + if isinstance(x, int | float): + return number_hash(x) + + if isinstance(x, Array | tuple | list): + return array_hash(cast(Iterable[object], x)) + + if is_hashable_py(x): + return int32(hash(x)) + + return number_hash(ObjectRef.id(x)) def array_hash(xs: Iterable[object]) -> int32: diff --git a/tests/Js/Main/ComparisonTests.fs b/tests/Js/Main/ComparisonTests.fs index 2366c60c7a..a2f02317a9 100644 --- a/tests/Js/Main/ComparisonTests.fs +++ b/tests/Js/Main/ComparisonTests.fs @@ -70,6 +70,22 @@ type Status = type MyClass(v) = member val Value: int = v with get, set +// See https://github.com/fable-compiler/Fable/issues/4834 +type CustomValue(value: int, label: string) = + member _.Value = value + member _.Label = label + + override _.Equals(other) = + match other with + | :? CustomValue as other -> value = other.Value + | _ -> false + + override _.GetHashCode() = value + +type WrappedUnion = WrappedUnion of CustomValue + +type WrappedRecord = { Value: CustomValue } + [] type FuzzyInt = | FuzzyInt of int @@ -507,6 +523,22 @@ let tests = (Test(1).GetHashCode(), Test(1).GetHashCode()) ||> equal (Test(2).GetHashCode(), Test(1).GetHashCode()) ||> notEqual + testCase "Custom Equals and GetHashCode are used by structural values" <| fun () -> // See #4834 + let left = CustomValue(1, "left") + let right = CustomValue(1, "right") + + left = right |> equal true + hash left = hash right |> equal true + + WrappedUnion left = WrappedUnion right |> equal true + hash (WrappedUnion left) = hash (WrappedUnion right) |> equal true + + { Value = left } = { Value = right } |> equal true + hash { Value = left } = hash { Value = right } |> equal true + + [| left |] = [| right |] |> equal true + hash [| left |] = hash [| right |] |> equal true + testCase "GetHashCode with same object works" <| fun () -> let o = OTest(1) let h1 = o.GetHashCode() diff --git a/tests/Python/TestComparison.fs b/tests/Python/TestComparison.fs index eec68c6c7b..fbcdeb0d94 100644 --- a/tests/Python/TestComparison.fs +++ b/tests/Python/TestComparison.fs @@ -69,6 +69,22 @@ type Status = type MyClass(v) = member val Value: int = v with get, set +// See https://github.com/fable-compiler/Fable/issues/4834 +type CustomValue(value: int, label: string) = + member _.Value = value + member _.Label = label + + override _.Equals(other) = + match other with + | :? CustomValue as other -> value = other.Value + | _ -> false + + override _.GetHashCode() = value + +type WrappedUnion = WrappedUnion of CustomValue + +type WrappedRecord = { Value: CustomValue } + [] let ``test PhysicalEquality works`` () = // See #3998 let r1 = ResizeArray([1; 2]) @@ -581,6 +597,23 @@ let ``test Classes must use identity hashing by default`` () = // ([1; 2].GetHashCode(), [1; 2].GetHashCode()) ||> equal // ([2; 1].GetHashCode(), [1; 2].GetHashCode()) ||> notEqual +[] +let ``test Custom Equals and GetHashCode are used by structural values`` () = // See #4834 + let left = CustomValue(1, "left") + let right = CustomValue(1, "right") + + left = right |> equal true + hash left = hash right |> equal true + + WrappedUnion left = WrappedUnion right |> equal true + hash (WrappedUnion left) = hash (WrappedUnion right) |> equal true + + { Value = left } = { Value = right } |> equal true + hash { Value = left } = hash { Value = right } |> equal true + + [| left |] = [| right |] |> equal true + hash [| left |] = hash [| right |] |> equal true + [] let ``GetHashCode with primitives works`` () = ((1).GetHashCode(), (1).GetHashCode()) ||> equal