Runtime: Remove [DisallowNull] from IEqualityComparer<T>.GetHashCode by interpreting existing contracts as applying to nonnullable T only

[jnm2]

In the previous discussion about removing [DisallowNull] for the EqualityComparer class, the blocker was that the existing implementations of GetHashCode sometimes threw for null, and the contract was explicitly documented to allow any implementation to throw for null. Therefore the nullability warning would be appropriate.

An argument was made that it would be better to not cause a nullability warning even in cases when the implementation threw for null. This is not the argument I'm making.

My proposal is that any implementation which throws for null must implement IEqualityComparer<T> with a non-nullable T. All documentation of contracts, expectations, and implementations existing prior to NRTs is interpreted to apply only to instantiations of IEqualityComparer<T> with non-nullable T.

However, if an implementation chooses to implement IEqualityComparer<T> with nullable T, that implementation must not throw for null in GetHashCode. Implementing with a nullable T is interpreted as a new category that did not exist prior to the introduction of NRTs.

Proposed change:

namespace System.Collections.Generic
{
    public interface IEqualityComparer<in T>
    {
        bool Equals([AllowNull] T x, [AllowNull] T y);
-       int GetHashCode([DisallowNull] T obj);
+       int GetHashCode(T obj);
    }

    public abstract partial class EqualityComparer<T> : IEqualityComparer, IEqualityComparer<T>
    {
        public abstract bool Equals([AllowNull] T x, [AllowNull] T y);
-       public abstract int GetHashCode([DisallowNull] T obj);
+       public abstract int GetHashCode(T obj);

}

/cc @stephentoub, @safern, @sharwell, @CyrusNajmabadi, @jasonmalinowski, @333fred, @jcouv from previous discussions https://github.com/dotnet/runtime/issues/30998 and https://github.com/dotnet/roslyn/issues/37913.

[StephenCleary]

I've been updating my Comparers library with nullable types, and also found the current attributes unexpected, surprising, and confusing.

I've encountered a few interesting corner cases when writing my Comparers library. Side note: this gets even more fun when looking at the non-generic interfaces.

The core of the problem is that the documentation itself is inconsistent, and always has been. The same page that says GetHashCode can throw ArgumentNullException also says "Implementations are required to ensure that if the Equals(T, T) method returns true for two objects x and y, then the value returned by the GetHashCode(T) method for x must equal the value returned for y." - in other words, Equals(x, y) == true <implies> GetHashCode(x) == GetHashCode(y). But Equals allows null.

StringComparer is the most commonly-used type that throws for GetHashCode(null), and it doesn't satisfy the docs that say Equals(x, y) == true <implies> GetHashCode(x) == GetHashCode(y):

```C#
// The string comparer allows equating null to null
Assert.True(StringComparer.Ordinal.Equals(null, null));

// But throws if null is passed to GetHashCode
// https://github.com/dotnet/corefx/blob/36e812b0e974fa113c50be71e1701735a8a63481/src/Common/src/CoreLib/System/StringComparer.cs#L150-L153
Assert.ThrowsAny(() => StringComparer.Ordinal.GetHashCode(null));

// The framework default comparer does not throw
Assert.True(EqualityComparer.Default.Equals(null, null));
EqualityComparer.Default.GetHashCode(null);
```

Thus, StringComparer does not satisfy both the documentation requirements (and never has).

Users of these interfaces must choose one or the other of the conflicting requirements: either Equals(x, y) == true <implies> GetHashCode(x) == GetHashCode(y) is true for the entire set of values (including null) and GetHashCode implementations should accept null, or Equals(x, y) == true <implies> GetHashCode(x) == GetHashCode(y) is true for only the set of non-null values and GetHashCode may throw for null.

It looks like the .NET team has adopted the second resolution (and should update the docs accordingly). My comparers library adopted the first resolution, assuming that Equals(x, y) == true <implies> GetHashCode(x) == GetHashCode(y) should also hold for null values (and thus it is acceptable to call GetHashCode(null)). So, that's a bummer for me.

Moving forward, I think my Comparers library will continue to keep the IMO more consistent interpretation that GetHashCode(null) is valid. My Comparers library does a lot of LINQ-like comparer combination and manipulation, and it's easier to deal with and reason about if null is consistently in the set of valid argument values.

[stephentoub]

Implementations are required to ensure that if the Equals(T, T) method returns true for two objects x and y, then the value returned by the GetHashCode(T) method for x must equal the value returned for y." - in other words, Equals(x, y) == true GetHashCode(x) == GetHashCode(y). But Equals allows null.

Thanks. Since we're nit-picking the language used in the docs (and I'm sure they could be improved), FWIW I think there are two potentially incorrect assumptions stated by this "in other words":

1. That null _is_ an object.
2. That "the value returned" applies even when a value isn't returned.

Remove those assumptions, and the docs are still correct, at least as far as I read what you quoted, and the .NET implementations adhere to them.

[sharwell]

@stephentoub I agree that those assumptions are a reasonable interpretation provided the goal is to be logically-consistent ("technically correct"). However, I agree that the more important concern here is client usability, and on that front the interpretation by @StephenCleary in line with the original proposal above is an overwhelming improvement.

There is _no way_ to define nullability for this API that avoids the needs to update either implementation code (types that implement IEqualityComparer<T>) or client code (types that use IEqualityComparer<T>). Currently the definition places the burden of change on client code, which is much more intrusive than a definition that places the burden on implementations. The proposal from @jnm2 shifts that burden to the implementation, which is a less intrusive overall change for the majority of implementing code.

[stephentoub]

The proposal from @jnm2 shifts that burden to the implementation

How so? In this proposal, a type like StringComparer would implement IEqualityComparer<string> rather than IEqualityComparer<string?>, as it abides by the long-standing contract and ends up throwing for nulls passed to GetHashCode. Now, as the client, I want to have a HashSet<string?>, and I want to use StringComparer.Ordinal with it. HashSet will not pass nulls into GetHashCode, yet I'm going to get a warning when I try to write new HashSet<string?>(StringComparer.Ordinal)... how is that not a burden on the client?

[sharwell]

@stephentoub The impact of that case depends on the outcome of #25584. If both proposals were implemented at the same time, it would not have any impact on the scenario you describe, and the code changes would have occurred in the implementation of StringComparer.

[stephentoub]

StringComparer is just one example. There are more in the .NET core libraries that we do control, and more outside of the .NET core libraries that we don't.

It also does have an impact: to play nicely with this set of annotations, every GetHashCode implementation needs to end up supporting null, even as consumers need to assume they don't, meaning we now have double the null checks, on top of effectively trying to retcon the behavior.

Also, StringComparer isn't sealed; changing its behavior and then codifying that in its contract in turn impacts types derived from it. Just looking at one code base I have access to, I see more than 20 types derived from it, many of which have the same null-isn't-allowed GetHashCode behavior.

[jnm2]

Ah, I see. The HashSet<T> constructor can't specify that it takes IEqualityComparer<T> rather than IEqualityComparer<T?>. C# doesn't allow this distinction without a reference constraint on T.

Having to suppress warnings on each usage of StringComparer.OrdinalIgnoreCase or third-party implementation that is passed to a generic API would be just as distasteful as having to suppress warnings when using EqualityComparer<T>.Default.GetHashCode and would probably be a lot more frequent.

Without a new language feature, there's no way around making IEqualityComparer<X> and IEqualityComparer<X?> mean the same thing, is there?