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

(=) uses non-generic equality on custom types #9398

Closed
charlesroddie opened this issue Jun 4, 2020 · 9 comments
Closed

(=) uses non-generic equality on custom types #9398

charlesroddie opened this issue Jun 4, 2020 · 9 comments

Comments

@charlesroddie
Copy link
Contributor

type T() =
    interface System.IEquatable<T> with member x.Equals _ = true
    override x.Equals _ = failwith "non-generic equality"
    override x.GetHashCode() = 0
T() = T() // System.Exception: non-generic equality

The current .Net advice is: If you implement IEquatable, you should also override ... Equals(Object).

The reason for this advice is: Non-Strongly typed collections and Frameworks don’t use IEquatable.

However now DE0006: Non-generic collections shouldn't be used.

So the expectation is that if you don't use .Net collections that predate generics, which you shouldn't do, then non-generic equality will not be used in preference to generic equality.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 4, 2020

I think an attempt to fix this was here, but fell short of completion as of yet: #6175. In the last comments, the problem with implemented interfaces is explained, though I don't fully understand it.

Also, this thread is related: #9348.

@TIHan
Copy link
Contributor

TIHan commented Jun 5, 2020

Related: #526

@manofstick
Copy link
Contributor

@charlesroddie

This is kinda fixed by this PR, although it mightn't work 100% how you want it!

In the above example, you would have to add a [<Sealed>] attribute, because the comparer otherwise doesn't know if a derived class would implement IStructuralEquatable, and hence it falls back to the current way f# does it.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 10, 2020

@manofstick, so this fix works correctly for records and unions, they are sealed, and for value types, they cannot be overridden.

That raises the question that, by the time you fall through, you're either having a non sealed type that doesn't implement IEquatable, or you have a type that implements it. Wouldn't a type test by that time solve the issue? Boxing then doesn't matter anymore, because you're going to call a virtual method anyway, or is it not possible to implement such path deterministically?

@manofstick
Copy link
Contributor

@abelbraaksma I think it would just make things more complex... I mean if you have an object hierarchy you probably shouldn't be using a specific IEquatable on a base type anyway... So marking a class that does have one as sealed probably isn't a terrible thing anyway...

And boxing is never an issue for these, because this only affects reference types, so the call to equals is free (although obviously there is the cast back of the comparand, but this going to be at least as fast as a type check). Although it does do the other checks (am I IStructuralEquatable, am I an array, maybe that's it...), so possibly slight shaving of time there...

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 10, 2020

@manofstick Oh yes, that makes sense, I didn't realize that a virtual call on Equals gets dispatched to the furthest descendent anyway. I gotta stay awake ;). And if anyone is crazy enough to implement IEquatable without overriding Equals and GetHashCode, that's their funeral.

I suddenly understand why the compiler always complains if I forget to do that. My mind goes like "why the extra work, you already have IEquatable", it all makes sense now :)

@dsyme
Copy link
Contributor

dsyme commented Aug 24, 2020

This is by design, the F# spec makes it clear that in the worst case Equals gets used for the implementation of a = b

@charlesroddie
Copy link
Contributor Author

in the worst case Equals gets used for the implementation of a = b

Why would this be the worst case? The type implements System.IEquatable<T> so there isn't a need for the worst-case Object.Equals is there?

@charlesroddie
Copy link
Contributor Author

The corresponding language suggestion is fsharp/fslang-suggestions#816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants