sbyte array comparison doesn't work correctly with negative values

[manofstick]

Whilst working on the IComparable (which includes the inequality operators) version of #5112, I found that the currently implementation of sbyte[] 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.

[forki]

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.

[vasily-kirichenko]

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.

[vasily-kirichenko]

The opposite has bug too

[vasily-kirichenko]

C# has it also

[vasily-kirichenko]

https://blogs.msdn.microsoft.com/ericlippert/2009/09/24/why-is-covariance-of-value-typed-arrays-inconsistent/

[vasily-kirichenko]

Fixed in #5267

[cartermp]

Labeling, though given that this is explicitly marked as "fast path" and the C# behavior, this may be by design.

[forki]

It's by design to give wrong answer?

[manofstick]

@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.

[cartermp]

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.