JuliaServices / AutoHashEquals.jl

A Julia macro to add == and hash() to composite types.
Other
58 stars 12 forks source link

== vs isequal #18

Closed colinxs closed 1 year ago

colinxs commented 4 years ago

I'm curious why AutoHashEquals defines == in terms of isequal. In the README it says:

we use isequal() because we want to match Inf values, etc.

but it seems like the following would make more sense:

Base.hash(a::Foo, h::UInt) = hash(a.b, hash(a.a, hash(:Foo, h)))
Base.(:(==))(a::Foo, b::Foo) = a.b == b.b && a.a == b.a && true
Base.isequal(a::Foo, b::Foo) = isequal(a.b, b.b) && isequal(a.a, b.a) && true

This should still guarantee that hash(a) == hash(b) implies isequal(a, b). Looking at the definitions in Base for AbstractArray it seems like this is correct: isequal(A, B) applies isequal element-wise and ==(A, B) applies == element-wise.

Code for reference: Base.(:==) Base.isequal

goretkin commented 3 years ago

I agree that the above definitions would make auto-hash-equal'd structs behave like other containers.

Compare (with current release of AutoHashEquals.jl)

julia> using AutoHashEquals

julia> @auto_hash_equals struct Foo{T}
       _::T
       end

julia> ==(Foo(0/0), Foo(0/0))
true

julia> isequal(Foo(0/0), Foo(0/0))
true

julia> ==(Foo(missing), Foo(missing))
true

julia> isequal(Foo(missing), Foo(missing))
true

to

julia> ==((0/0,),  (0/0,))
false

julia> isequal((0/0,),  (0/0,))
true

julia> ==((missing,),  (missing,))
missing

julia> isequal((missing,),  (missing,))
true
ericphanson commented 1 year ago

@gafter I saw you had a big overhaul of this package in https://github.com/JuliaServices/AutoHashEquals.jl/pull/32; I was wondering, do you have an opinion on this issue?

gafter commented 1 year ago

My opinion is that this is likely a change that this package should and will make. Do you want to offer a PR?

ericphanson commented 1 year ago

Sure, I can do that. Should it be opt-in and non-breaking, or just a breaking change?

BTW: my reasoning for why this is the right choice is a tiny bit different than the original post, in that I don't think Array is the right comparison, but rather NamedTuple's. I think they are essentially the canonical "anonymous record type", using "record type" to refer to a struct that is defined directly by the data stored in its fields (in contrast to a struct used to implement another data structure, like the struct definition of a Dict), and that AutoHashEquals is most useful/relevant for these types. NamedTuples define == in terms of == of the entries, and the same with isequal. Also, Legolas.jl makes the same choices too, albeit with a bug: https://github.com/beacon-biosignals/Legolas.jl/issues/101.