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.

Sign up for GitHub

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

Jump to bottom

Use RuntimeHelpers.GetHashCode instead of obj.GetHashCode #2128

Closed
jozkee opened this issue Jan 24, 2020 · 5 comments · Fixed by #31756
Closed

Use RuntimeHelpers.GetHashCode instead of obj.GetHashCode #2128

jozkee opened this issue Jan 24, 2020 · 5 comments · Fixed by #31756
Assignees
@Marusyk
Labels
area-System.Text.Json bug good first issue Issue should be easy to implement, good for first-time contributors
Milestone
5.0.0

Comments

@jozkee
Copy link
Member

jozkee commented Jan 24, 2020

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceEqualsEqualityComparer.cs

Line 24 in 996e668

return obj!.GetHashCode();

Replace above line with the following to avoid misbehavior when comparing objects that use a bad implementation of GetHashCode

return RuntimeHelpers.GetHashCode(obj!);

Having a bad GetHashCode causes the Dictionary to always add an entry, even for objects that are identical.

Also update related tests.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime untriaged New issue has not been triaged by the area owner labels Jan 24, 2020
@jozkee jozkee added area-System.Text.Json and removed area-System.Runtime untriaged New issue has not been triaged by the area owner labels Jan 24, 2020
@jozkee jozkee added this to the 5.0 milestone Jan 24, 2020
@ahsonkhan ahsonkhan added bug good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels Jan 25, 2020
@ntovas
Copy link

ntovas commented Jan 25, 2020 •
edited
Loading

I would like to fix this, @jozkee can you assign me?

@Marusyk
Copy link
Member

Marusyk commented Jan 29, 2020

@ntovas are you working on this?

@ahsonkhan
Copy link
Contributor

@Marusyk - it's been a few days and I haven't heard back from @ntovas. Would you like to take this on?

@Marusyk
Copy link
Member

Marusyk commented Feb 4, 2020

yes, please

@ahsonkhan
Copy link
Contributor

Go for it :)

@ahsonkhan ahsonkhan assigned Marusyk and unassigned ntovas Feb 4, 2020
@ahsonkhan ahsonkhan removed the help wanted [up-for-grabs] Good issue for external contributors label Feb 4, 2020
@GrabYourPitchforks GrabYourPitchforks mentioned this issue Feb 4, 2020
Merged
@Marusyk Marusyk mentioned this issue Feb 4, 2020
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Marusyk Marusyk

Labels
area-System.Text.Json bug good first issue Issue should be easy to implement, good for first-time contributors
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

Use RuntimeHelpers.GetHashCode instead of obj.GetHashCode Marusyk/runtime
5 participants
@ahsonkhan @Marusyk @jozkee @ntovas @Dotnet-GitSync-Bot