Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KalleOlaviNiemitalo opened this issue Jul 24, 2020 · 7 comments
Labels
area-System.Runtime enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jul 24, 2020

#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 #39624 (comment), 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
Copy link
Collaborator

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.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 24, 2020
@KalleOlaviNiemitalo
Copy link
Author

The area label might be either area-Infrastructure-libraries (because this is a packaging request) or area-System.Runtime (as in #39624).

@AaronRobinsonMSFT
Copy link
Member

/cc @ericstj

@ericstj
Copy link
Member

ericstj commented Jul 24, 2020

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
Copy link
Author

KalleOlaviNiemitalo commented Jul 25, 2020

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).

@joperezr joperezr added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Aug 24, 2020
@joperezr joperezr added this to the Future milestone Aug 24, 2020
@KalleOlaviNiemitalo
Copy link
Author

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
Copy link
Member

ericstj commented Aug 30, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

6 participants