19

The documentation states that bool Dictionary<TKey, TValue>.ContainsKey(TKey key) throws an exception in case a null key is passed. Could anyone give a reason for that? Wouldn't it be more practical if it just returned false?

Marcin Kaczmarek
  • 1,063
  • 2
  • 9
  • 20
  • 3
    A null key is no value at all. It can never be used as a valid key so it can never be considered a valid input. – Panagiotis Kanavos Dec 06 '13 at 15:47
  • 3
    What is the value of `null.GetHashCode()`? – plinth Dec 06 '13 at 15:47
  • 1
    @plinth it is whatever the `IEqualityComparer` says it is =p. It's actually entirely viable to choose to write a dictionary that accepts null keys, the library just didn't choose to do that. – Servy Dec 06 '13 at 15:48
  • @plinth Sure, then just instead `if (key == null) throw new ArgumentNullExceltion;` do `if (key == null) return false;` – Marcin Kaczmarek Dec 06 '13 at 15:50
  • @mkacz You should either support null keys or not support them. Returning false really does give a misleading impression (as stuartd answered). A better question would be, why not support null keys at all. – GregRos Dec 06 '13 at 15:51
  • Why are you asking this question? Is it just theoretical? What are you trying to do? – Aaron Palmer Dec 06 '13 at 15:52
  • 1
    @Servy There's a good reason for that, though. Dictionary accepts both reference types and value types as keys, so it can't just write `if (key == null) ...`. You can do the comparison, but it doesn't make any sense for value types. – Luaan Dec 06 '13 at 15:52
  • 1
    @Luaan The dictionary has to go out of it's way to throw exceptions in the case where the key is null. It would be *easier* to support null keys; just pass all keys to the `IEqualityComparer` and let it either return a value or throw an exception. My guess is they needed to cast the key to `object` and then compare it to null in order to be able to add such a check, given that, as you say, the key can be a value type. – Servy Dec 06 '13 at 15:56
  • I think not supporting `null` keys makes sense if your design philosophy is that `null` represents a kind of invalid value (like, well, a null pointer), rather than a valid bottom type. I guess that's the general philosophy of the .NET framework. – GregRos Dec 06 '13 at 16:02
  • @Servy Interestingly, using ILSpy seems to indicate that the code always does boxing of the key. Apparently, it *is* legal to compare a non-constrained generic type value to null. Strange. – Luaan Dec 06 '13 at 16:06
  • 1
    @Luaan: Every generic type `T` must either be a reference type, in which case it may be directly compared to `null`, a nullable type, in which case comparison to `null` can be "compared" to `null` using a `HasValue` check, or a structure type in which case a widening conversion will exist to `T?`, which may then be "compared" to `null`. To actually box the key if it's a non-nullable value type would be a little silly; it would seem either the JIT should optimize that away or a `NullChecker.IsNull` method should be able to expedite the test. – supercat Dec 06 '13 at 16:12
  • @supercat: Yeah, it's probably the nullable lifting that allows this when the type is known at compile-time (ie. `int test = 31; if (test == null) ...`). However, the IL code still plainly states that there's a `box` instruction. The JIT will quite certainly optimize it away for reference types, but it's there in the IL. The IL code for the generic version is different than the one you get when the type is known (and no, it doesn't *explicitly* mention the Nullable at all, but that's due to the various Nullable optimization in the C# compiler). – Luaan Dec 06 '13 at 16:19
  • @Luaan: It would probably not be difficult for the JIT to recognize 99% of the cases where the result of a "box" is compared with null and abandoned before anything else is done with it. If it does so, then the comparison would look ugly in CIL but be comparatively harmless at run-time. If the thing actually did get boxed, then `IEquatable` wouldn't be necessary (given generic value-type variables `t1` and `t2` and object `o1`, if one has performed o1 = t1;`, then `t2.Equals(o1)` won't be much slower than using the `IEquatable` override as `t2.Equals(t1). – supercat Dec 06 '13 at 16:27

2 Answers2

22

If ContainsKey(null) returned false it would give the misleading impression that null keys are allowed..

stuartd
  • 70,509
  • 14
  • 132
  • 163
  • 1
    And it probably also helps to locate your mistake sooner - since there's no way you should use a null reference in ContainsKey, you know there's a problem, rather than just getting a "nope, it's not here, keep trying". Yeah, I think I like it this way better :) – Luaan Dec 06 '13 at 16:08
7

This is how it is implemented: (Source)

public bool ContainsKey(TKey key) {
    return FindEntry(key) >= 0;
}

And the method FindEntry as:

private int FindEntry(TKey key) {
    if( key == null) {
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
    }

    if (buckets != null) {
        int hashCode = comparer.GetHashCode(key) & 0x7FFFFFFF;
        for (int i = buckets[hashCode % buckets.Length]; i >= 0; i = entries[i].next) {
            if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key)) return i;
        }
    }
    return -1;
}

Since having a null value as key in dictionary is not allowed. 

Dictionary<string, int> dictionary = new Dictionary<string, int>();
dictionary.Add(null, 10);

The above would produce an exception:

Value cannot be null. Parameter name: key

For your question:

Wouldn't it be more practical if it just returned false?

Someone from Microsoft could probably reply that. But IMO, since adding a null value for key is not allowed there is no point in checking for null key in ContainsKey

Habib
  • 219,104
  • 29
  • 407
  • 436
  • I expect the sole reason is probably that while `IEquatable.GetHashCode(T)` could have been specified to return a constant when given a null value, it wasn't. If a `Dictionary`'s call to `IEquatable` is going to throw an exception when given a null argument, it would be better to have the exception thrown from `Dictionary`, giving the name of its parameter, than from `IEquatable.GetHashCode()`. – supercat Dec 30 '13 at 18:32