System.Numerics.Vector2.Equals doesn't use float.Equals in Equals implementation

[rotanov]

It is possible to add multiple new System.Numerics.Vector2(float.NaN, float.NaN) to a HashSet<object>:

While it isn't possible to add multiple float.NaN because

float.NaN == float.NaN is false

float.NaN.Equals(float.NaN) is true

This applies to other System.Numeric.* structures as well.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

---

It is possible to add multiple new System.Numerics.Vector2(float.NaN, float.NaN) to a HashSet<object>:

While it isn't possible to add multiple float.NaN because

float.NaN == float.NaN is false

float.NaN.Equals(float.NaN) is true

Author:	rotanov
Assignees:	-
Labels:	area-System.Numerics, untriaged
Milestone:	-

[tfenise]

#13872

[rotanov]

@tfenise thank you, I'm sorry I didn't search properly enough. I read the discussion and it seems most likely this issue won't be addressed.

I agree with @CodesInChaos

I also want to point to what MSDN says Object.Equals Method:

The following statements must be true for all implementations of the Equals(Object) method. In the list, x, y, and z represent object references that are not null.

• x.Equals(x) returns true, except in cases that involve floating-point types. See ISO/IEC/IEEE 60559:2011, Information technology -- Microprocessor Systems -- Floating-Point arithmetic.
• x.Equals(y) returns true if both x and y are NaN.

Indeed instances of System.Numerics types with all members being NaN are not NaN by themselves but on intuitive level it seems that the semantic should stay the same.

Removing .Equals invocations from "all hash based sets and dictionaries, Contains, Find, IndexOf on various collections/LINQ, set based LINQ operations (Union, Except, etc.)." ((c) @CodesInChaos) and only using .GetHashCode() instead will break much more things and it will not be possible to provide custom equality comparer in such places.

So I suggest one of two following things:

1. Remove JIT intrinsics from System.Numerics.*.Equals(*) and leave them on == and != operators. This will enable being able to use these types with associative containers and we'll still have performant ways to compare mathematical objects.

2. Explicitly point in System.Numerics Namespace documentation that all types from the namespace are not safe for use with associative containers provided by .NET. And in the documentation for each Equals method note about this behavior and how it is different from float.Equals and double.Equals

---

My case. I was iterating over properties of some data recursively with reflection. The idea was 1. find values of types for which validators are defined and validate them 2. test getters. There's a Vector2 implementation in our game engine and I quickly stumbled upon a case of vector instance properties returning other Vector2 instances resulting in infinite recursion and stack overflow. I thought I can mitigate that by storing all processed instances in HashSet<object>. Finally I got another stack overflow, I looked into hashset and it was full of {NaN,NaN} Vector2 instances. I investigated how float avoids that and "fixed" Vector2.Equals implementation. Since we were gonna start using System.Numerics instead of our implementation I investigated how things are with System.Numerics.Vector2.

I understand this approach is not very good and something like Vector2 property Vector2 RandomVector2 => new Vector2(RandomFloat(), RandomFloat()) will easily break what I tried to achieve with hashset, but it was all a research anyway. Sorry for the off topic.

[tannergooding]

I read the discussion and it seems most likely this issue won't be addressed.

I have a different opinion on this than Eric did and am willing to consider the breaking change here.

It is unexpected that Vector2/3/4 cannot be used in hash tables in all scenarios and the logic required to support this while still remaining performant is not actually overly complex. Its basically the following:

  vmovupd xmm0, [rdx]             ; load left into xmm0
  vmovupd xmm1, [r8]              ; load right into xmm1
  vcmpeqps xmm2, xmm0, xmm1       ; get mask of left == right following IEEE 754 rules
+ vcmpunordps xmm0, xmm0, xmm0    ; get mask of NaN values in left
+ vcmpunordps xmm1, xmm1, xmm1    ; get mask of NaN values in right
+ vandps xmm0, xmm0, xmm1         ; get mask of NaN values in left & right
+ vorps xmm0, xmm0, xmm2          ; combine mask of IEEE equality with mask of NaN values in left & right
  vmovmskps eax, xmm0             ; extract upper bit of each element from mask to integer register
  cmp eax, 0xf                    ; compare to see if all entries are "true"
  sete al                         ; normalize to 1 or 0
  movzx eax, al                   ; zero extend result

This is notably 4 instructions more expensive than the current implementation and is a breaking change from current behavior. However, it would only impact users relying on .Equals equality and isn't that much more expensive considering. I also believe we could reduce this cost a bit more if I sat down to work out a better sequence.

[tannergooding]

Marking this as ready-for-review as it needs to be considered by the API Review team more generally as to:

1. Do we want to support using Vector2/3/4 in something like hash tables or dictionaries?
2. If yes, is the breaking change here acceptable or do we want to provide an analyzer to help flag incorrect usages and require a customer comparer be used instead

My own take on 1 is that this is largely no different from a Point and it is conceivable that a user would want to track points (effectively 2 or 3 dimensional indices) in a hash set. There are indeed potential issues with equality in some cases, as minor differences in an algorithm can produce "near" results due to the imprecise nature of floating-point values. However, this is also not a unique problem to Vector2/3/4 and is one that users may have to more generally consider anyways.

Having an analyzer is potentially a decent alternative to help catch incorrect usages of this.

[CodesInChaos]

Concerning efficient implementation: Discussion on stackoverflow

[rotanov]

Having an analyzer is potentially a decent alternative to help catch incorrect usages of this.

I don't think the analyzer will help in case of adding objects to a HashSet<object> though.

[tannergooding]

It likely wouldn't no. Analyzers will never catch all cases, but they can certainly help catch common cases and pitfalls, which is the general goal.

I think fixing it is likely the better option, but there are several factors to consider and we may choose a different path ultimately.

[terrajobst]

Video

• We're OK with taking this breaking change for .NET 7.0
• Should we add an analyzer that flags usages of == when comparing floating point data types in the body of an Equals method?

[tannergooding]

@terrajobst, I think you mean .NET 7. It's likely too late to take this for .NET 6 😄

[rotanov]

Should we add an analyzer that flags usages of == when comparing floating point data types in the body of an Equals method?

That would be great!

[danmoseley]

@tannergooding should tihs be 'up for grabs' (possibly for @rotanov if he's interested)?

[tannergooding]

@danmoseley, this one likely requires JIT work. If someone feels confident enough in C# and C++ to do both sides of the work, then I'm happy to assign it out. But it's not necessarily an "easy" issue.

[danmoseley]

"up-for-grabs" doesn't imply easy, we've had community members handle that kind of task before I think. But it maybe nobody "grabs" it...