ValueTuple throws for null comparer, unlike other BCL APIs

[jnm2]

Continuing from https://github.com/dotnet/corefx/issues/18432.

You would expect this to succeed, but it throws NullReferenceException:

((IStructuralEquatable)(1, 2)).Equals((1, 2), null)

I'll quite often take advantage of the fact that all BCL APIs use EqualityComparer<T>.Default when you pass null, and chain constructors and other methods with the parameter IEqualityComparer<T> comparer = null. If my own constructor or extension method takes IEqualityComparer<T> comparer = null, I assume that I can pass that into the BCL method. It's not intuitive to make it the call site's responsibility to check for null and pass EqualityComparer<object>.Default or call one or the other BCL overload depending whether comparer = null.

It's not critical since the workaround is to pass comparer ?? EqualityComparer<object>.Default instead of comparer. This is an API gotcha that may go unnoticed until code is in the field though.

ValueTuple<*>.IStructuralEquatable.Equals and ValueTuple<*>.IStructuralComparable.CompareTo have no null comparer check. If it followed the convention set by all other BCL methods, it would use EqualityComparer<object>.Default if you pass null.

Array.IStructuralEquatable.Equals and Array.IStructuralComparable.CompareTo, and Tuple<*>.IStructuralEquatable.Equals and Tuple<*>.IStructuralComparable.CompareTo have the same problem. They are in coreclr. Should I open an issue over there?

[safern]

cc: @stephentoub @ianhays @danmosemsft

[danmoseley]

I think @jcouv owns VAlueTuple.

[jcouv]

