search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.26k stars 4.73k forks source link

Publish Microsoft.Bcl.HashCode supporting HashCode.Add(null, comparer) #39895

Open KalleOlaviNiemitalo opened 4 years ago

KalleOlaviNiemitalo commented 4 years ago

https://github.com/dotnet/runtime/pull/32090 changed the public void Add<T>(T value, IEqualityComparer<T> comparer) method of System.HashCode so that it no longer calls comparer.GetHashCode(value) if value == null. That is particularly useful if the comparer is a StringComparer that would throw if given null.

Can a new version of the Microsoft.Bcl.HashCode NuGet package be published with this change? I'd like to use it on .NET Framework or generally on .NET Standard 2.0.

According to https://github.com/dotnet/runtime/issues/39624#issuecomment-662031667, Microsoft.Bcl.HashCode is not built nowadays. The current source code of System.HashCode uses nullable reference types, perhaps making the binaries difficult to support on .NET Standard 2.0.

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

KalleOlaviNiemitalo commented 4 years ago

The area label might be either area-Infrastructure-libraries (because this is a packaging request) or area-System.Runtime (as in https://github.com/dotnet/runtime/issues/39624).

AaronRobinsonMSFT commented 4 years ago

/cc @ericstj

ericstj commented 4 years ago

We cannot change API in HashCode since it must unify with the version that’s already inbox on .NETCore.

We can change implementation but we should do that in servicing so that the fix isn’t missing from 3.x (for similar reasons).

I’m not sure it’s worth it for this change. Can the caller check null themself and just call Add(null) without the compared?

Cc @terrajobst @danmosemsft

KalleOlaviNiemitalo commented 4 years ago

If the Microsoft.Bcl.HashCode package has to match .NET Core 3.1, does it have to match .NET Core 2.1 as well?

At the caller side, I suppose the easiest workaround would be to define an AddNullable<T> extension method and ban the original.

M:System.HashCode.Add``1(``0,System.Collections.Generic.IEqualityComparer{``0})

Alternatively, a dedicated analyzer could offer a code fix and warn only if T may be nullable (which in C# 7 includes all reference types).

KalleOlaviNiemitalo commented 4 years ago

Will this become easier to do when .NET Core 3.1 goes out of support on December 3, 2022? At that time, the change could still be valuable for use with .NET Framework.

ericstj commented 4 years ago

It is still a servicing change to make this fix. I don't imagine it'll be any easier to justify this change in 2 years. If you have a justification to make now that can be tested against the servicing bar it'd be best to share that now.