Fable icon indicating copy to clipboard operation
Fable copied to clipboard

Array.compareWith issue

Open ncave opened this issue 3 years ago • 4 comments

The current Array.compareWith implementation in Fable is inconsistent with the F# implementation and documentation. Here is an example:

let a = [|1;3|]
let b = [|1;2;3|]
// compares lengths first, then elements
let c1 = a < b // equals true
let c2 = compare a b // equals -1
// should compare elements first, then lengths
let c3 = Array.compareWith compare a b // equals 1 in .NET, -1 in Fable
printfn "c1 = %A, c2 = %A, c3 = %A" c1 c2 c3

I'm not sure why FSharp.Core has this inconsistent compare behavior for arrays only (unlike in other collections), but it's there, so this probably needs to be made the same in Fable.

  • [x] Rust
  • [x] JS
  • [ ] Python
  • [ ] Dart

ncave avatar Jul 19 '22 08:07 ncave

Oh my, it's so confusing that compare operator and Array.compareWith have different behavior that I'm tempted to keep it "fixed" in Fable 😅 But anyways I've sent #2967 to fix it. We should do it for the other languages as well.

alfonsogarciacaro avatar Jul 21 '22 07:07 alfonsogarciacaro

@alfonsogarciacaro Yes, somewhat inconsistent behavior, especially since the rest of the collections don't do that (as checking length can be prohibitively expensive or impossible on infinite sequences). It's probably just a performance optimization for arrays only, but still, confusing.

I was initially thinking of proposing a change for it in the F# repo, but there is very little chance of it being accepted since it's a breaking change on existing code.

ncave avatar Jul 21 '22 13:07 ncave

I btw need to wait for main sync before I can fix Python since Array.fs fixes is not available in snake_island yet.

dbrattli avatar Aug 05 '22 07:08 dbrattli

Sorry @dbrattli, I've finally synced with main. Let's focus now releasing Fable 4 so we don't need to spend time syncing the branches. I assume the fix will automatically propagate to Python as the Array module is shared, but you may need to make a change in Replacements. Please check this commit (you can also use the test for Python): https://github.com/fable-compiler/Fable/commit/dea7dbc1c518d8b8c43aa8148aab01a84fe1e7e0

alfonsogarciacaro avatar Aug 17 '22 01:08 alfonsogarciacaro