Update == and isequal semantics to match NamedTuple's (#45)

Specifically:
- Define `==` in terms of `==` of members, properly handing `missing`.
- Define `isequal` in terms of `isequal` of members.
- Add an option `compat1=true` for clients who prefer the old behavior.
- This code tickles a bug in Julia that apparently is fixed in 1.8, so that is the new min version.
- Bump version to `2.0.0`

Co-authored-by: Neal Gafter <neal@gafter.com>

.github/workflows/CI.yml

@@ -18,9 +18,8 @@ jobs:
       fail-fast: false
       matrix:
         version:
-          - '1.6'
-          - '1.7'
           - '1.8'
+          - '1.8.2'
           - '1.9'
           - 'nightly'
         os:

Project.toml

@@ -1,10 +1,10 @@
 name = "AutoHashEquals"
 uuid = "15f4f7f2-30c1-5605-9d31-71845cf9641f"
 authors = ["Neal Gafter <neal@gafter.com>", "andrew cooke <andrew@acooke.org>"]
-version = "1.1.0"
+version = "2.0.0"
 
 [deps]
 Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
 
 [compat]
-julia = "1.6"
+julia = "1.8"

README.md

@@ -3,7 +3,7 @@
 
 # AutoHashEquals.jl - Automatically define hash and equals for Julia.
 
-A macro to add `==` and `hash()` to struct types: `@auto_hash_equals`.
+A macro to add `isequal`, `==`, and `hash()` to struct types: `@auto_hash_equals`.
 
 # `@auto_hash_equals`
 
@@ -24,10 +24,11 @@ struct Box{T}
     x::T
 end
 Base.hash(x::Box, h::UInt) = hash(x.x, hash(:Box, h))
-Base.(:(==))(a::Box, b::Box) = isequal(a.x, b.x)
+Base.(:(==))(a::Box, b::Box) = a.x == b.x
+Base.isequal(a::Box, b::Box) = isequal(a.x, b.x)
 ```
 
-We do not take the type arguments of a generic type into account for either `hash` or `==` unless `typearg=true` is specified (see below).  So a `Box{Int}(1)` will test equal to a `Box{Any}(1)`.
+We do not take the type arguments of a generic type into account for `isequal`, `hash`, or `==` unless `typearg=true` is specified (see below).  So by default, a `Box{Int}(1)` will test equal to a `Box{Any}(1)`.
 
 ## User-specified hash function
 
@@ -71,7 +72,12 @@ end
 function Base._show_default(io::IO, x::Box)
     AutoHashEqualsCached._show_default_auto_hash_equals_cached(io, x)
 end
+# Note: the definition of `==` is more complicated when there are more fields,
+# in order to handle `missing` correctly. See below for a more complicated example.
 function Base.:(==)(a::Box, b::Box)
+    a._cached_hash == b._cached_hash && Base.:(==)(a.x, b.x)
+end
+function Base.isequal(a::Box, b::Box)
     a._cached_hash == b._cached_hash && Base.isequal(a.x, b.x)
 end
 function Box(x::T) where T
@@ -106,16 +112,34 @@ end
 function Base.hash(x::Foo, h::UInt)
     Base.hash(x.b, Base.hash(x.a, Base.hash(:Foo, h)))
 end
-function (Base).:(==)(a::Foo, b::Foo)
+function Base.isequal(a::Foo, b::Foo)
     Base.isequal(a.a, b.a) && Base.isequal(a.b, b.b)
 end
+# Returns `false` if any two fields compare as false; otherwise, `missing` if at least
+# one comparison is missing. Otherwise `true`.
+# This matches the semantics of `==` for Tuple's and NamedTuple's.
+function Base.:(==)(a::Foo, b::Foo)
+    found_missing = false
+    cmp = a.a == b.a
+    cmp === false && return false
+    if ismissing(cmp)
+        found_missing = true
+    end
+    cmp = a.b == b.b
+    cmp === false && return false
+    if ismissing(cmp)
+        found_missing = true
+    end
+    found_missing && return missing
+    return true
+end
 ```
 
 ## Specifying whether or not type arguments should be significant
 
 You can specify that type arguments should be significant for the purposes of computing the hash function and checking equality by adding the keyword parameter `typearg=true`.  By default they are not significant.  You can specify the default (they are not significant) with `typearg=false`:
 
