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

[Breaking] Avoid boxing on equality and comparison #1280

Open
3 of 5 tasks
Happypig375 opened this issue Jun 30, 2023 · 10 comments
Open
3 of 5 tasks

[Breaking] Avoid boxing on equality and comparison #1280

Happypig375 opened this issue Jun 30, 2023 · 10 comments

Comments

@Happypig375
Copy link
Contributor

I propose we solve dotnet/fsharp#526 (accepting the breaking change at dotnet/fsharp#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.

dotnet/fsharp#526 (comment)

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

  1. Great performance
  2. One less reason for C# people not to move over
  3. 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: #472

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • 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
  • 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 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@T-Gro
Copy link

T-Gro commented Jul 3, 2023

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
Copy link

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
Copy link

charlesroddie commented Jul 3, 2023

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
Copy link
Member

cartermp commented Aug 5, 2023

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
Copy link

charlesroddie commented Aug 6, 2023

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
Copy link
Contributor Author

Draft RFC here fsharp/fslang-design#747

@vzarytovskii
Copy link

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
Copy link

General discussion for breaking changes in corelib: dotnet/fsharp#16231

@u3r
Copy link

u3r commented Jun 10, 2024

Question: is dotnet/fsharp#526
a) a partial fix for this
b) a full fix for this
c) unrelated?

@vzarytovskii
Copy link

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

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

No branches or pull requests

6 participants