Remove [DisallowNull] from EqualityComparer.GetHashCode

[jnm2]

Visual Studio 16.3.1 generates code like this for GetHashCode specifically because this way the null check is handled. EqualityComparer.GetHashCode is well-known to return 0 if you pass null:

struct Foo
{
    public string? Bar { get; }

    public override int GetHashCode()
    {
        // CS8604 Possible null reference argument for parameter 'obj' in
        // 'int EqualityComparer<string?>.GetHashCode(string? obj)'.      ↓
        return 1739646154 + EqualityComparer<string?>.Default.GetHashCode(Bar);
    }
}

Rather than generating a ! to go along with it, could [DisallowNull] be removed from EqualityComparer<>.GetHashCode as a kind of contravariance, even though IEqualityComparer<>.GetHashCode has it? It would reflect the true behavior of the EqualityComparer<> class.

[jnm2]

I missed that it's only EqualityComparer<>.Default that has this property of not throwing when getting a hash code for null and that EqualityComparer.GetHashCode is documented to throw for null in general. Thanks @sharwell!

[jnm2]

Reopening since @CyrusNajmabadi believes that the virtual EqualityComparer<>.GetHashCode class method should allow null and the docs should change, and I'm quite partial to this idea. See dotnet/roslyn#37913 (comment).

[CyrusNajmabadi]

Looking into it, it looks like this is wrongly annotated in the BCL. This instance very specifically and intentionally does not throw. This is baked through so thoroughly that the C# compiler takes a hard dependency on the existence of this behavior when generating teh code for things like anonymous-types. i.e. it will make direct calls (without null checks) into EqualityComparer<whatever>.Default.Equals/GetHashCode precisely to be able to avoid synthesizing null checks and to benefit from the appropriate null-behavior this impl has.

It seems wrong to have this extraordinarily common value be null-tolerant, but then mark the api entrypoint into it as being null-intolerant. This forces code to have to suppress or null-check to avoid C#'s warnings, even though it's expected idiomatic behavior around using this API.

[sharwell]

I was under the impression that the type would be this:

public interface IEqualityComparer<in T>
{
  bool Equals(T x, T y);
  int GetHashCode(T obj);
}

With EqualityComparer<T> matching that. Roslyn assumes this is the behavior in many places, but we don't have any build-time validation because there are no annotations in .NET Standard. It's leading to increasing divergence between how we are using the framework, and how the framework is being defined.

[scalablecory]

Both EqualityComparer<> and IEqualityComparer<> docs state that they throw on null. It'd be a breaking change to remove that expectation, so I don't think this is a good change.

That C# and maybe parts of BCL rely on an implementation detail for EqualityComparer.Default should not dictate the annotation.

[CyrusNajmabadi]

It'd be a breaking change to remove that expectation

Wait... how? It's not a breaking change ot state that something that threw before won't throw anymore.

[sharwell]

@scalablecory I'm having a hard time finding implementations of this interface which throw on GetHashCode but do not throw on Equals.

[CyrusNajmabadi]

That C# and maybe parts of BCL rely on an implementation detail for EqualityComparer.Default should not dictate the annotation.

Yes it should. This feature is about ensuring that appropriate coding patterns aren't being unnecessarily penalized by incorrect annotations. Annotations should reflect the actual state of the world, so that code that is written properly isn't told it is incorrect and forced to change.

--

Note: we've had to do this in roslyn itself in a few places. There are areas where we'd like to annotate a certain way, but we found out that annotation isn't correct.

[jnm2]

I was under the impression that the type would be this:

Me too. Both the [AllowNull] and [DisallowNull] overrides surprised me in the current definition:

https://github.com/dotnet/corefx/blob/e667c29636a622eb4f9493f75232b44e0ae90b29/src/Common/src/CoreLib/System/Collections/Generic/IEqualityComparer.cs#L12-L16

[scalablecory]

Annotations should reflect the actual state of the world, so that code that is written properly isn't told it is incorrect and forced to change.

You don't know the actual state of the world here because EqualityComparer<>.GetHashCode() is virtual. You're changing the contract for anyone who has implemented the class.

[CyrusNajmabadi]

You don't know the actual state of the world here because EqualityComparer<>.GetHashCode() is virtual.

We can err on the side of looking at our own most common impl that is used and depended on heavily.

Since this is a relaxing and not a restricting choice, i see no problem there.

--

I would say that those that want to restrict against the most common known form here should provide the argument why that would be desirable given how it hurts those upgrading to NRT whose code is totally fine.

You're changing the contract for anyone who has implemented the class.

No. I'm stating that EqualityComparer should say nothing at the NRT level about this one way or the other. If a user says EqualityComparer<string?> they should be allowed to pass in string?s. If they say EqualityComparer<string> then they can't pass in a string?. This accurately conveys how this API actually works and leaves it in control of the consumer of the API to specify the behavior they expect this to have.

[CyrusNajmabadi]

This bit is especially relevant:

With EqualityComparer<T> matching that. Roslyn assumes this is the behavior in many places, but we don't have any build-time validation because there are no annotations in .NET Standard. It's leading to increasing divergence between how we are using the framework, and how the framework is being defined.

This case is also a bit strange for me because i have no idea how these annoations could have been added in Core without causing Core itself to break (i.e. we must at least have tests that validate this behavior). How did they not trigger any issues here?

[jnm2]

@CyrusNajmabadi

Since this is a relaxing and not a restricting choice, i see no problem there.

It's of course a new demand on implementers of the class to not throw if someone passes null.

@scalablecory Under what circumstances would someone implement the EqualityComparer class? It seems like implementers would be very rare, and consumers of anything but EqualityComparer<>.Default (or direct IEqualityComparer interface implementations) just as rare.

Then, what are the consequences for the class implementors if [DisallowNull] is removed?