Commutative equality

[straight-shoota]

While we're talking about strictness of the eq expectation in #13389, I'd like to push some other thought 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

--- 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.

[straight-shoota]

I'd like to retract my previous comment and state the opposite. Even though in stdlib most comparison operators are implemented via Compareable, there are still types which define the operators directly. And these spec helpers are not exclusively for stdlib.

I believe a commutativity check could be implemented quite easily be doubling all expectations that are supposed to be commutative. The patch is pretty simple:

diff --git c/src/spec/expectations.cr w/src/spec/expectations.cr
index f50658a5d..29e65cf74 100644
--- c/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 == actual_value
       end
     end

@@ -192,13 +192,13 @@ module Spec
     def match(actual_value)
       case @op
       in .less_than?
-        actual_value < @expected_value
+        actual_value < @expected_value > actual_value
       in .less_or_equal?
-        actual_value <= @expected_value
+        actual_value <= @expected_value >= actual_value
       in .greater_than?
-        actual_value > @expected_value
+        actual_value > @expected_value < actual_value
       in .greater_or_equal?
-        actual_value >= @expected_value
+        actual_value >= @expected_value <= actual_value
       end
     end

Draft PR: https://github.com/crystal-lang/crystal/pull/15733

Testing this against stdlib specs, there are a couple legitimate failures (i.e. actual bugs), caused by missing overloads for #== receiving wrapper types. I've already submitted a patch to fix these: https://github.com/crystal-lang/crystal/pull/15732

So this additional validation helps to detect deficiencies in the API.

Testing across test-ecosystem, there are only violations in a couple of shards. All of them again on the == operator and related to wrapper types similar to JSON::Any in https://github.com/athena-framework and https://github.com/crystal-community/msgpack-crystal (see test-ecosystem log). I have not looked at each individual failure in detail, but they appear to be very similar to the ones in stdlib, indicating legitimate bugs.

There could be some edge cases where a comparison operator is not expected to be commutative for whatever reason. I believe this would be very rare, though. And probably not very legit. Anyway, I don't think it's relevant to provide a helper for that in the standard library. As an alternative, you can build a custom expectation, or just test the comparison operators directly (e.g. (a > b).should be_true).

[ysbaddaden]

Some raw reactions:

• Math says it's commutative, but in crystal many math operations aren't commutative: for example UInt64 + UInt32. Switching these will change the result type, and possibly start raising math overflows.

• I'm not sure about specs enforcing the commutativity. Won't that just force monkey patching, or to use a much less explicit expectation? Also, it's a breaking change.

• Public value objects such as Any should indeed be commutative.

[straight-shoota]

Won't that just force monkey patching, or to use a much less explicit expectation?

Not sure what you mean by monkey patching. The expected effect should be that comparison operators are implemented as commutative, which may mean adding missing methods (see #15732).

I'd consider their absence a bug, and the commutative expectations helps discover these bugs. So I would find it acceptable to introduce such a change, even if it breaks behaviour. The breakages are indicators of bugs that exist regardless of the expectation. The details of how to introduce this with the least amount of disruption is of course subject to discussion.