Commutative equality

[straight-shoota]

While we're talking about strictness of the eq expectation in #13389, I'd like to push some other though that has bugged me for a while.

Equality is usually expected to be commutative (i.e. from a == b follows b == a). That's a practical assumption, but not necessarily the case. If a and b have different types (say A and B), it might be that only one leg, A#==(B) or B#==(A) is implemented and the inverse would fall back to the default implementation which always returns false (ref #10277).

In stdlib there are a couple of cases where this applies. They're related to types that imitate a wrapped type, namely YAML::Any and Log::Metadata::Value.

require "log"

a = Log::Metadata::Value.new("foo")
b = "foo"

a == b # => true
b == a # => false

I think that these types that implement custom equality methods should do the same on the types they define equality with.

Having specs ensure equality in both directions should go a long way to provide sanity on this topic.

If type strictness gets implemented as proposed in #13389, this will also cover specs for all such cases because a eq b requires both operands to have the same type. Otherwise, it would be good for eq to test reverse equality.

Patch

```diff --- i/src/spec/expectations.cr +++ w/src/spec/expectations.cr @@ -16,7 +16,7 @@ module Spec actual_value.bytesize == expected_value.bytesize && actual_value.size == expected_value.size else - actual_value == @expected_value + (actual_value == @expected_value) && (@expected_value == actual_value) end end ```

Failing examples with this patch:

``` 1) YAML::Schema::Core parses "!!set { 1, 2, 3 }" Failure/Error: YAML::Schema::Core.parse(string).should eq(expected) Expected: Set{1, 2, 3} : Set(Int32) got: Set{1, 2, 3} : YAML::Any # spec/std/yaml/schema/core_spec.cr:6 2) YAML::Schema::Core parses "!!binary aGVsbG8=" Failure/Error: YAML::Schema::Core.parse(string).should eq(expected) Expected: Bytes[104, 101, 108, 108, 111] : Slice(UInt8) got: Bytes[104, 101, 108, 108, 111] : YAML::Any # spec/std/yaml/schema/core_spec.cr:6 3) YAML::Schema::Core parses "!!timestamp 2010-01-02" Failure/Error: YAML::Schema::Core.parse(string).should eq(expected) Expected: 2010-01-02 00:00:00.0 UTC : Time got: 2010-01-02 00:00:00.0 UTC : YAML::Any # spec/std/yaml/schema/core_spec.cr:6 4) Log::Metadata []? Failure/Error: md[:a]?.should eq(3) Expected: 3 : Int32 got: 3 : Log::Metadata::Value # spec/std/log/metadata_spec.cr:89 5) Log::Metadata [] Failure/Error: md[:a].should eq(3) Expected: 3 : Int32 got: 3 : Log::Metadata::Value # spec/std/log/metadata_spec.cr:81 6) HTTP::Server::RequestProcessor does not bleed Log::Context between requests Failure/Error: logs.entry.context[:foo].should eq "bar" Expected: "bar" : String got: "bar" : Log::Metadata::Value # spec/std/http/server/request_processor_spec.cr:345 7) HTTP::Server::RequestProcessor catches raised error on handler and retains context from handler Failure/Error: logs.entry.context[:foo].should eq "bar" Expected: "bar" : String got: "bar" : Log::Metadata::Value # spec/std/http/server/request_processor_spec.cr:289 8) HTTP::Client logging emit logs Failure/Error: logs.entry.data[:method].should eq("GET") Expected: "GET" : String got: "GET" : Log::Metadata::Value # spec/std/http/client/client_spec.cr:445 9) HTTP::Client logging emit logs with block Failure/Error: logs.entry.data[:method].should eq("GET") Expected: "GET" : String got: "GET" : Log::Metadata::Value # spec/std/http/client/client_spec.cr:459 Finished in 1:31 minutes 15327 examples, 9 failures, 0 errors, 20 pending Failed examples: crystal spec spec/std/yaml/schema/core_spec.cr:184 # YAML::Schema::Core parses "!!set { 1, 2, 3 }" crystal spec spec/std/yaml/schema/core_spec.cr:192 # YAML::Schema::Core parses "!!binary aGVsbG8=" crystal spec spec/std/yaml/schema/core_spec.cr:235 # YAML::Schema::Core parses "!!timestamp 2010-01-02" crystal spec spec/std/log/metadata_spec.cr:86 # Log::Metadata []? crystal spec spec/std/log/metadata_spec.cr:78 # Log::Metadata [] crystal spec spec/std/http/server/request_processor_spec.cr:325 # HTTP::Server::RequestProcessor does not bleed Log::Context between requests crystal spec spec/std/http/server/request_processor_spec.cr:271 # HTTP::Server::RequestProcessor catches raised error on handler and retains context from handler crystal spec spec/std/http/client/client_spec.cr:438 # HTTP::Client logging emit logs crystal spec spec/std/http/client/client_spec.cr:453 # HTTP::Client logging emit logs with block ```

[straight-shoota]

Additionally, there would also be a technical option to ensure commutativity of #== by the compiler. For example. If there is no specific implementation for b == a and the call would resolve to the default implementation, the compiler could theoretically reverse the operands and transform b == a into a == b.

But this would likely still be brittle. Specific implementations could still revert to the default implementation for some cases. Or even if there are specific implementations for both directions, does not necessarily mean they are required to agree 😆 So I don't think this would lead anywhere (nor would making everything more strict). At least it's always necessary having tests to ensure expected behaviour. Doing some compiler magic won't help about that, it's just more effort and more complex behaviour for developers to understand.

[j8r]

Any comparison operator can be concerned by such inconsistencies, like ===, >=, <=, < and >.

[straight-shoota]

Good point. However comparison operates are typically implemented via the Comparable interface based on <=> which ensures consistency to a high degree. There would only be a chance for error when implementing the comparison operators manually, or when A includes Comparable(B) but B does not include Comparable(A).

The case equality operator === though is explicitly directional:

(1..2) === 1 # => true
1 === (1..2) # => false

So I'd argue it's not actually an equality, it's more pattern matching. But that's a different story.

There's also the pattern match operator =~ which I guess is symmetric in practice, because in stdlib it's only defined between String and Regexp in both directions. But I'm not sure it's required to be that. Actually, I'm not even sure about the semantic difference between === and =~ except that the former usually expands on == and its return value is usually Bool. Anyway, I those would be excluded here because they're either explicitly not symmetric or not necessarily so.

[HertzDevil]

For standard library specs we could consider using something like this in as many places as possible:

https://github.com/crystal-lang/crystal/blob/98c091ee9dd5a0e75b3dd47b642bd170b4c84a39/spec/std/big/big_rational_spec.cr#L8-L23

[straight-shoota]

Like I said in the previous post, I don't think there is much need for this. The only implementations of these comparison operators in stdlib are in Class and Comparable. And the integer primitives. All of those are already extensively tested.

All other types implement the derived operators through Comparable. So tests really only need to cover the respective comparison operator <=> implementation.