When Implementing IEqualityComparer Should GetHashCode check for null?

Question

When implementing IEqualityComparer<Product> (Product is a class), ReSharper complains that the null check below is always false:

public int GetHashCode(Product product)
{
  // Check whether the object is null. 
  if (Object.ReferenceEquals(product, null))
    return 0;

  // ... other stuff ...
}

(Code example from MSDN VS.9 documentation of Enumerable.Except)

ReSharper may be wrong, but when searching for an answer, I came across the official documentation for IEqualityComparer<T> which has an example where null is not checked for:

public int GetHashCode(Box bx)
{
    int hCode = bx.Height ^ bx.Length ^ bx.Width;
    return hCode.GetHashCode();
}

Additionally, the documentation for GetHashCode() states that ArgumentNullException will be thrown when "The type of obj is a reference type and obj is null."

So, when implementing IEqualityComparer should GetHashCode check for null, and if so, what should it do with null (throw an exception or return a value)?

I'm interested most in .NET framework official documentation that specifies one way or another if null should be checked.

I opened a bug on this with JetBrains, see the discussion there: https://youtrack.jetbrains.com/issue/RSRP-468630. — Ohad Schneider, Mar 12 '18 at 23:17
I also opened the following related issue on the .NET CoreFX repo (due to inconsistent implementation of this contract in the framework itself): https://github.com/dotnet/corefx/issues/28170. — Ohad Schneider, Mar 17 '18 at 14:26

score 10 · Accepted Answer · answered Jun 27 '14 at 20:34

10

ReSharper is wrong.

Obviously code you write can call that particular GetHashCode method and pass in a null value. All known methods might ensure this will never happen, but obviously ReSharper can only take existing code (patterns) into account.

So in this case, check for null and do the "right thing".

Corollary: If the method in question was private, then ReSharper might analyze (though I'm not sure it does) the public code and verify that there is indeed no way that this particular private method will be called with a null reference, but since it is a public method, and one available through an interface, then

ReSharper is wrong.

answered Jun 27 '14 at 20:34

Lasse V. Karlsen

380,855
102
628
825

Of course, the "right thing" is [up for debate](http://stackoverflow.com/questions/10723458/should-the-hash-code-of-null-always-be-zero-in-net?rq=1) when you consider that the documentation states that an `ArgumentNullExpection` should be thrown but .NET built-in classes do not actually throw on null. – zastrowm Jun 28 '14 at 03:29
@FriendlyGuy I don't think it's up for debate. The docs are clear. Any BCL classes that don't follow it are wrong, just like Resharper is. – Ohad Schneider Mar 12 '18 at 23:16

Servy · Answer 2 · 2014-06-27T20:32:13.983

4

The documentation says that null values should never be hashable, and that attempting to do so should always result in an exception.

Of course, you're free to do whatever you want. If you want to create a hash based structure for which null keys are valid, you're free to do so, in this case you should simply ignore this warning.

edited Jun 27 '14 at 20:32

answered Jun 27 '14 at 20:25

Servy

202,030
26
332
449

1

@digEmAll Because it believes the documentation. – Servy Jun 27 '14 at 20:31
https://msdn.microsoft.com/en-us/library/system.object.gethashcode(v=vs.110).aspx says overrides "should not throw exceptions". Can you link to the contradictory documentation that says it should throw exceptions on null? – Denise Skidmore Mar 12 '18 at 16:43
1

@DeniseSkidmore If you try to call `GetHashCode` on `null` you'll get a null reference exception. What the `override` does is irrelevant. You can't *get* to the override, because there is no object to dispatch on. This question is asking about `IEqualityComparer.GetHashCode`, because a comparer can actually compute a hash for a `null` value. The object's instance method *can't* (not shouldn't, but *can't*) compute a hash for `null`, no matter what you implemented it to do. – Servy Mar 12 '18 at 17:40
Ah right, they are two separate GetHashCode functions, one is not an overload of the other. – Denise Skidmore Mar 13 '18 at 16:25
But you should ignore the ReSharper warning, because the documentation that suggests throwing an exception says to throw `ArgumentNullException`, not `NullReferenceException`. You can't just rely on the system generated NullReferenceException. – Denise Skidmore Mar 13 '18 at 16:45
@DeniseSkidmore And the resharper warning *say* that an `ArgumentNullException` is what's expected, not a NRE, so again, it's in line with the documentation. – Servy Mar 13 '18 at 17:12
My version of ReSharper (and the ops) complains that obj can never be null if you do the check. – Denise Skidmore Mar 13 '18 at 18:09
@DeniseSkidmore Oh, you're talking about the first snippet. In that case, as mentioned in the answer, Resharper is expecting you to throw if the value is `null`, because that's what the documentation says it's supposed to do, and it's basing its warnings on that assumption. If you violate that assumption, the warnings will be wrong. – Servy Mar 13 '18 at 18:15

score 1 · Answer 3 · answered Mar 13 '18 at 18:49

ReSharper has some special case code here. It will not warn about the ReferenceEquals in this:

if (ReferenceEquals(obj, null)) { throw new ArgumentNullException("obj"); }

It will warn about the ReferenceEquals in this:

if (ReferenceEquals(obj, null)) { return 0; }

Throwing an ArgumentNullException exception is consistent with the contract specified in IEqualityComparer(Of T).GetHashCode

If you go to the definition of IEqualityComparer (F12) you'll also find further documentation:

    // Exceptions:
    //   System.ArgumentNullException:
    //     The type of obj is a reference type and obj is null.
    int GetHashCode(T obj);

So ReSharper is right that there is something wrong, but the error displayed doesn't match the change you should make to the code.

score 1 · Answer 4 · answered Apr 03 '20 at 20:31

There is some nuance to this question.

The docs state that IEqualityComparer<T>.GetHashCode(T) throws on null input; however EqualityComparer<>.Default - which is almost certainly by far the most used implementation - does not throw.

Clearly, an implementation does not need to throw on null it merely has the option too.

However, I'd argue that no implementation should ever throw on null here, it's just confusing, and a possible source of bugs. Exceptions are a pain in any case, being a non-local control flow mechanism, and that alone argues for using them when necessary only (i.e.: not here). But additionally, for IEqualityComparer specifically, the docs state that whenever Equals(x, y) then GetHashCode(x) should equal GetHashCode(y) - and Equals does allow nulls, and is not documented as throwing any exceptions.

The invariant that equality implies hashcode equality makes implementing things relying on those hashcodes much simpler. Having a gotcha with the null value is a design cost you should avoid paying without need. And here there is no need, ever.

In short:

do not throw from GetHashCode, even though it is allowed
and do check for nulls; Resharper's warning is incorrect.

Doing this results in simpler code with fewer gotchas, and it follows the behavior of EqualityComparer<>.Default which is the most common implementation used.

When Implementing IEqualityComparer Should GetHashCode check for null?

4 Answers4