fsharp / fslang-suggestions

The place to make suggestions, discuss and vote on F# language and core library features
345 stars 21 forks source link

Satisfy the "comparison" constraint with IComparable<'T> #816

Open teo-tsirpanis opened 4 years ago

teo-tsirpanis commented 4 years ago

I propose we allow a class that implements just the generic IComparable<'T> interface, satisfy the comparison constraint.

The existing way of approaching this problem in F# is by implementing the non-generic interface IComparable (or IStructuralComparable). However this approach is not ideal, because it requires a type cast (and potentially unboxing) of a value of type obj. Implementing all three interfaces would increase boilerplate code even more.

Pros and Cons

The advantages of making this adjustment to F# are increased ease of defining a custom comparison function, and conformity to modern coding practices by using generic interfaces.

The disadvantages of making this adjustment to F# are none, I guess. As for backwards compatibility, we can provide an automatic implementation of the two non-generic interfaces, if there are not specified.

Extra information

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

Related suggestions: The issue was first reported in https://github.com/dotnet/fsharp/issues/7945.

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

cartermp commented 4 years ago

I haven't seen a similar suggestion, though I'd be surprised if this hasn't been brought up before...

...regardless, I think this would be great. It's very surprising to me that the non-generic one satisfies, but the generic one does not.

Also, I really wish equality and comparison in .NET wasn't so difficult. Just a quick glance at .NET FX for comparison

IComparable has 76 implementations and 216 references IComparable<'T> has 41 implementations and 25 references IComparer has 102 implementations and 200 references IComparer<'T> has 62 implementations and 209 references

So if we're going by implementations/references, then the non-generic IComparable is the best choice for satisfying the comparison constraint. But as you can see, the framework certainly makes use of all four to a large degree.

teo-tsirpanis commented 4 years ago

IComparable has 76 implementations [...]

@cartermp, keep in mind that many of them are either in private or internal types, or in the likes of System.Web or LINQ to SQL.

And you were talking about .NET Framework. I guess this has changed with .NET Core.

cartermp commented 4 years ago

And you were talking about .NET Framework. I guess this has changed with .NET Core.

I would expect none of that to change with .NET Core, at least for any existing API, otherwise it would be an API breaking change.

teo-tsirpanis commented 4 years ago

I’m referring to more public types implementing the generic interfaces.

abelbraaksma commented 4 years ago

While I'm in favor of this, and I vaguely remember this has been discussed before, it may not be so trivial as it seems.

This is not a breaking change to the F# language design

I'm not sure this is true.

Considering that the compiler currently expects the non-generic IComparable, and suppose this were accepted, how can existing non-generic code that relies on IComparable compare two objects that are not of the same type? If you would only implement IComparable<T>, this is not possible,and existing code would fail to compile. Meaning that you would still have to implement the non-generic version anyway.

At best I see this moving forward if the compiler can take advantage of the fact that the generic interface is implemented (not sure if it currently does). But simply allowing the comparison constraint to be satisfied when only the generic interface is implemented, I'm not sure that's even possible.

In addition, when you implement IComparable<T>, you should also implement IEquatable<T>, and overload the comparison and equation operators (as strongly recommended by the docs, under 'Notes to implementers'). So, if we replace the constraint to mean that the generic, but not the non-generic is to be implemented, that's going to result in more work for programmers.

(that is not to say that it would be good to have this, and esp with value types it can yield a far better performance too)

cartermp commented 4 years ago

@abelbraaksma Currently, these are the rules:

If the type is a named type, then the type definition does not have, and is not inferred to have, the NoComparison attribute, and the type definition implements System.IComparable or is an array type or is System.IntPtr or is System.UIntPtr.

(section 5.2.10 of the spec)

This suggestion would change it to be (bolded difference):

If the type is a named type, then the type definition does not have, and is not inferred to have, the NoComparison attribute, and the type definition implements System.IComparable, or System.IComparible<'T>, or is an array type, or is System.IntPtr, or is System.UIntPtr.

I don't believe that would be a breaking change. Existing nongeneric code that doesn't meet the current rules would continue to fail to satisfy that constraint.

teo-tsirpanis commented 4 years ago

@abelbraaksma

overload the comparison and equation operators (as strongly recommended by the docs, under 'Notes to implementers').

This advice is for C# developers.

If you would only implement IComparable, this is not possible,and existing code would fail to compile.

If you would only implement IComparable<T>, your intent is that this type must not be compared against objects of another type.

Then the compiler would still implement IComparable behind the scenes, by casting the object (and raising an exception if it is of a different type), and defer to IComparable<T> for the actual comparison.

abelbraaksma commented 4 years ago

I don't believe that would be a breaking change. Existing nongeneric code that doesn't meet the current rules would continue to fail to satisfy that constraint.

@cartermp, You are right, maybe my concern was less of a concern than I thought. However, I still think it is a breaking change, though arguably much smaller, and probably only in edge cases:

The problem is with "or". Previously, any code that expects a type having the comparison constraint can rely on the fact that IComparison is implemented. After this change, it can be either IComparison, IComparison<T> or both.

I believe that when the BCL was faced with the same issue, they typically introduced different methods for sorting: either generic, or non-generic. Perhaps this all can be inferred and none of my concerns above will end up becoming real-world problems, but on the face of it, I think there are certainly risks involved.

(again, still would love this in, if it is possible and with little backwards-compatibility issues)

abelbraaksma commented 4 years ago

Just some thoughts/ideas (not necessarily good ones):

  1. We'd default to the generic version if both are present
  2. If a type is comparable and has IComparable implemented, but not IComparable<'T>, then the compiler will emit boilerplate code for IComparable<T> that invokes the user-code of the IComparable implementation. We may emit a warning.
  3. But what if the type is not user-code? Will the compiler magically switch when a function expects comparison and casts to IComparable or IComparable<'T> and if either is not present, implement this on the fly?
  4. Perhaps existing library code can be updated to prefer the generic over the non-generic implementation if both are present
baronfel commented 4 years ago

how would this interact with the language suggestion around allowing multiple implementations of interfaces with distinct generic arguments? Would the derived IComparable implementations type-test and invoke the an appropriate matching member? If so, what order would the types be checked? Declaration order? Some normalized name ordering?

charlesroddie commented 3 years ago

I think this goes together with IEquatable and IEquatable. Can those be added to the suggestion? There was work done on IEquatable in https://github.com/dotnet/fsharp/pull/6175 and I hope that can be returned to.

@abelbraaksma In addition, when you implement IComparable, you should also implement IEquatable, and overload the comparison and equation operators

Even though that is still the official advice (https://docs.microsoft.com/en-us/dotnet/api/system.iequatable-1?view=net-5.0), the non-generic interfaces are only needed to support non-generic collections which are obsolete . So I think the advice is unnecessary at this point.

It is so horrible to have to write:

type T(value:int) =
    member t.Value = value
    interface System.IEquatable<T> with member x.Equals y = x.Value = y.Value
    override x.Equals(yObj:obj) =
        match yObj with
        | :? T as y -> x.Value = y.Value
        | _ -> failwith "incompatible types"
    override x.GetHashCode() = 0