-```julia-repl
+```julia
 julia> @auto_hash_equals struct Box1{T}
            x::T
        end
@@ -161,3 +185,32 @@ If `typearg=true`, then `e(t)` is used as the type seed, where `t` is the type o
 
 Note that the value of `typeseed` is expected to be a `UInt` value when `typearg=false` (or `typearg` is not specified),
 but a function that takes a type as its argument when `typearg=true`.
+
+## Compatibility mode
+
+In versions `v"1.0"` and earlier of `AutoHashEquals`, we produced a specialization of `Base.==`, implemented using `Base.isequal`.
+This was not correct.
+See https://docs.julialang.org/en/v1/base/base/#Base.isequal and https://docs.julialang.org/en/v1/base/math/#Base.:==.
+More correct would be to define `==` by using `==` on the members, and to define `isequal` by using `isequal` on the members.
+In version `v"2.0"` we provide a correct implementation, thanks to @ericphanson.
+
+To get the same behavior as `v"1.0"` of this package, in which `==` is implemented based on `isequal`,
+you can specify `compat1=true`.
+
+```julia
+@auto_hash_equals struct Box890{T}
+    x::T
+end
+@assert ismissing(Box890(missing) == Box890(missing))
+@assert isequal(Box890(missing), Box890(missing))
+@assert ismissing(Box890(missing) == Box890(1))
+@assert !isequal(Box890(missing), Box890(1))
+
+@auto_hash_equals compat1=true struct Box891{T}
+    x::T
+end
+@assert Box891(missing) == Box891(missing)
+@assert isequal(Box891(missing), Box891(missing))
+@assert Box891(missing) != Box891(1)
+@assert !isequal(Box891(missing), Box891(1))
+```

src/AutoHashEquals.jl

@@ -10,7 +10,7 @@ include("impl.jl")
 """
     @auto_hash_equals [options] struct Foo ... end
 
-Generate `Base.hash` and `Base.==` methods for `Foo`.
+Generate `Base.hash`, `Base.isequal`, and `Base.==` methods for `Foo`.
 
 Options:
 
@@ -19,6 +19,7 @@ Options:
 * `fields=a,b,c` the fields to use for hashing and equality. Default: all fields.
 * `typearg=true|false` whether or not to make type arguments significant. Default: `false`.
 * `typeseed=e` Use `e` (or `e(type)` if `typearg=true`) as the seed for hashing type arguments.
+* `compat1=true` To have `==` defined by using `isequal`.  Default: `false`.
 """
 macro auto_hash_equals(args...)
     kwargs = Dict{Symbol,Any}()

src/impl.jl

@@ -153,6 +153,7 @@ function auto_hash_equals_impl(__source__::LineNumberNode, typ; kwargs...)
     fields=nothing
     typearg=false
     typeseed=nothing
+    compat1=false
 
     # Process the keyword arguments
     for kw in kwargs
@@ -181,17 +182,22 @@ function auto_hash_equals_impl(__source__::LineNumberNode, typ; kwargs...)
             fields = kw.second
         elseif kw.first === :typeseed
             typeseed = kw.second
+        elseif kw.first === :compat1
+            if !(kw.second isa Bool)
+                error_usage(__source__, "`compat1` argument must be a Bool, but got `$(kw.second)`.")
+            end
+            compat1 = kw.second
         else
             error_usage(__source__, "invalid keyword argument for @auto_hash_equals: `$(kw.first)`.")
         end
     end
 
     typ = get_struct_decl(__source__::LineNumberNode, typ)
 
-    auto_hash_equals_impl(__source__, typ, fields, cache, hashfn, typearg, typeseed)
+    auto_hash_equals_impl(__source__, typ, fields, cache, hashfn, typearg, typeseed, compat1)
 end
 
