Runtime: StringComparers should return 0 rather than throwing for GetHashCode(null)

[madelson]

EqualityComparer<string>.Default.GetHashCode returns 0 when provided with null.

The various StringComparers, however, throw ArgumentNullException. This is frustrating because it means you can't always swap in something like StringComparer.OrdinalIgnoreCase for the default comparer.

Additionally, it feels inconsistent for StringComparers to throw on null for GetHashCode because all other methods are null-safe:

StringComparer.Ordinal.Equals(default(string), default(string)) => true
StringComparer.Ordinal.Compare(default(string), default(string)) => 0

Therefore, I think we should consider changing the behavior of GetHashCode to be consistent with everything else.

[jnm2]

Am I remembering wrong, or do Roslyn and ReSharper generate implementations of GetHashCode that delegate to EqualityComparer<string>.Default.GetHashCode as a way to avoid the branch in str != null ? 0 : str.GetHashCode()?

[joshfree]

/cc @tarekgh

[tarekgh]

Although I agree on the principle of consistency here, but this can break libraries which taken a dependency on such behavior for different comparers. Hopefully, this is not the case :-)

Also, other people can create their own custom comparers and throw on null strings too. So, if your code allows custom comparers, then your code needs to handle such cases anyway.

[jnm2]

The breaking change rules say:

✓ Allowed

• Increasing the range of accepted values for a property or parameter if the member _is not_ virtual

Note that the range can only increase to the extent that it does not impact the static type. e.g. it is OK to remove if (x > 10) throw new ArgumentOutOfRangeException("x"), but it is not OK to change the type of x from int to long.

It sounds like in general it's okay to assume that people don't depend on argument validation exceptions.

[madelson]

Also, other people can create their own custom comparers and throw on null strings too. So, if your code allows custom comparers, then your code needs to handle such cases anyway.

I agree that it's true that any particular custom comparer could throw for any particular argument. That said, in my case (and I'll bet in many others) I'm not using "anyone's" comparer, I'm explicitly choosing .NET's built-in custom comparer for case-insensitivity. It feels like that, at least, should be expected to work seamlessly as a replacement for EqualityComparer<string>.Default.

[jnm2]

The fact that StringComparer implements IEqualityComparer<string> rather than IEqualityComparer<string?> is really annoying:

string.Join(", ", values
    .Where(values => !string.IsNullOrWhiteSpace(values))
    // CS8620 Argument of type 'StringComparer' cannot be used for parameter 'comparer' of type
    // 'IEqualityComparer<string?>' in 'IEnumerable<string?> Enumerable.Distinct<string?>(
    // IEnumerable<string?> source, IEqualityComparer<string?> comparer)' due to differences in
    // the nullability of reference types.
    //        ↓
    .Distinct(StringComparer.CurrentCultureIgnoreCase))

This should work even specifically without the null check.

Would it be possible to change StringComparer to implement IEqualityComparer<string?> now that it has shipped implementing IEqualityComparer<string>?

[tarekgh]

@stephentoub @buyaa-n

@jnm2 has a nullable question. I think we can convert StringComparer to implement IEqualityComparer instead of IEqualityComparer

[stephentoub]

The fact that StringComparer implements IEqualityComparer<string> rather than IEqualityComparer<string?> is really annoying:

First, please open a new issue for such things. It's not related to this issue.

Second, I don't understand the concern: StringComparer _does_ inherit IEqualityComparer<string?>. Please clarify.
https://github.com/dotnet/corefx/blob/967eb3c5cb5fd36c8bb56d06f4c840f6a7a5a754/src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs#L989

[jnm2]

I'm sorry, I should have checked the corefx source. The warning above is a tooling bug then.

First, please open a new issue for such things. It's not related to this issue.

If the warning was correct and StringComparer really did differ in nullability from IEqualityComparer<string?>, it would imply that StringComparer implements IEqualityComparer<string>. The only good reason for it not to implement IEqualityComparer<string?> that I could think of was because StringComparer.GetHashCode(null) throws, so it would then depend on this issue.

[buyaa-n]

@jnm2 I see from code base StringComparer is implemented IEqualityComparer https://github.com/dotnet/coreclr/blob/5b1c001bc0fb5bb1ce035c02ec275763f66defc8/src/System.Private.CoreLib/shared/System/StringComparer.cs#L14
which just means methods implemented from IEqualityComparer StringComparer.Equals(string? x, string? y), StringComparer.GetHashCode(string? s) should accept null values. But the the exising implementation of the StringComparer.GetHashCode(string s) is throwing on null even the interface is allowing null. In other words StringComparer.GetHashCode(null) throws not because of StringComparer implements IEqualityComparer<string> instead of IEqualityComparer<string?>. So it is not nullabilty related issue, it is implementation issue/enhancement