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

Fixing Nan=Nan behavior for Set and Map collection types #14515

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Dec 28, 2022

Fixes #14507 bug .

Per documentation, GenericEqualityER "Compare two values for equality using equivalence relation semantics ([nan] = [nan])". It is a sibling to GenericEquality , which treats nan<>nan.

This nan=nan behaviour GenericEqualityER in should work for structural data, incl. collections, unions, records etc.
Summary of existing behavior for various types, when holding nan (or nanf):
Scalar value = OK
Lists = OK (generated code coming from DU implementation)
Array = OK (special-cased in Fsharp.Core)
Sets = GenericEqualityER behaves the same like GenericEquality , i.e. set [nan] <> set [nan] => wrong
Maps = fails just like it does for Sets, both for Keys and for values

An inconsistency between lists and Sets/Maps is considered a bug, because all 3 of them are typical F# constructs for data modelling.
The fact that they did not support GenericEqualityER out of the box was an implementation-detail-bug.

This PR addresses this by having both Set<> and Map<,> implement interface System.Collections.IStructuralEquatable.
For Maps, I decided to use the passed in comparer for both keys and values (one can argue about the logical validity of floats as keys in a map, but it is technically possible).

@T-Gro T-Gro linked an issue Dec 28, 2022 that may be closed by this pull request
Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it comes to NaNs in collections, we may want to discuss what we should treat as correct behaviour, since there are some more inconsistencies:
#13207

@T-Gro
Copy link
Member Author

T-Gro commented Dec 28, 2022

When it comes to NaNs in collections, we may want to discuss what we should treat as correct behaviour, since there are some more inconsistencies: #13207

I am aware of that issue, that is more opinionated because it asks what the implicit default should be.

In the case of
GenericEqualityER
GenericEquality

functions, it is nothing to discuss - those are explicit and documented functions, which state what kind (nan=nan vs nan<>nan) is used internally.

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 28, 2022

@T-Gro
Also, could you please summarize the behaviour in the issue description? I.e. before/after, for anyone who may read it in future.

@T-Gro
Copy link
Member Author

T-Gro commented Dec 28, 2022

@T-Gro Also, could you please summarize the behaviour in the issue description? I.e. before/after, for anyone who may read it in future.

@T-Gro Also, could you please summarize the behaviour in the issue description? I.e. before/after, for anyone who may read it in future.

added

@T-Gro T-Gro marked this pull request as ready for review December 28, 2022 16:55
@T-Gro T-Gro requested a review from a team as a code owner December 28, 2022 16:55
@tannergooding
Copy link
Member

tannergooding commented Dec 28, 2022

When it comes to NaNs in collections, we may want to discuss what we should treat as correct behaviour, since there are some more inconsistencies:

It would probably make sense for F# to match the .NET rules here, which is that object.Equals or IEquatable<T>.Equals is used to determine equality. This means x == x, but also +0 == -0 (as is the case for operator ==) and NaN == NaN (which differs from operator ==).

  • Doing bitwise equality is potentially problematic since that would mean two NaNs that differ by sign or payload would not be compared as "equal" and that could lead to general confusion for consumers
  • Doing IEEE 754 total order equality is also potentially problematic for similar reasons. NOTE: This is a relational comparison that does take into account signs, so +0 != -0, and other bits. Thus +qNaN != +sNaN and +qNaN != -qNaN, etc. .NET 8+ will expose this via System.Numerics.TotalOrderIeee754Comparer<T>

In general, GetHashCode and Equals are considered a pair and the docs for these methods require they be implemented in such a way so-as to work for dictionaries, sets, maps, or other such keyed collections.

@T-Gro T-Gro requested review from 0101 and vzarytovskii January 2, 2023 13:53
@vzarytovskii vzarytovskii enabled auto-merge (squash) January 3, 2023 12:38
@vzarytovskii vzarytovskii merged commit a9c194a into main Jan 3, 2023
@T-Gro T-Gro deleted the 14507-genericequalityer-is-broken-for-sets branch January 3, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

GenericEqualityER is broken for sets.
5 participants