Roslyn: Generated GetHashCode implementation falls foul of BCL nullability of IEqualityComparer<>.GetHashCode

[jnm2]

Version Used: Visual Studio 16.2.1, <LangVersion>preview</LangVersion>, <Nullable>enable</Nullable>
Generate equality members for:

struct Foo
{
    public string? Bar { get; }
}

Result:

    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);
    }

This can be fixed by hand to get rid of the warning by changing EqualityComparer<string> to EqualityComparer<string?>:

    public override int GetHashCode()
    {
        return 1739646154 + EqualityComparer<string?>.Default.GetHashCode(Bar);
    }

[CyrusNajmabadi]

I think it's a feature bug. we should be generating EqualityComparer<string?>.Default.GetHashCode. @jasonmalinowski ?

[jasonmalinowski]

I have no idea. @333fred, @jcouv or @gafter what's the intent here? I was similarly confused here:

https://github.com/dotnet/roslyn/pull/37881/files#diff-48ce97c41d3c9a5bf28ef1718c0ade67R76

Since equality comparers historically have accepted null no matter what...?

[CyrusNajmabadi]

Is IEqualityComparer (or EqualityComparer) not properly annotated? It should probably have [MaybeNull] parameters right?

[jasonmalinowski]

That's what I'd expect. Otherwise this means any existing generated code would have to be updated.

[jasonmalinowski]

So looking at this further, IEqualityComparer<T> is annotated in current framework:

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

So this asks the question: what do we do for the users trying to use nullability in .NET framework targeting code? It's not a fully supported experience, but what do we want to do? We can have it add suppressions or surround with #nullable disable if you're targeting a framework that isn't annotated, but that's then leaving code in a ugly state that can be removed if you target a newer framework.

[CyrusNajmabadi]

@jasonmalinowski for this particular issue, wouldn't this be solved if we emit as EqualityComparer<string?>.Default.GetHashCode(Bar);?

[jasonmalinowski]

Yes, which at least in some sense is "writing ugly code that you wouldn't write if targeted a newer framework". I'd say that's the path of least pain, but still not fun. 😢

[jasonmalinowski]

The one thing I can think of why we might want to emit GetDefaultHashCode(Bar!) is we will eventually have a "remove unneeded !" refactoring, so that'd be easier to fix up than trying to infer removing the ? at the EqualityComparer<string?> is what the "right" fix is once you've upgraded frameworks.

[jasonmalinowski]

(that's not a strong argument in my opinion but at least tips scales a bit)

[CyrusNajmabadi]

Note: i don't see anything wrong with EqualityComparer<string?>...

That's waht we would have gotten if we just had real nullability on ITypeSymbol. it's also what we would do for something like a nullable custom struct. I think that would be fine here.

[333fred]

EqualityComparer<string?> should be fine here, I would think.

[jasonmalinowski]

Oh I might have misread the scenario. My bad. Boo me.

[jnm2]

Could you reopen, please? IEqualityComparer.GetHashCode has [DisallowNull] which means that the code currently generated by 16.3.1 produces a warning:

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);
    }
}

.NET Core 3.0 definition:

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

https://github.com/dotnet/corefx/blob/v3.0.0/src/Common/src/CoreLib/System/Collections/Generic/IEqualityComparer.cs

Would the best way to handle this be to special-case knowledge of the EqualityComparer class and emit an extra !?

I also filed https://github.com/dotnet/corefx/issues/41405 to ask if [DisallowNull] can be removed from EqualityComparer<>.GetHashCode (suppressing the discrepancy with the IEqualityComparer<>.GetHashCode signature) in future versions of the BCL.

[sharwell]

@jnm2 EqualityComparer<T>.GetHashCode is public, virtual, and documented to throw on null:
https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.equalitycomparer-1.gethashcode?view=netframework-4.8#System_Collections_Generic_EqualityComparer_1_GetHashCode__0_

We would have to special case the behavior here for EqualityComparer<T>.Default.

[CyrusNajmabadi]

We would have to special case the behavior here for EqualityComparer.Default.

