Open manofstick opened 6 years ago
Urgs. Can you please submit this in a pr as a separate test to fsharp.core's test suite?
Also this makes a strong case for more property based tests.
Look at this:
It results with going this route https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/prim-types.fs#L838, i.e. comparing sbyte
arrays as byte
arrays with the wrong result.
The opposite has bug too
C# has it also
Labeling, though given that this is explicitly marked as "fast path" and the C# behavior, this may be by design.
It's by design to give wrong answer?
@cartermp
We'll the c# behaviour is not to compare array elements, so I don't think it's behaviour should be of any relevance here.
The comment re: fast path was due, I believe, to the fact that otherwise the general comparison would fall back to the array boxing comparer, not specifically to the sbyte case.
If this was by design there should have been some unit tests to show this functionality. The only tests that show the problem are the regression tests that I created to ensure backward compatibility when I was modifying equality and comparison operators - they should not be taken as truth, just regression. If there had been some generative tests in the first place they would have shown the problem sure to the inconsistency.
I'm perfectly in favor of this getting adjusted, I'm just a bit hesitant to advocate for that given the space. Lack of tests for that behavior is promising w.r.t making this change, but unfortunately there aren't always tests for intended behavior.
Whilst working on the
IComparable
(which includes the inequality operators) version of #5112, I found that the currently implementation ofsbyte[]
do not work correctly when one of the operands is negative.Repro steps
let a = -1y let b = 0y
printfn "%b" (a < b) printfn "%b" ([a] < [b]) printfn "%b" ([|a|] < [|b|])
Expected behavior
true true true
Actual behavior
true true false
Known workarounds
None. Well could convert the array to a list and then do a comparison there (if you can).
Related information
This was found when I was creating the alternative implementation and then running against the test suite created in #577. So presumably this has been a bug dating back prior to the creation of that regression suite (Aug 9, 2015). So probably no urgency to fix, and when the next PR that I'm working on is done then it will be cleared up.