[Breaking] Avoid boxing on equality and comparison

Happypig375 commented 1 year ago

I propose we solve https://github.com/dotnet/fsharp/issues/526 (accepting the breaking change at https://github.com/dotnet/fsharp/pull/9404 due to use of IEquatable<T> and IComparable<T>). We will include a BackCompat module to enable past behaviour. These current behaviours typically have no known instance of usage and the breaking change is only theoretical for code that already is designed poorly in the first place, like .Equals behaving differently from IEquatable<T> and relying on nan <> nan without using System.Double.IsNaN (IEquatable<double> makes nan = nan).

The existing way of approaching this problem in F# is taking the cost of boxing every equality and comparison for structs. Retaining the same behaviour has made F# perform worse than C#. We are keeping a reason for C# people not to move over simply because we retain a behaviour no one wants.

https://github.com/dotnet/fsharp/issues/526#issuecomment-1045552667

Anyway, it has lost me as an fsharp developer, which is a shame as I really love writing code in fsharp, but with advancements in csharp over the years, it's, for the type of code I write, a lesser worse option

Pros and Cons

The advantages of making this adjustment to F# are

Great performance
One less reason for C# people not to move over
One less reason for people to abandon F#

The disadvantage of making this adjustment to F# is this is a breaking change after all. But as mentioned, it's a hypothetical breaking change only. There is no known instance of people depending on IEquatable<T> behaving differently from .Equals as this is poor design.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions:

We should do this together with another wart in FSharp.Core that has gone unnoticed, in the same update: https://github.com/fsharp/fslang-suggestions/issues/472

Affidavit (please submit!)

Please tick this by placing a cross in the box:

[x] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
[x] I have searched both open and closed suggestions on this site and believe this is not a duplicate
[ ] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it. This is arguing against a previous decision not to accept this breaking change.

Please tick all that apply:

[ ] This is not a breaking change to the F# language design
[x] I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

T-Gro commented 12 months ago

I personally believe the likelihood would increase if this would be an opt-in behavior, e.g. a separate project-wide setting. Like a language-feature flag which is preview only, not becoming the default for vNext automatically.

vzarytovskii commented 12 months ago

I personally believe the likelihood would increase if this would be an opt-in behavior, e.g. a separate project-wide setting. Like a language-feature flag which is preview only, not becoming the default for vNext automatically.

I think for this particular change an opt out is good. Nobody will be able to explore and opt into it.

charlesroddie commented 12 months ago

I would love to get the perf benefits here, would love to work with generic IEquatable<'T>/IComparable<'T> rather than the non-generic ones, and would love to have the property always satisfied that x = x (i.e. putting compliance with logic above compliance with IEEE).

cartermp commented 11 months ago

Just here to say that this:

Estimated cost (XS, S, M, L, XL, XXL): S

Is something I'd put in L or XL size. There were several attempts to land this in the compiler before and one the reasons why is because it's complex and difficult to implement. There's also a long tail of legacy code that will now fail at runtime in subtle ways, which needs to be dealt with.

charlesroddie commented 11 months ago

Is something I'd put in L or XL size. There were several attempts...

I would say this suggestion (deferring to IEquatable<T> whenever possible) is S. Other approaches that were tried were L or XL because they tried to tiptoe around the nan bug.

We would need a notice similar to https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/7.0/equals-nan .

There's also a long tail of legacy code that will now fail at runtime in subtle ways...

It would be good to see a concrete example. Is there some existing plausible code which would depend, for correctness, on x <> x where x = nan?

I think it's much more likely that there will be a long tail of subtle bugs that would be fixed by this change, rather than created by this change, since it is much more likely that programs would assume that x = x rather than x <> x.

Happypig375 commented 11 months ago

Draft RFC here https://github.com/fsharp/fslang-design/pull/747

vzarytovskii commented 11 months ago

It would be good to see a concrete example. Is there some existing plausible code which would depend, for correctness, on x <> x where x = nan?

Pretty much all code which uses floating point comparison, may implicitly rely on it. So it will be hugely runtime breaking. Another question, whether these applications relying on NaNs.

vzarytovskii commented 7 months ago

General discussion for breaking changes in corelib: https://github.com/dotnet/fsharp/discussions/16231

u3r commented 3 weeks ago

Question: is https://github.com/dotnet/fsharp/issues/526 a) a partial fix for this b) a full fix for this c) unrelated?

vzarytovskii commented 3 weeks ago

It's a partial fix for the problem, a workaround really, at the cost of additional codegen.

fsharp / fslang-suggestions