Skip to content

[FS-1138] Avoid boxing on equality and comparison#747

Closed
Happypig375 wants to merge 2 commits intofsharp:mainfrom
Happypig375:patch-27
Closed

[FS-1138] Avoid boxing on equality and comparison#747
Happypig375 wants to merge 2 commits intofsharp:mainfrom
Happypig375:patch-27

Conversation

@Happypig375
Copy link
Copy Markdown
Contributor

@Happypig375 Happypig375 commented Aug 5, 2023

Click “Files changed” → “⋯” → “View file” for the rendered RFC.


# Motivation

Currently, F#'s equality and comparison operators [cause boxing on equality and comparison for value types](https://github.com/dotnet/fsharp/issues/526). However, an attempt to fix this through the use of `IEquatable<T>` and `IComparable<T>` was [rejected](https://github.com/dotnet/fsharp/pull/9404) citing the breakage for `nan` equality.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps I'm mistaken, but I thought it was rejected on the grounds that it always looked for IEquatable<'T>.Equals first instead of obj.Equals, which has the potential to break any class implementation that doesn't follow the (non-required) .NET guidance for implementing IEquatable<'T>.Equals. That's a far larger breaking change than simply breaking nan comparison.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only systematical deviation is NaN comparison which may apply to System.Half, and other floating point representations that have a NaN value.

Do you have more examples of IEquatable<`T> deviating from obj.Equals?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've not seen anyone on the compiler team state that this only affects nan equality. The PR linked says nothing about that, it explicitly states that anyone can make obj.Equals and IEquatable<T>.Equals have different behavior, and this would be a runtime-breaking change for them. Over the course of 13+ years of F# being a supported enterprise product in VS, there is undoubtedly code in the wild that works perfectly today and will be broken silently when there is an updated F# compiler delivered by their tools.

Not saying this means the feature stops, but I've simply not seen "we need preserve the weird nan behavior" as the actual reason for not accepting work like this in the past.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, so hypothetically someone somewhere could be broken. Any examples?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This chat got a bit stale, but @Happypig375 I think you are asking about where obj.Equals can result in different response than IEquatable.Equals.

The simplest example is probably that the default implementation for obj.Equals is often reference equality, which would return false for two different instances of the same object (let's say two persons that are have field name = "Happy Pig"). Using IEquatable.Equals(a, b), this would return true instead.

Another case might be certain time and date instances, perhaps from NodaTime, but I haven't checked, where two instances may be bitwise unequal, but considered equal after resolving, say, the timezone.

However, an argument can be made that if you implement IEquality.Equals that you would want it to behave similar to object.Equals.

Copy link
Copy Markdown
Member

@abelbraaksma abelbraaksma May 16, 2024

Choose a reason for hiding this comment

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

In fact, reading more on it in several blog posts and about Microsoft accepting the oddity of nan = nan => false, nan <> nan => true, but forcing the incompatible change for nan.Equals(nan) => true and (nan :> IEquatable<float>).Equals(nan) => true strengthens me in the believe that making this change would be a good thing ™️. :)

@abelbraaksma
Copy link
Copy Markdown
Member

@Happypig375 I am wondering if the new PR dotnet/fsharp#16857, which was just merged, removes the requirement for this RFC and/or replaces it.

I'll make a note in that particular PR as well. Perhaps we can update this RFC to address the changes over there done by @psfinaki?

@psfinaki
Copy link
Copy Markdown

So this suggestion, starting with its title in the RFC doc, mixes a few things together: NaNs and unnecessary boxing, and also this unnecessary boxing is currently happening in many places. Generally, the situation is quite nicely overviewed here now.

The NaN problem is still there and should be addressed. I guess we can either rewrite this RFC or close this one + a new one. Just saying that the more focused the suggestions are, the easier they are to reason about. I know I sound like Captain Obvious but IMO that's exactly the reason why all these problems have been take so long here.

@abelbraaksma
Copy link
Copy Markdown
Member

abelbraaksma commented May 17, 2024

No, I totally agree. And by focusing the NaN problem to mimic the C# track, we would have a more tailored and succinct discussion.

Ideally, I would just compare all blittable types as binary blobs and have the fastest speed up possible for equality, but unfortunately, that's not going to work for many reasons (some of which are shown in that white paper on equality optimizations).

@T-Gro
Copy link
Copy Markdown
Contributor

T-Gro commented Feb 12, 2025

In the context of dotnet/fsharp#16857 , I will close this PR in it's current form.
The remains of the RFC would be better rebranded as focusing on nan <> nan itself - most of the text in the .md would still apply.

(this PR can be then reopened)

@T-Gro T-Gro closed this Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants