Update == and isequal semantics to match NamedTuple's

[ericphanson]

closes #18

I went for the breaking change, since that is easier to implement and less confusing for users to have a setting that changes the semantics in subtle ways.

[ORBAT]

Personally I'd prefer to have a way to keep using the old semantics for ==, since in my current project I've defined my :(==)(a::T, b::T) methods in terms of isequal anyhow as its semantics are what I'd want from a == b. I don't need for my types' equality definition to be consistent with eg. NamedTuple and I always rather take semantics that won't annoy me with syntax that is much more pleasant to use than isequal(a, b).

Seems like it's important from an interface perspective that isequal(::T,::T) should always follow what isequal does, but I'm not sure if the same applies to upholding guarantees about == except in specific cases – eg. maybe exported types?

I wonder how common it is for folks to actually need the "base" == semantics, vs. == defined in terms of isequal?

[ericphanson]

The docstring of == says:

  ==(x, y)

  Generic equality operator. Falls back to ===. Should be implemented for all types with a notion of
  equality, based on the abstract value that an instance represents. For example, all numeric types are
  compared by numeric value, ignoring type. Strings are compared as sequences of characters, ignoring
  encoding. For collections, == is generally called recursively on all contents, though other properties
  (like the shape for arrays) may also be taken into account.

  This operator follows IEEE semantics for floating-point numbers: 0.0 == -0.0 and NaN != NaN.

  The result is of type Bool, except when one of the operands is missing, in which case missing is
  returned (three-valued logic (https://en.wikipedia.org/wiki/Three-valued_logic)). For collections,
  missing is returned if at least one of the operands contains a missing value and all non-missing
  values are equal. Use isequal or === to always get a Bool result.

I think it is hard to decide exactly how to apply this for AutoHashEquals; if the type is a "container", then it seems clear that we should have the missing semantics that this PR provides. If the type is a wrapper around a numeric type and that wrapper itself should be seen as a number, then it should have the IEEE semantics this PR provides. But in general all we are told is that:

Should be implemented for all types with a notion of equality, based on the abstract value that an instance represents.

Maybe it should be another keyword argument? It seems hard to say in general exactly what the right notion of equality is when we don't know what the type is for.

[gafter]

Maybe it should be another keyword argument? It seems hard to say in general exactly what the right notion of equality is when we don't know what the type is for.

If the type represents a number, I don't think we will be able to implement the correct semantics. The user has to do it in that case. I think this macro is just for container-like types.

[gafter]

Personally I'd prefer to have a way to keep using the old semantics for ==, since in my current project I've defined my :(==)(a::T, b::T) methods in terms of isequal anyhow as its semantics are what I'd want from a == b.

@ORBAT You can always do this:

@auto_hash_equals struct MyType ... end
Base.:(==)(a::MyType, b::MyType) = isequal(a, b)

You could even make your own helper macro to do that.

Does your current project really have missing values in the data structures? If not, you don't have to worry about this.

[gafter]

@ericphanson How would you feel about adding an option, equalsonly, default to false. When it is true, implements the old behavior (define only == in terms of isequal). When it is false (the default), implement the new behavior.

[ORBAT]

@gafter yeah true, I can always go with a macro that does == the way I like.

I don't currently have anything that uses missing values "on purpose", but I'm building a framework so the idea is more to make sure that potential missing values in user inputs won't screw things up, and that == for internal types works "correctly" by default

[gafter]

and that == for internal types works "correctly" by default

Your concept of "correct" disagrees with the library.

You're supposed to use isequal if you implement something like a hash table.

[gafter]

@ericphanson Could you please update to the latest master branch and add the keyword option we discussed? Alternatively, let me know that you cannot do so and I could give it a try.

[ORBAT]

and that == for internal types works "correctly" by default

Your concept of "correct" disagrees with the library.

You're supposed to use isequal if you implement something like a hash table.

I know, hence the scare quotes. I simply prefer the semantics of isequal compared to vanilla == and I don't need to care about "actual correctness" in personal projects, which is why I've liked the current implementation where == uses isequal under the hood

[gafter]

@ericphanson This PR was broken, so I made a couple of fixes. Please look at them.

I then tried to merge with master. After doing so, all the tests pass on Julia 1.8 and later, but Julia crashes on 1.6 and 1.7:

     Testing Running tests...
rosetta error: unexpectedly need to EmulateForward on a synchronous exception x86_rip=0x4512647104 arm_pc=0x4520857412 num_insts=6 inst_index=3 x86 instruction bytes: 0x6215344901283465301 0x17041981987679720769

I think we should have it working smoothly on 1.6, since that is a long-term-support release. I'm not sure what the next step is. I'll play for a bit to try to fix it, but if I cannot do so I'll ask the Julia gurus where to go from here.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@8967719). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff            @@
##             master      #45   +/-   ##
=========================================
  Coverage          ?   92.33%           
=========================================
  Files             ?        3           
  Lines             ?      300           
  Branches          ?        0           
=========================================
  Hits              ?      277           
  Misses            ?       23           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I have fixed the things I requested be fixed.

[gafter]

@ORBAT @ericphanson Please have a look at the changes I've made to this PR. Are you satisfied with this approach?

[nickrobinson251]

LGTM - thanks @ericphanson and @gafter

[ericphanson]

sorry, I've been on vacation and haven't been following github much. Thanks for finishing this up and getting it merged!