We don't need to do that. Rather. EqualityComparer<T> needs to be fixed up. It's GetHashCode does not throw (which is why BCL recommended we used this pattern when generating default hashcodes for people.

[CyrusNajmabadi]

Note: thsi is also critical for Roslyn since htis is how we emit .GetHashCode for an anonymous type. We just use EqualityComparer<whatever-ref-type>.Default.Equals knowing that it will properly handle null checks.

So something has gone off the rails here in terms of how this type is actually annoted in the BCL.

[jnm2]

@CyrusNajmabadi The virtual EqualityComparer<T>.GetHashCode is documented to throw as Sam points out, but the instance you're referring to is the one from EqualityComparer<T>.Default which never throws.

[CyrusNajmabadi]

Yes. I get that. I'm pointing out the documentation must be wrong here (and also cannot change as it would break potentially everyone) :)

[CyrusNajmabadi]

EqualityComparer<T>.Default returns an EqualityComparer<T> so it is wrong for the documentation to say that impls must throw.

[jnm2]

This is the extent of the relevant documentation I can see from Sam's link:

Exceptions
ArgumentNullException
The type of obj is a reference type and obj is null.

I understand this to be how the docs usually say, "you might get such and such an exception. If you do, this is why."

[CyrusNajmabadi]

Ok. So if you might get an exceptoin, but you won't always, then this function should be allowed to take in null. it shoudln't say "do not pass in null here" since that's super common and expected and doesn't have any problem with likelythe most commonly used EqualityComparer out there (i.e. .Default).

[sharwell]

@CyrusNajmabadi Derived types are allowed to expand on the set of supported inputs, but not restrict it. If the base implementation says null is allowed, then derived implementations can't implement that method while disallowing null (i.e. allowing null is a widening operation).

[CyrusNajmabadi]

(i.e. allowing null is a widening operation).

If that's the case, and the most common use case is one that accepts null, then the base type should not restrict null as you end up in just this scenario where the user is told there's a problem, despite there being no problem.

[CyrusNajmabadi]

Again, who is helped by this type being annotated like this? Literally all code out there that specifically uses this exact API for this exact purpose (which is something we've communicated) are now told there's a problem that must be dealt with even though this type is idiomatically the way to easily do equality/gethashcode for values that may be null.

[CyrusNajmabadi]

Note: this is not in reference to IEqualityComparer. This is specifically about EqualityComparer. Expectation there (as born out from everyone using the static field it exposes) is that it can handle null.

[jnm2]

@CyrusNajmabadi I can reopen https://github.com/dotnet/corefx/issues/41405 if you want to make the case there. I'm certainly not going to complain if this changes.

[CyrusNajmabadi]

Yeah, i would reopen that. It def seems wrong IMO.

[jasonmalinowski]

@jnm2 @CyrusNajmabadi So how do we feel about the overall status of this bug? We'll now use System.HashCode directly which removes the problematic code. Do we feel comfortable calling that the 'real' fix here?

[jnm2]

@jasonmalinowski

That won't fix it for me. I'm targeting .NET Standard and .NET Framework (where HashCode is not available) and using @sharwell's excellent https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator to borrow .NET Core's annotations into the matching APIs in the .NET Standard and .NET Framework reference assemblies.

So effectively for this question it is the same as if I was using a .NET Core 3.0 that didn't have HashCode.

[sharwell]

I'm not sure there's much else we can do from the Roslyn side. It appears unlikely that the reference assemblies will change in order to allow users to make these safe calls without warnings. Instead of forcing a smaller number of users who implement IEqualityComparer<T> to update their GetHashCode implementations, we are forcing larger numbers of users who consume IEqualityComparer<T> to update their calls. There is no painless solution to the situation, but among the available options we find ourselves stuck with the one that causes trouble for more users.

[jasonmalinowski]

So the original discussion of just having this consume EqualityComparer<string?> instead is still a perfectly viable approach, and would be a trivial change to make. We last went down that path which ended in https://github.com/dotnet/corefx/issues/41405 but if that's not happening then should we just do that?

[jasonmalinowski]

This does bring up an interesting problem with @sharwell's tool though: if we write any code under the assumption that _annotated_ APIs means you're on a newer framework that has certain additional APIs to address other concerns (like I did in my comment), that our solutions won't work. It puts you in a fairly "interesting" framework surface area. It's not _bad_ given it is doing what sometimes needs to be done, it's just a strange case for us doing testing.

[CyrusNajmabadi]

I thought we could still generate a hashcode (without System.HashCode) that would at least not fall afoul of the nullable rules. Shouldn't we still do that as well?

[jnm2]

@sharwell

I'm not sure there's much else we can do from the Roslyn side.

Is a warning suppression still on the table?

[jasonmalinowski]

So for somebody who is using nullable down-level without @sharwell's tool, then EqualityComparer<string?> would work because the APIs are otherwise unannotated so the T will be string?. If the API _is_ still annotated then I think suppression is the only way to go, but it seems like we're doing workarounds around workarounds around workarounds at this point and I'm not sure how much we want to support this scenario.

[jnm2]

Fair. Is it possible for suppression to be implemented by authoring an analyzer?

[jasonmalinowski]

Oops @jnm2 sorry I missed the question. Generally we have a concept of analyzers emitting suppression. @mavasani do we have documentation on analyzers emitting suppressions?