Nullability headaches with IComparer and IEqualityComparer

[jhudsoncedaron]

We currently have:

public interface IComparer<T> {
    int Compare(T? x, T? y);
}

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

The nullable annotations are messed up in the stock methods.

Demonstration:

   List<string> GetOrderedListOfItems(HashSet<string> items)
       => items.OrderBy(new ItemComparer()).ToList());

   class ItemComparer : IComparer<string>() {
       int IComparer<string>.Compare(string x, string y) { // Declaration has useless compiler warning. x and y are actually staticly typed to be not possibly null.
          // implement
       }
   }

IEqualityComparer has the opposite problem as well:

   // DictionaryFactory returns an IDictionary that behaves like Dictionary but with the null key check knocked out.
   IDictionary<string?, string?> container = DictonaryFactory<string?, string?>(new ItemEqualityComparer());
   container.Add(null, null); // Actually works if the IEqualityComparer handles null GetHashCode(!)

   class ItemComparer : IEqualityComparer<string?>() {
       int IEqualityComparer<string?>.GetHashCode(string? item) { // Useless compiler warning
          // implement
       }

       bool IEqualityComparer<string?>.Equals(string? x, string? y) {
          // implement
       }
   }

Both of these came up in real code multiple times. We've been slow to adapt nullability checks due to false alarms and false IDE automated fixes to them (but the latter is another story).

In general, these two are the same problem. I had both of these come up in live code with these exact use cases.

The problem is the nullability of the arguments doesn't correspond to the nullability of the interface. If the enumerable cannot contain a null; its OrderBy() comparer need not support nulls; and the inverse: if the dictionary key is allowed to be null then IEqualityComparer.GetHashCode() can receive a null. There's no way to shut up the compiler other than manually suppressing the warnings because the warnings are wrong because nullability needs to actually follow the type.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

[eiriktsarpalis]

Declaration has useless compiler warning. x and y are actually staticly typed to be not possibly null.

Why do you think it's useless? It indicates that if you were to pass your implementation to a collection that does run equality comparisons against null values, your code wouldn't be able to handle them. The contract for IEqualityComparer expects that null values should be accepted.

[jhudsoncedaron]

If you are writing a comparer that can be used with nulls you always have to implement IComparer<T?> rather than IComparer otherwise somebody who turns on nullable checks can't even use it.

By your own argument "It tells the implementer of the comparer implementation that it must support nulls" I expect zero cases of implementing IComparer<T> as opposed to T? in the standard libraries that are reference types.

[stephentoub]

If you are writing a comparer that can be used with nulls

Every comparer is supposed to be usable with null. That's the point.

[jhudsoncedaron]

Rather, Every standard comparer is supposed to be usable with null. When you have a comparer for which it's ill defined to compare an element in most of the space the type can store, handling nulls is completely superfluous.

[chrisoverzero]

You see, I'm talking about useless compiler warnings; the actual rule is "the comparer gets passed exactly the objects that interact with the collection" […]

can't pass null without violating the nullability constraint of the collection so we know this isn't null.

What collection?

StringComparer.OrdinalIgnoreCase.Equals(userInput, "activate") // "Activate" ==> true

[jhudsoncedaron]

@chrisoverzero

   string? userInput = null;
   (new VersionComparer).Compare(userInput, "0.0.0.0")

yields the expected compiler warning. StringComparer implements IComparable<string?>. VersionComparer (my class) implements IComparable<string>.

[RenderMichael]

Defense in depart is always a good thing. There’s no guarantee a null didn’t make it in the List<string>, especially if it got de-serialized or called by a null-oblivious caller. Best to handle the null scenario.

[jhudsoncedaron]

@RenderMichael : My most immediate encounter of it was a case where any object not in the list being sorted would be doomed to failure as I actually have to parse the string to order by it; checking for null doesn't really help.

[RenderMichael]

You can turn a hard-to-diagnose NullReferenceException into a meaningful InvalidOperationException that has an explanatory message.

[jhudsoncedaron]

Hint to anybody trying to answer this: if you answer doesn't handle the IEqualityComparer.GetHashCode case where the static typing says it can't be null but it definitely can be null (I can put a breakpoint on the return statement in if (item is null) return -1; and it will be reached); it's not an answer.

So far everybody's trying to say IComparer.Compare should always handle nulls; think about what the equivalent to that is for IEqualityComparer.GetHashCode ; and no changing the interface type is not an answer there; most of the time these are stock implementations of IEqualityComparer.

[RenderMichael]

It sounds like the disagreement here is with the nullability annotations placed on two specific types in the BCL, not with NRT broadly.

You can make a proposal to change the annotations forIComparer.Compare and IEqualityComparer.Equals. One option is to make them nullable-oblivious, like the return type for the non-generic IEnumerable.GetEnumerator().

The proposal could look something like this:

namespace System.Collections.Generic

public interface IComparer<T>
{
    public int Compare(
#nullable disable
        T x,
        T y
#nullable restore
    );
}

public interface IEqualityComparer<T>
{
    public bool Equals(
#nullable disable
        T x,
        T y
#nullable restore
    );
}

[jhudsoncedaron]

Ok now we're talking about something. I think that's a bit of a step backwards; I would totally expect intantiating IComparer with string to have a Compare(string, string) but a string? to have a Compare(string?, string?) so the null-ness through the T that is being implemented.

Both of these types have explicit markings to change the rules away from that. IComparer literally says int Compare(T? x, T? y); ; almost as though it's for an older version of nullable where it didn't flow right through generic types.

[RenderMichael]

Feel free to make an API proposal.

[elgonzo]

if (item is null) return -1;

think about what the equivalent to that is for IEqualityComparer.GetHashCode

Well, according to the IEqualityComparer<T>.GetHashCode documentation, an implementation is supposed to throw an ArgumentNullException when a null reference is passed to it: https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.iequalitycomparer-1.gethashcode#exceptions

[jhudsoncedaron]

That document was wrong when it was first written in .NET Framework 2.0 and it's wrong today. Just because a stock implementation of IEqualityComparer can't handle a null doesn't mean a custom one can't.

I have a local sensible meaning for a null key, and it makes sense in context; and it makes sense to use it as the key for an IDictionary. Since I'm not going to reduplicate the IDictionary implementation again after writing it once, it needs to take stock IEqualityComparar, and will pass null to whatever IEqualityComparer it has it if it gets a null.

If I hadn't had to rewrite a new IDictionary implementation anyway I'd be requesting the check in Dictionary be knocked out and just let the one in EqualityComparer.Default handle it.