ValueTuple is modeled very closely after Tuple types. One reason is to enable switching (this is especially important in F#, since it had syntactic sugar for Tuple for a while now). So if we change one, we'd have to change both.
And it sounds like it would be beyond those two sets of types (as you have listed a number of other types with the same behavior).
And then there is the question of how the change affects API customers. Is such a change considered breaking?

Given that ValueTuple is a value type, I don't feel that comparison with null is a common scenario. So I would not use it as poster case to drive a decision.
If there is a broad decision to make this change across multiple other BCL types, I can update Tuple and ValueTuple to match whatever new convention is decided.

[svick]

Given that ValueTuple is a value type, I don't feel that comparison with null is a common scenario.

How does ValueTuple being a value type matter? This issue is not about comparing the tuple with null, it's about passing null for the comparer parameter of IStructuralEquatable.Equals (and similar methods).

[jnm2]

@jcouv It's not a number of types. Out of dozens of types, the odd ones out now are ValueTuple, Tuple, and Array, and only when they are accessed via IStructuralEquatable and IStructuralComparable. And it has nothing to do with the value being compared. The BCL convention is for null comparer to indicate the default comparer, for good reasons listed in the original post.

[jcouv]

Thanks for the clarification. I'll follow up this week with @stephentoub and @AlexGhiondea to come to a decision for Tuple and ValueTuple, for corefx and desktop.

[jcouv]

Sounds like a reasonable proposal, although not high priority.
From discussion with Alex and Wes, this would need vetting with compat council and also need fixing across the board (3 implementations: desktop, coreCLR and coreFX) for consistency. I haven't not checked with F# folks yet.

In any case, I most likely will not be able to get to this in the next couple of weeks to catch this wave.
Marking as up-for-grabs.

[jnm2]

@jcouv I'm motivated to get this through. I'll go ahead and PR if that's okay.

[jcouv]

@jnm2 That'd be awesome! Thanks for offering :-)
The window for .NET Core 2.0 is 5/10. If you take care of coreCLR and coreFX, I will take on the desktop framework side of things.
I will also start internal thread with compatibility council to confirm they are ok with API behavior change (for both Tuple and ValueTuple). I expect it should be ok.
I'll also ping the F# team to confirm Tuple change is ok with them (since they are heavy users).

Note that tests live in coreFX. To test a coreCLR change against existing coreFX tests, see instructions.

While you're in there, would you consider making this optimization (https://github.com/dotnet/corefx/issues/18666, suggested by @azhmur)?

[jnm2]

@jcouv Can I target ValueTuple, Tuple and Array all at once then?

While you're in there, would you consider making this optimization (#18666, suggested by @azhmur)?

Sure. Separate PR?

[jnm2]

@jcouv @omariom says 18666 is waiting for https://github.com/dotnet/coreclr/issues/6688. Can you clarify?

[jcouv]

@jnm2 Separate PR is probably safer. Thanks

Yes, I think doing all three types (ValueTuple, Tuple and Array). I'm waiting to hear back from compatibility council and F# team.

For the ValueTuple optimization, the optimization would not be required if the runtime could do it itself (dotnet/coreclr#6688). But I don't expect the runtime issue to make progress soon, so it is good to optimize from the library itself.

[jnm2]

@jcouv Which is better?

Acts just like the comparerless Equals(object) implementation by using three different comparers; boxes:

        bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
        {
            if (other == null || !(other is ValueTuple<T1, T2, T3>)) return false;

            var objTuple = (ValueTuple<T1, T2, T3>)other;

            return (comparer ?? EqualityComparer<T1>.Default).Equals(Item1, objTuple.Item1)
                && (comparer ?? EqualityComparer<T2>.Default).Equals(Item2, objTuple.Item2)
                && (comparer ?? EqualityComparer<T3>.Default).Equals(Item3, objTuple.Item3);
        }

Fewer branches, acts just like the comparerless Equals(object) implementation by using three different comparers; does not box if it can help it:

        bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
        {
            if (other == null || !(other is ValueTuple<T1, T2, T3>)) return false;

            var objTuple = (ValueTuple<T1, T2, T3>)other;
            
            if (comparer == null) return Equals(objTuple);
            return comparer.Equals(Item1, objTuple.Item1)
                && comparer.Equals(Item2, objTuple.Item2)
                && comparer.Equals(Item3, objTuple.Item3);
        }

Fewer branches, acts more like the normal logic of using a single comparer, also boxes:

        bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
        {
            if (other == null || !(other is ValueTuple<T1, T2, T3>)) return false;

            var objTuple = (ValueTuple<T1, T2, T3>)other;
            
            if (comparer == null) comparer = EqualityComparer<object>.Default;
            return comparer.Equals(Item1, objTuple.Item1)
                && comparer.Equals(Item2, objTuple.Item2)
                && comparer.Equals(Item3, objTuple.Item3);
        }

[jnm2]

I would typically lean towards if (comparer == null) comparer = EqualityComparer<object>.Default; since that is the exact analog of every other BCL method, but that is guaranteed to just end up calling ValueTuple<*>.Equals(object) which calls ValueTuple<*>.Equals(ValueTuple<*>) and uses three different comparers, so we could optimize all that away without side effects by going with the second code sample.

[jcouv]

@jnm2
The third option also makes more sense to me (comparer = comparer ?? EqualityComparer<object>.Default;).

but that is guaranteed to just end up calling ValueTuple<>.Equals(object) which calls ValueTuple<>.Equals(ValueTuple<*>) and uses three different comparers

I didn't understand that part. The comparison at this point is on elements, so ValueTuple<*>.Equals(object) method should not be involved.

[jnm2]

@jcouv Right, I mixed myself up there. So we'd want to call EqualityComparer<object>.Default.Equals on each element because that is an observable behavioral change compared to calling EqualityComparer<T*>.Default.Equals. Perfect.

[jnm2]

@jcouv Last question, is this a worthwhile optimization?

        bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
        {
            if (other == null || !(other is ValueTuple<T1, T2, T3>)) return false;

            var objTuple = (ValueTuple<T1, T2, T3>)other;

            if (comparer == null)
            {
                return object.Equals(Item1, objTuple.Item1)
                    && object.Equals(Item2, objTuple.Item2)
                    && object.Equals(Item3, objTuple.Item3);
            }
            else
            {
                return comparer.Equals(Item1, objTuple.Item1)
                    && comparer.Equals(Item2, objTuple.Item2)
                    && comparer.Equals(Item3, objTuple.Item3);
            }
        }

Or, alternatively, should EqualityComparer<object>.Default be cached as part of dotnet/corefx#18666?

[jcouv]

Other code in coreFX seems to use EqualityComparer<object>.Default. I'd stick with that, absent benchmarking information.

        bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
        {
            if (other == null || !(other is ValueTuple<T1, T2, T3>)) return false;
            var objTuple = (ValueTuple<T1, T2, T3>)other;
            comparer = comparer ?? EqualityComparer<object>.Default;
           ...

Whether or not to cache, I'd also lean towards not caching absent benchmarking.

[jnm2]

comparer = comparer ?? EqualityComparer<object>.Default;

That is not preferential to if (comparer == null) comparer = EqualityComparer<object>.Default;, is it?

[jcouv]

@jnm2
Good point. The if statement would be better (doesn't assign unless needed). Do put curly braces around the body though.

[jnm2]

Do put curly braces around the body though.

Sure, if that's what you want:

            if (comparer == null)
            {
                comparer = EqualityComparer<object>.Default;
            }
            return comparer.Equals(Item1, objTuple.Item1)
                && comparer.Equals(Item2, objTuple.Item2)
                && comparer.Equals(Item3, objTuple.Item3);

However ValueTuple is full of this convention:

        int IStructuralComparable.CompareTo(object other, IComparer comparer)
        {
            if (other == null) return 1;

            if (!(other is ValueTuple<T1, T2, T3>))
            {
                throw new ArgumentException(SR.Format(SR.ArgumentException_ValueTupleIncorrectType, this.GetType().ToString()), nameof(other));
            }

            var objTuple = (ValueTuple<T1, T2, T3>)other;

            int c = comparer.Compare(Item1, objTuple.Item1);
            if (c != 0) return c;

            c = comparer.Compare(Item2, objTuple.Item2);
            if (c != 0) return c;

            return comparer.Compare(Item3, objTuple.Item3);
        }

[jcouv]

Ok, never mind for braces then :-)

[jnm2]

Sure thing. I don't care, just checking.

[jnm2]

@jcouv Array.BinarySearch uses Comparer.Default if you pass null, whereas Tuple.IComparable.CompareTo(object) uses Comparer<object>.Default.

When adding the null checks, I was going to go with Comparer.Default, but now I'm not sure which it should be. Or should it be different for Array than for the tuple types?

[jnm2]

The coreclr PR is up dotnet/coreclr#11345

[jcouv]

@jnm2 I'm not sure either (between Comparer.Default or Comparer<object>.Default. I'll take a deeper look tonight. The PR reviewers may also be able to chime in.

Just to keep you updated, the compat council is still discussing the question. I'll let you know once settles (hopefully with an approval).

[jcouv]

@jnm2 The compat discussion is still not settled, but it seems that we shouldn't update the implementation in coreFX. Otherwise, we'd be introducing a significant break from people moving from 4.6.x plus package (no NRE) to 4.7 (throws NRE).

[jnm2]

@jcouv

Otherwise, we'd be introducing a significant break from people moving from 4.6.x plus package (no NRE) to 4.7 (throws NRE).

This was done with https://github.com/dotnet/corefx/issues/18432. I believe that this is par for the course and one of the reasons that https://github.com/terrajobst/platform-compat is being developed.

[jnm2]

The conclusion is that the benefits do not outweigh the costs. See dotnet/coreclr#11345 (comment)

Thanks for the discussion and I look forward to the next one! =)