Runtime: Possible type equality bug using generics and the `==` operator after deserialization.

[Carl-Hugo]

Title

Possible type equality bug using generics and the == operator after deserialization.

Scenario

I was implementing an IEqualityComparer<T> to simplify my life, and all my tests worked fine until I compared a deserialized version of the same objects.

Functional impact

The == operator does not behave the same after deserialization as when no serialization is involved.

The comparison of two values using the == operator should be the same with or without serialization involved (consistency).

Minimal repro steps

I created a full repro project here, including a workaround (under MyClassEqualityComparer.AlternateEquals(...)):
https://github.com/Carl-Hugo/ReproGenericEqualityNetCoreBug

Please note that, for simplicity sake, MyClassEqualityComparer does not implement IEqualityComparer<T> since it was not needed.

The minimal repro steps are:

namespace ReproProject
{
    public class MyClass<T>
        where T : class
    {
        public MyClass() { }

        public MyClass(T value)
        {
            Value = value;
        }

        public virtual T Value { get; set; }
    }

    public class MyClassEqualityComparer<T>
        where T : class
    {
        public bool Equals(MyClass<T> x, MyClass<T> y)
        {
            var sameValue = x.Value == y.Value;
            return sameValue;
        }
    }
    public class Program
    {
        private static MyClassEqualityComparer<string> Comparer { get; } = new MyClassEqualityComparer<string>();

        public static void Main(string[] args)
        {
            var object1 = new MyClass<string>("My value 1");
            var object2 = new MyClass<string>("My value 1");

            var serializedObject1 = JsonConvert.SerializeObject(object1);
            var serializedObject2 = JsonConvert.SerializeObject(object2);

            var deserializedObject1 = JsonConvert.DeserializeObject<MyClass<string>>(serializedObject1);
            var deserializedObject2 = JsonConvert.DeserializeObject<MyClass<string>>(serializedObject2);

            var valuesAreEquals = Comparer.Equals(deserializedObject1, deserializedObject2); // this should be true since `"My value 1" == "My value 1"`
            Console.WriteLine($"Values ({deserializedObject1.Value}|{deserializedObject2.Value}), with serialization, should be equals: {valuesAreEquals}");
            Console.Read();
        }
    }
}

Expected result

The method call Comparer.Equals(...) should return true since "My value 1" == "My value 1", compared by the following: x.Value == y.Value.

Actual result

The result is false.

Conclusion

I hope this is the right place; it is a bit hard to figure out to what repo a general issue should be posted to.

If it is not, feel free to point me the right way :)

[JonHanna]

The == within Equals(MyClass<T> x, MyClass<T> y) is going to be a reference-identity-equals, which we'd expect to return true with the object1 and object2 because the strings are the very same string, taken from the intern pool and false with the other cases as the strings have been re-created during deserialisation and so are not reference-identical-equal to the other strings.

[jnm2]

This is often what you want with generics:

```c#
var sameValue = EqualityComparer.Default.Equals(x.Value, y.Value);

(If the two values are typed as `object` rather than `T`, you usually want `Equals(objA, objB)`.)

As @JonHanna pointed out, your original code compiles to the same thing as this code:

```c#
var sameValue = ReferenceEquals(x.Value, y.Value);

[JonHanna]

Yeah, the code @jnm2 gives will fix your bug, though for more resilient code you might want to check on whether x or y are null, unless you can be 100% sure that won't happen.

[jnm2]

@JonHanna Good point. If there's a semantic difference between MyClass<>(null) and null, which is how my code usually would be, you'd want this:

c# if (x == null) return y == null; if (y == null) return false; var sameValue = EqualityComparer<T>.Default.Equals(x.Value, y.Value);

That way you catch bugs, because in my experience comparing things like MyClass<>(null) to null is almost always an implementation error. There's the ?. operator if you really want them to be treated the same.

[JonHanna]

That and if x is null then Default.Equals(x.Value, y.Value) is going to throw, which was the main thing I was thinking about there.

More generally, a useful ordering of the checks for null is:

C# if (x == y) return true; if (x == null | y == null) return false; // do more complicated equality comparison here.
The first if handles both the both-null case and the both-same-instance case simultaneously and it often helps to skip on both-same (consider if you have to look at a lot of data before you know you have equality, and the advantage of skipping) and when it doesn't give a performance boost, it doesn't hurt.

(If == is overloaded an not being turned into reference-identity-equals by the sort of generic situation this issue was about in the first place then call ReferenceEquals()).

The use of | over || in the second if is a much more marginal micro-opt, and examples could no doubt be found where it's worse, but nullity checks are very likely to be cheaper than having another branch so I tend to have that as the default go-to code unless it's worth measuring both | and || over realistic data.

[jnm2]

@JonHanna That assumes that reference equality implies domain-specific equality. It sounds like a given, but imagine that T implements IEquatable<T>.Equals for custom domain equality. Further down the chain as part of that custom comparison, two enumerables are compared for sequence equality. Since an enumerable has the potential to generate two different sequences on subsequent calls, you'd want to ignore the fact that they are the same instance of T and actually compare it to itself, which lower down enumerates twice to make sure that you are actually saying something about the sequences it generates.

Sounds far-fetched, but if you're in a separate library at the top of the food chain, you don't want to short-cut decisions like this for someone lower down. I can construct a fake example that doesn't sound that far-fetched.

See *EqualityComparer.Equals implementations.

[JonHanna]

That assumption fits with the documented requirements of IEqualityComparer<T>.Equals and object.Equals, https://msdn.microsoft.com/en-us/library/system.collections.iequalitycomparer.equals(v=vs.110).aspx and https://msdn.microsoft.com/en-us/library/bsc2ak47(v=vs.110).aspx

[jnm2]

@JonHanna Interesting. For object.Equals:

x.Equals(x) returns true, except in cases that involve floating-point types. See ISO/IEC/IEEE 60559:2011, Information technology -- Microprocessor Systems -- Floating-Point arithmetic.

So shouldn't MyClass<double>.Equals called on itself return false if the wrapped value should compare inequal with itself?

There must have been heavy discussion on the implementation of the equality comparers and I'd love to hear why they landed on that they did, which happens to not assume a reflexive underlying comparison.

[JonHanna]

Yeah, it's a tricky one for double (the same is also true of float), note that EqualityComparer<double>.Default.Equals(double.NaN, double.NaN) and most of the time if you aren't actually doing arithmetic, that's what you want, but there are times you want the arithmetic definition of NaN != NaN. A similar thing is true in ordering.

[JonHanna]

(We're totally being too chatty in someone's issue though, ping me on slack to continue our aside).

[JonHanna]

And I finally found a reference to the actual behaviour in question here, though not the full standard one I was looking for. From https://msdn.microsoft.com/en-us/library/d5x73970.aspx

When applying the where T : class constraint, avoid the == and != operators on the type parameter because these operators will test for reference identity only, not for value equality. This is the case even if these operators are overloaded in a type that is used as an argument. The following code illustrates this point; the output is false even though the String class overloads the == operator.

This is the behaviour seen in the code under discussion here.

[Carl-Hugo]

Then case closed, this is not a bug.
Thanks for the quick replies @JonHanna, I would never have found this one...

That being said, my fix is to use the Equal() method of the string property instead of the == operator: var sameValue = x.Value.Equals(y.Value);, after null checking and all.

[jnm2]

@JonHanna what slack channel? (I randomly came across yet another example in HashSet<>.HashSetEquals.)

[JonHanna]

I'm normally idling in #corefx That case though is known to be buggy with a couple of different issues, see dotnet/runtime#18931