Calling Equals with arguments of different types

[bentefay]

Hi Stephen,

Given a type deriving from EquatableBase:

        public class ReferenceType: EquatableBase<ReferenceType>
        {
            static ReferenceType()
            {
                DefaultComparer = EqualityComparerBuilder
                    .For<ReferenceType>()
                    .EquateBy(x => x.Value);
            }
            
            public string Value { get; set; }
        }

I was expecting that calling Equals(new ReferenceType { Value = "" }, "Hi!") would return false. Instead, I got an exception Unable to cast object of type 'System.String' to type 'ReferenceType'. Flipping the argument order does return false, as expected. I can see this is because ComparableImplementations.ImplementEquals<T>(IEqualityComparer<T> equalityComparer, T @this, object obj) immediately casts obj to T.

It looks like this is probably intended, but I just wanted to confirm, as this behaviour was unexpected for me. I understand this is a complicated topic, so I'm probably completely wrong here, but it seems like it would be safer to check whether obj is of type T before casting? Would this have some major downsides?

Just to give some context, I ran into this issue when using Json.Net serializer, which calls Equals with parameters of various types from the object graph being serialized. It took me a while to realize the problem was actually with the equality implementation, not the serializer.

[StephenCleary]

I'm not sure this is desirable behavior. In general, equality and other comparisons get really murky when you start dealing with different types - particularly in object hierarchies. But I wouldn't expect it to throw.

I'll take a deeper look at this in a bit.

[StephenCleary]

Indeed, this is an issue, and there's more than one instance of this in the codebase.

I'm currently writing some exhaustive unit tests that should flesh them all out, and then I'll start fixing.

[bentefay]

Great! Thanks for taking the time to investigate and fix the issue Stephen.

[StephenCleary]

Thanks for sharing the problem! It can be expressed as two separate problems:

1. Comparing different types would do a cast, resulting in InvalidCastException instead of the proper ArgumentException.
2. For the same reason, comparing a value of null from a nongeneric API for a comparer of a non-nullable type would result in a NullReferenceException.

These problems exist in Compare, Equals, and GetHashCode. They're actually caused by the same cast, but the solutions are different for each:

1. Incompatible comparisons will raise ArgumentException.
2. The "special null handling" flag will be ignored if the compared type is a non-nullable type.

I've got a few more commits to get these fixes in; these fixes will be released in v5.0.6.

[StephenCleary]

We now have thorough unit tests, and this fix will roll out in v5.0.6 shortly.