Inconsistencies in EqualityComparer null treatment

[franklin-ross]

EqualityCompare<int?>.Default().GetHashCode(null).Dump();                          // == 0
EqualityCompare<int?>.EquateBy(k => k, allowNulls:true).GetHashCode(null).Dump();  // == 585969562
EqualityCompare<int?>.EquateBy(k => k, allowNulls:false).GetHashCode(null).Dump(); // == 585969562

//All true.
EqualityCompare<int?>.Default().Equals(null, null).Dump();
EqualityCompare<int?>.EquateBy(k => k, allowNulls:true).Equals(null, null).Dump();
EqualityCompare<int?>.EquateBy(k => k, allowNulls:false).Equals(null, null).Dump();

Is this expected behaviour?

• I would have thought .Default() would be equivalent to one of the other two for .GetHashCode(null).
• I also expected .Equals(null, null) would either throw or return false for allowNulls:false.

Just came across these as I was writing some tests for this unordered sequence comparer. My null treatment was off, so I've been investigating how the existing classes handle it.

[franklin-ross]

I was hoping to lean on the element comparer used to create the sequence comparer for null handling, thinking it would either return a valid hash for a dictionary to use if it handles nulls, otherwise throwing a neat exception if it wasn't supposed to allow nulls.

Unfortunately Dictionary<,> just spits exceptions on null keys anyway, even if the comparer you create it with supports them. Still, I can handle that with a special case so long as the element comparer can let me know whether nulls are OK.

So far my unordered sequence compare code works with IEqualityComparer<TElement> because that's all I thought I needed, but I could inherit from something in your hierarchy if it reliably lets me detect whether an element comparer likes nulls or not. I'm not sure EqualityComparerBase<T> will get me there.

EDIT: I avoided having to do any capability detection by checking whether the element type is a reference type and using the element comparer to check each value against null in that case. That leaves the decision up to the comparer, which is what I wanted in the first place.

[StephenCleary]

The rules around handling null are fairly complex. The "normal" behavior is to have all null values be less than any non-null value, and all nulls equal to each other. I seem to recall there are parts of the .NET framework that actually assume this even for custom comparers, so tread carefully.

Different comparers may return different hash codes. In the case of Default, you'll get the actual IEqualityComparer<int?>.GetHashCode value. But EquateBy is actually implemented as a Null comparer linked to a Select comparer.

The allowNulls parameter is actually whether null values will be passed on to the user delegate. If it is false (the default), the comparer will follow the "normal" behavior and not invoke the delegate for null values. If true, the comparer will invoke the delegate for null values. Under no circumstances should a comparer operation throw.

[franklin-ross]

Ah, that all makes sense. I did wonder why that bool existed, since everything dealt with null already.

The rules around handling null are fairly complex. The "normal" behavior is to have all null values be less than any non-null value, and all nulls equal to each other. I seem to recall there are parts of the .NET framework that actually assume this even for custom comparers, so tread carefully.

Different comparers may return different hash codes. In the case of Default, you'll get the actual IEqualityComparer.GetHashCode value. But EquateBy is actually implemented as a Null comparer linked to a Select comparer.

The allowNulls parameter is actually whether null values will be passed on to the user delegate. If it is false (the default), the comparer will follow the "normal" behavior and not invoke the delegate for null values. If true, the comparer will invoke the delegate for null values. Under no circumstances should a comparer operation throw.

— Reply to this email directly or view it on GitHub https://github.com/StephenCleary/Comparers/issues/2#issuecomment-60436158.