dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.91k stars 785 forks source link

Operators.( .. .. ) throws an ArgumentException when ^Step does not implement IComparable #6238

Open JanWosnitza opened 5 years ago

JanWosnitza commented 5 years ago

Operators.( .. .. ) throws an ArgumentException when ^Step does not implement non-generic System.IComparable.

Repro steps

Run the following code:

type Step(value:int) =
    member __.Value = value

    static member Zero = Step 0

type Value(value:int) =
    member __.Value = value

    static member (+)(a:Value, b:Step) = Value(a.Value + b.Value)

    interface System.IComparable with
        member a.CompareTo(b) =
            match b with
            | :? Value as b' -> compare a.Value b'.Value
            | _ -> failwith "unsupported"

[Value 0 .. Step 2 .. Value 2]

Expected behavior

I would expect a compiler error. Similar to

Actual behavior

The last line throws this exception:

System.ArgumentException: Failure during generic comparison: the type 'FSI_0006+Step' does not implement the System.IComparable interface. This error may be arise from the use of a function such as 'compare', 'max'
or 'min' or a data structure such as 'Set' or 'Map' whose keys contain instances of this type.
   at Microsoft.FSharp.Core.LanguagePrimitives.HashCompare.FailGenericComparison[a](Object obj)
   at Microsoft.FSharp.Core.LanguagePrimitives.HashCompare.GenericCompare(GenericComparer comp, Object xobj, Object yobj)
   at Microsoft.FSharp.Core.LanguagePrimitives.HashCompare.GenericLessThanIntrinsic[T](T x, T y)
   at Microsoft.FSharp.Core.Operators.OperatorIntrinsics.gen@4848-3[TStep,T](TStep zero, FSharpFunc`2 add, T start, TStep step, T stop, Unit unitVar0)
   at Microsoft.FSharp.Core.Operators.OperatorIntrinsics.RangeStepGeneric@4936-1.System-Collections-Generic-IEnumerable`1-GetEnumerator()
   at Microsoft.FSharp.Core.CompilerServices.GeneratedSequenceBase`1.MoveNextImpl()
   at Microsoft.FSharp.Collections.SeqModule.ToList[T](IEnumerable`1 source)
   at <StartupCode$FSI_0006>.$FSI_0006.main@()

Known workarounds

Run the code and realize you need to implement the interface System.IComparable on Step:

    interface System.IComparable with
        member a.CompareTo(b) = 
            match b with
            | :? Step as b' -> compare a.Value b'.Value
            | _ -> failwith "unsupported"

Or not using the start .. step .. finish operator, but generate the sequence somehow different (where step is not required to be comparable to Zero at runtime).

Related information

I had this issue when using 2 structs defined in C# and tried to iterate over them.

brianrourkeboll commented 6 months ago

This seems odd at first glance. The implementation of RangeStepGeneric in prim-types.fs, which is called in this case for (.. ..), uses =, >, and < on step, which would normally force the equality and comparison constraints to bubble up:

https://github.com/dotnet/fsharp/blob/fc42ec9bc18740ccc3a1e41ed1082cb091f5c31f/src/FSharp.Core/prim-types.fs#L5790-L5796

https://github.com/dotnet/fsharp/blob/fc42ec9bc18740ccc3a1e41ed1082cb091f5c31f/src/FSharp.Core/prim-types.fs#L5942-L5944

https://github.com/dotnet/fsharp/blob/fc42ec9bc18740ccc3a1e41ed1082cb091f5c31f/src/FSharp.Core/prim-types.fs#L6032

https://github.com/dotnet/fsharp/blob/fc42ec9bc18740ccc3a1e41ed1082cb091f5c31f/src/FSharp.Core/prim-types.fs#L6911-L6912

The signature for (.. ..) in prim-types.fsi, however, specifies neither the equality constraint nor the comparison constraint for ^Step, unless ^Step is unified with ^T. I wonder if that plays into it at all?

https://github.com/dotnet/fsharp/blob/fc42ec9bc18740ccc3a1e41ed1082cb091f5c31f/src/FSharp.Core/prim-types.fsi#L3756-L3762

It would be a compile-time breaking change to update the signature for (.. ..) to include and ^Step: equality and ^Step: comparison, but since any existing code where ^Step doesn't conform to those constraints already fails at runtime, maybe it could be done?

abelbraaksma commented 6 months ago

It seems like auto-inference doesn't add the and ^Step: comparison. Normally that happens automatically if you use it (and it is used, as you mention):

> let foo a b = a > b;;
val foo: a: 'a -> b: 'a -> bool when 'a: comparison

But also note that the *.fsi file has and default ^Step: ^T. This should be enough to infer that ^Step requires equality and comparison as well. Is this a bug in using the default ^X: ^Y syntax?

abelbraaksma commented 6 months ago

@brianrourkeboll, I don't know if you tested the OP's example code, but I just tried it with the latest from main and it throws another tantrum:

error FS0193: The member or object constructor 'op_Addition' is not public. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.

image

This member is clearly accessible...

This, I think, is a regression. I cannot repro this with the latest RTM of F#.

brianrourkeboll commented 6 months ago

I don't know if you tested the OP's example code, but I just tried it with the latest from main and it throws another tantrum:

error FS0193: The member or object constructor 'op_Addition' is not public. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.

Hmm, I do not get that when using a VSIX built from main in the last week or two.

abelbraaksma commented 6 months ago

Ok, this is weird.

I pasted it under module internal RangeTestsHelpers = in the test file PrimTypes.fs. Now, I would argue that any internal code should be accessible from other internal code, but this is likely a different issue and should be reported separately.

Once I put the code in a public module, it worked as expected.

Note that, if I change the definition to be this (as you suggested):

val inline (.. ..): start: ^T -> step: ^Step -> finish: ^T -> seq< ^T >    
                        when (^T or ^Step): (static member (+): ^T * ^Step -> ^T) 
                        and ^Step: (static member Zero: ^Step)
                        and ^T: equality
                        and ^T: comparison                                
                        and ^Step: equality
                        and ^Step: comparison                                
                        and default ^Step: ^T
                        and default ^T: int

it will behave as the OP suggested (though the error is thrown 8 times???)

error FS0193: The type 'Step' does not support the 'comparison' constraint. For example, it does not support the 'System.IComparable' interface

brianrourkeboll commented 6 months ago

I pasted it under module internal RangeTestsHelpers = in the test file PrimTypes.fs. Now, I would argue that any internal code should be accessible from other internal code, but this is likely a different issue and should be reported separately.

That seems to be #16762.