-function auto_hash_equals_impl(__source__, struct_decl, fields, cache::Bool, hashfn, typearg::Bool, typeseed)
+function auto_hash_equals_impl(__source__, struct_decl, fields, cache::Bool, hashfn, typearg::Bool, typeseed, compat1::Bool)
     is_expr(struct_decl, :struct) || error_usage(__source__)
 
     type_body = struct_decl.args[3].args
@@ -335,24 +341,56 @@ function auto_hash_equals_impl(__source__, struct_decl, fields, cache::Bool, has
         end))
     end
 
-    # Add the == function
-    equalty_impl = foldl(
-        (r, f) -> :($r && $isequal($getfield(a, $(QuoteNode(f))), $getfield(b, $(QuoteNode(f))))),
-        fields;
-        init = cache ? :(a._cached_hash == b._cached_hash) : true)
-    if struct_decl.args[1]
-        # mutable structs can efficiently be compared by reference
-        equalty_impl = :(a === b || $equalty_impl)
-    end
-    if isnothing(where_list) || !typearg
-        push!(result.args, esc(:(function $Base.:(==)(a::$type_name, b::$type_name)
-            $equalty_impl
-        end)))
-    else
-        # If requested, require the type arguments be the same for two instances to be equal
-        push!(result.args, esc(:(function $Base.:(==)(a::$full_type_name, b::$full_type_name) where {$(where_list...)}
-            $equalty_impl
-        end)))
+    # Add the `==` and `isequal` functions
+    for eq in (==, isequal)
+        # In compat mode, only define ==
+        eq == isequal && compat1 && continue
+
+        if eq == isequal || compat1
+            equality_impl = foldl(
+                (r, f) -> :($r && $isequal($getfield(a, $(QuoteNode(f))), $getfield(b, $(QuoteNode(f))))),
+                fields;
+                init = cache ? :(a._cached_hash == b._cached_hash) : true)
+            if struct_decl.args[1]
+                # mutable structs can efficiently be compared by reference
+                # Note this optimization is only valid for `isequal`, e.g.
+                # a = [missing]
+                # a == a # missing
+                # isequal(a, a) # true
+                equality_impl = :(a === b || $equality_impl)
+            end
+        else
+            # Julia library defines `isequal` in terms of `==`.
+            compat1 && continue
+
+            # Here we have a more complicated implementation in order to handle missings correctly.
+            # If any field comparison is false, we return false (even if some return missing).
+            # If no field comparisons are false, but one comparison missing, then we return missing.
+            # Otherwise we return true.
+            # (This matches the semantics of `==` for `Tuple`'s and `NamedTuple`'s.)
+            equality_impl = Expr(:block, :(found_missing = false))
+            if cache
+                push!(equality_impl.args, :(a._cached_hash != b._cached_hash && return false))
+            end
+            for f in fields
+                push!(equality_impl.args, :(cmp = $getfield(a, $(QuoteNode(f))) == $getfield(b, $(QuoteNode(f)))))
+                push!(equality_impl.args, :(cmp === false && return false))
+                push!(equality_impl.args, :($ismissing(cmp) && (found_missing = true)))
+            end
+            push!(equality_impl.args, :(return $ifelse(found_missing, missing, true)))
+        end
+
+        fn_name = Symbol(eq)
+        if isnothing(where_list) || !typearg
+            push!(result.args, esc(:(function ($Base).$fn_name(a::$type_name, b::$type_name)
+                $equality_impl
+            end)))
+        else
+            # If requested, require the type arguments be the same for two instances to be equal
+            push!(result.args, esc(:(function ($Base).$fn_name(a::$full_type_name, b::$full_type_name) where {$(where_list...)}
+                $equality_impl
+            end)))
+        end
     end
 
     # Evaluating a struct declaration normally returns the struct itself.

test/runtests.jl

@@ -20,9 +20,7 @@ function serialize_and_deserialize(x)
 end
 
 macro noop(x)
