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

Make System.HashCode available as standalone nuget package #26412

Closed
MaStr11 opened this Issue Jan 18, 2018 · 13 comments

Comments

Projects
None yet
8 participants
@MaStr11
Copy link

MaStr11 commented Jan 18, 2018

The new System.HashCode type seems to be distributed as part of the core runtime (Based on this Comment by @morganbr in #14354).

The implementation is in System.Private.CoreLib.dll, so it would come as part of the runtime package. The contract is System.Runtime.dll.

System.HashCode could could benefit a wider audience, if it is distributed as standalone (.netstandard based) package. Every .NET developer on every platform sooner or later faces the problem to implement GetHashCode and doing so is fraught with pitfalls (there are more than 350 comments in total in this repro alone discussing a good hash code algorithm and a good API to expose it). Therefore an official standalone package with a good general purpose algorithm and a well designed API would be much appreciated. (I would even suggest to make System.HashCode part of .netstandard, but that's another topic).

@joshfree

This comment has been minimized.

Copy link
Member

joshfree commented Jan 18, 2018

Hi @MaStr11 - this is related to the existing tracking issue #25666. Please continue the discussion on #25666. Thanks! 😄

@joshfree joshfree closed this Jan 18, 2018

@MaStr11

This comment has been minimized.

Copy link

MaStr11 commented Jan 18, 2018

@joshfree

I'm sorry, but I must disagree. There are some fundamental differences between #25666 and this issue:

  1. #25666 discusses a future API, while I’m asking for a finished – soon to be released – API, to be made available by a different mechanism.
  2. #25666 seems to focus on hashing of streams. So, this is more about calculating checksums. System.HashCode only purpose is to serve making implementing GetHashCode easier, hence the return value of all System.HashCode methods is int. The API in #25666 is much more complicated and doesn’t really help implementing GetHashCode (e.g. public byte[] ComputeHash(ReadOnlySpan<byte> data);).
  3. The need for such an helper was raised in dotnet/roslyn#17646 by @Pilchie a few month ago and recently by @CyrusNajmabadi here

Original request:

We should test the framework and use them instead of hardcoding strange constants and stuff into the user's code if they invoke Generate Equals/GetHashCode.

Recent question from @CyrusNajmabadi about the availability System.HashCode

Note: what nuget package is this api even being included in for us to add a reference to?

In PR dotnet/roslyn#24161 @CyrusNajmabadi already takes dependency on System.HashCode and it seemed he was as surprised as I was when he learned that System.HashCode wasn’t made available as package:

Ok. If that's the case, then it sounds like a user would get this if/when they move to a more recent Target Framework. That sort of thing is not at all a step i would have the "generate equals+hashcode" do to a user's project.

If there are any technical restrictions or other reasons that prevent System.HashCode from being released standalone, you can close this issue. But #25666 is not an approriate substitute for what I'm asking for. In other words: I want to be able to use System.HashCode in the full .Net Framework via Install-Package System.HashCode

@joshfree joshfree reopened this Jan 18, 2018

@morganbr

This comment has been minimized.

Copy link
Contributor

morganbr commented Jan 18, 2018

This sounds like it would work similarly to ValueTuple (shipped as a combination of in-box and out-of-band). I do like the idea of making HashCode available to more people sooner, but I seem to recall that what we did with ValueTuple had some complications. @karelz, do you have details/thoughts on that?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 18, 2018

shipped as a combination of in-box and out-of-band

We call these "partial OOBs" and they are incredibly hard and expensive to get right and maintain. We try really hard to avoid introducing new ones.

If we want to ship HashCode as independent package, the in-box and out-of-band packages should have different assembly names. It would be basically two different types. System.Diagnostics.Tracing (in-box) and Microsoft.Diagnostics.Tracing.EventSource (out of band) is a good prior art to use for this.

@morganbr

This comment has been minimized.

Copy link
Contributor

morganbr commented Jan 18, 2018

@CyrusNajmabadi, would Roslyn be able to take advantage of such an out-of-band package? You indicated previously that adding a package reference as part of a code fix might be too heavyweight.

@CyrusNajmabadi

This comment has been minimized.

Copy link

CyrusNajmabadi commented Jan 19, 2018

would Roslyn be able to take advantage of such an out-of-band package?

Yes. In that we already have a mechnism to suggest nuget packages to users if they reference type names from those packages. We can also offer specific nuget packages in certain circumstances. For example, we know specifically about System.ValueTuple and we offer that package if you use tuples.

That said, IMO, there's little pressure to produce such a nuget package. I think we're at a reasonable point now. We have reasonably good stories for users who are targeting newer and older frameworks. If they're targetting newer frameworks, we'll use System.HashCode. If they're targetting older ones, we'll just spit out a reasonable GetHashCode impl.

IMO, this is a good-enough situation for users. It's already leaps and bounds ahead of what's the current state of affairs.

@CyrusNajmabadi

This comment has been minimized.

Copy link

CyrusNajmabadi commented Jan 19, 2018

In other words: if someone did the work to make a nuget package, i think i'd b willing to surface that in some specialized way. But I'm not specifically asking for that package to be created. I'm satisfied with where we are now.

@MaStr11

This comment has been minimized.

Copy link

MaStr11 commented Jan 19, 2018

This sounds like it would work similarly to ValueTuple

I think this would be more like System.Numerics.Vectors or Microsoft.Net.Http, than ValueTuple, but yes that was the intend here. I was wondering why this package is part of the core runtime at all. Couldn't this be published completely out of band? What part of the core runtime takes dependency on this?

But I'm not specifically asking for that package to be created. I'm satisfied with where we are now.

While this is true from the IDE perspective, I'd like to point out that there are plenty developers out there, that would benefit from such a package. Take for instance all the the line-of-business application devs:

  • LOB developers are often forced to work on older platforms or the full framework (ASP.NET WebForms, WinForms, WPF, etc.)
  • LOB developers often need to implement equality, because they operate on business objects with composite keys (e.g. if you create an EF Code-First model).
  • LOB developers are much more relying on other libraries and don't do low level stuff that often. (xor is considered exotic in that domain)
  • LOB developers are not very vocal and therefore often ignored, but this is still a huge crowd (being one myself).

While those developers are well served by the generate equals/hashcode code fixes (as @CyrusNajmabadi pointed out), I still see great value in having System.HashCode on the other platforms because:

  • It is more performant
  • The generated code is less obscure (compare with and without System.HashCode)

I would also suggest to create a roslyn analyzer, that detects GetHashcode anti-patterns (e.g. a.GetHashCode() + b.GetHasCode()) and suggests to replace it with either System.HashCode or the code currently generated by the GetHashCode code generator. And in such a case a package import would make sense (while it doesn't make sense in the equals/gethashcode generator). I think this is something @CyrusNajmabadi had in mind when he said:

if someone did the work to make a nuget package, i think i'd b willing to surface that in some specialized way.

And as a last note. This package would nothing less than bring .NET on a par with Java 7 from 2011. [PS: You may have a look how reviewed JDK-6891113 back then 😄].

@jnm2

This comment has been minimized.

Copy link
Collaborator

jnm2 commented Jan 19, 2018

I can confirm the netfx LOB developers' hope that we'll finally get a palatable API for implementing GetHashCode.

That said, I don't want a repeat of the series of pitfalls that I recall coming with partial OOB packages. I think partial OOBs could do just fine if a lot of design effort was put into specifically making them a first-class scenario on all platforms, perhaps by creating an improved way to unify types at runtime.

@CyrusNajmabadi

This comment has been minimized.

Copy link

CyrusNajmabadi commented Jan 19, 2018

While this is true from the IDE perspective, I'd like to point out that there are plenty developers out there, that would benefit from such a package. Take for instance all the the line-of-business application devs:

That's fine. My point is that this is something you guys need to figure out :) You get to decide if this is the right choice for how you deliver and whatnot. The IDE specifically is not requesting this. I'm only pointing this out so that whoever is making the decisions here doesn't think that that ask is coming from that direction.

@jnm2

This comment has been minimized.

Copy link
Collaborator

jnm2 commented Apr 24, 2018

Just checking in. I have multiple .NET Framework IEqualityComparer implementations duplicating corefx code with comments pointing to this issue. Is this a definite no, no hashing API will be available for .NET Framework developers?

@MaStr11

This comment has been minimized.

Copy link

MaStr11 commented Jul 30, 2018

It seems that System.Hashcode is going to be part of a future dotnet/standard#823

https://github.com/dotnet/standard/pull/823/files#diff-2e4f92c1322ad4c9bcd1107011e336a7R2439

This makes shipping it as a package irrelevant.

@MaStr11 MaStr11 closed this Jul 30, 2018

@YairHalberstadt

This comment has been minimized.

Copy link
Contributor

YairHalberstadt commented Nov 15, 2018

@MaStr11
.Net Framework will likely never implement .Net Standard 2.1
As such, can I suggest you reopen this issue?
Thanks

@karelz karelz modified the milestones: Future, 3.0 Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment