specialNullHandling parameter ignored

[franklin-ross]

I just hit an exception that I didn't expect where null values are being passed into an inner comparer, which then chokes. I've got a minimal repro below where I would expect the default null handling to kick in and protect the ordinal string comparer from null values, but they always seem to get passed through, no matter whether I specify true or false for specialNullHandling.

Is this expected behaviour? If so, how do I go about making this tolerant of null keys?

void Main()
{
    Measurement.StructuralComparer.GetHashCode(new Measurement { Key = null });
}

struct Measurement
{
    public string Key { get; set; }

    public static readonly IEqualityComparer<Measurement> StructuralComparer =
        EqualityComparerBuilder.For<Measurement>()
            .EquateBy(x => x.Key, StringComparer.Ordinal, specialNullHandling: false);
}

[StephenCleary]

I'll take a look at this. specialNullHandling is one of the most complex parts of this library.

[StephenCleary]

Ah, yes. specialNullHandling applies to the input of that comparer. So in your example, it would only apply when Measurement is null, not Measurement.Key. See this unit test. The key comparer (StringComparer.Ordinal in this case) is the one that decides how to handle null keys.

Microsoft's comparers accept null values for comparison, but not GetHashCode. Nito comparers accept null values for comparison and GetHashCode. So, you can work around this behavior by defining a Nito comparer wrapper for the key, something like:

struct Measurement
{
    public string Key { get; set; }

    // Wrap StringComparer.Ordinal so GetHashCode doesn't throw on null.
    private static readonly IEqualityComparer<string> KeyComparer =
        EqualityComparerBuilder.For<string>().EquateBy(x => x, StringComparer.Ordinal);

    public static readonly IEqualityComparer<Measurement> StructuralComparer =
        EqualityComparerBuilder.For<Measurement>()
            .EquateBy(x => x.Key, KeyComparer);
}

Not terribly pretty, but it works. In this case, the KeyComparer will just handle null values and pass on any non-null values to StringComparer.Ordinal.

[franklin-ross]

I see. I did wonder whether the null handling was on the principle object, but the fact that there was a parameter for null handling on .EquateBy() made me think it applied to the mapped property value instead.

Would I be right in saying the specialNullHandling property of .EquateBy() actually relates to whether I want to receive nulls in the delegate I pass in, and has nothing to do with the comparer I provide?

[StephenCleary]

Yes, exactly. specialNullHandling is whether or not you'll get null arguments passed to your delegate.

I'll update the docs to be clearer.

[StephenCleary]

The docs for specialNullHandling already say A value indicating whether <c>null</c> values are passed to <paramref name="selector"/>.

One possible enhancement would be to make the wrapping easier. Having to use the identity selector is kind of ugly. I could add something like a NormalizeNullHandling() extension method. The default comparers already normalize the null handling, so it may make sense to do this as an extension method as well. I did consider adding wrapper instances for StringComparer instances (similar to how the Nito.Comparers default comparers work), but I felt it wasn't useful enough for the API complexity. An extension method would be cleaner.

Example usage:

struct Measurement
{
    public string Key { get; set; }

    public static readonly IEqualityComparer<Measurement> StructuralComparer =
        EqualityComparerBuilder.For<Measurement>()
            .EquateBy(x => x.Key, StringComparer.Ordinal.NormalizeNullHandling());
}

The problem is that the user still "just has to know" to use it, which I don't like. It doesn't really guide the user to the path of success.

[franklin-ross]

I see, that does make sense, thanks. I'm usually pretty good at reading the doc comments, but this time I did just assume.

I agree that it's not very clear, but also that it's a bit of a hard problem to make intuitive for users beyond the comments. Probably the extension method is a good way to go. I feel that a more "canonical" way to do the normalisation would be good because knowing to wrap an existing comparer for this purpose is not immediately obvious, even if it does make sense.