-    esc(quote
-       Base.@__doc__$(x)
-    end)
+    esc(x)
 end
 
 macro _const(x)
@@ -261,23 +259,31 @@ abstract type B{T} end
             @test hash(T135(1, :x)) == hash(serialize_and_deserialize(T135(1, :x)))
         end
 
-        @testset "contained NaN values compare equal" begin
+        @testset "contained NaN values compare isequal (but not ==)" begin
             @auto_hash_equals_cached struct T140
                 x
             end
             nan = 0.0 / 0.0
             @test nan != nan
-            @test T140(nan) == T140(nan)
+            @test isequal(T140(nan), T140(nan))
+            @test T140(nan) != T140(nan)
+
         end
 
-        @testset "ensure circular data structures, produced by hook or by crook, do not blow the stack" begin
+        @testset "circular data structures behavior" begin
             @auto_hash_equals_cached struct T145
                 a::Array{Any,1}
             end
             t::T145 = T145(Any[1])
             t.a[1] = t
+            # hash does not stack overflow thanks to the cache
             @test hash(t) != 0
-            @test t == t
+            # `==` overflows
+            @test_throws StackOverflowError t == t
+            # isequal does not
+            @test isequal(t, t)
+            @test !isequal(t, T145(Any[]))
+            # Check printing
             @test "$t" == "$(T145)(Any[$(T145)(#= circular reference @-2 =#)])"
         end
 
@@ -513,13 +519,14 @@ abstract type B{T} end
             @test hash(T313(1, :x)) == hash(serialize_and_deserialize(T313(1, :x)))
         end
 
-        @testset "contained NaN values compare equal" begin
+        @testset "contained NaN values compare isequal (but not ==)" begin
             @auto_hash_equals struct T330
                 x
             end
             nan = 0.0 / 0.0
             @test nan != nan
-            @test T330(nan) == T330(nan)
+            @test isequal(T330(nan), T330(nan))
+            @test T330(nan) != T330(nan)
         end
 
         @testset "give no error if the struct contains internal constructors" begin
@@ -840,6 +847,61 @@ abstract type B{T} end
             @test_throws MethodError hash(S681(1))
         end
 
+
+        @testset "== propogates missing, but `isequal` does not" begin
+            # Fixed by https://github.com/JuliaServices/AutoHashEquals.jl/issues/18
+            @auto_hash_equals struct Box18{T}
+                x::T
+            end
+            ret = Box18(missing) == Box18(missing)
+            @test ret === missing
+            ret = Box18(missing) == Box18(1)
+            @test ret === missing
+            @test isequal(Box18(missing), Box18(missing))
+            @test !isequal(Box18(missing), Box18(1))
+
+            @auto_hash_equals struct Two18{T1, T2}
+                x::T1
+                y::T2
+            end
+            ret = Two18(1, missing) == Two18(1, 2)
+            @test ret === missing
+
+            ret = Two18(5, missing) == Two18(1, 2)
+            @test ret === false
+
+            ret = Two18(missing, 2) == Two18(1, 2)
+            @test ret === missing
+
+            ret = Two18(missing, 5) == Two18(1, 2)
+            @test ret === false
+
+            @auto_hash_equals mutable struct MutBox18{T}
+                x::T
+            end
+            b = MutBox18(missing)
+            ret = b == b
+            @test ret === missing
+            @test isequal(b, b)
+        end
+
+        @testset "test the compat1 flag" begin
+            @auto_hash_equals struct Box890{T}
+                x::T
+            end
+            @test ismissing(Box890(missing) == Box890(missing))
+            @test isequal(Box890(missing), Box890(missing))
+            @test ismissing(Box890(missing) == Box890(1))
+            @test !isequal(Box890(missing), Box890(1))
+
+            @auto_hash_equals compat1=true struct Box891{T}
+                x::T
+            end
+            @test Box891(missing) == Box891(missing)
+            @test isequal(Box891(missing), Box891(missing))
+            @test Box891(missing) != Box891(1)
+            @test !isequal(Box891(missing), Box891(1))
+        end
     end
 end