Repository navigation
List.map3 misleading error when lists of different lengths #6897
Description
List.map3 (fun () () () -> ()) [();()] [()] [()]
// System.ArgumentException: The lists had different lengths.
// list1.Length = 1, list2.Length = 0, list3.Length = 0This is misleading because the lists in user code have lengths 2,1,1 not 1,0,0.
src/fsharp/FSharp.Core/local.fs
let map3 mapping xs1 xs2 xs3 =
match xs1, xs2, xs3 with
| [], [], [] -> []
| h1 :: t1, h2 :: t2, h3 :: t3 ->
...
map3ToFreshConsTail cons f t1 t2 t3
cons
| xs1, xs2, xs3 -> ...map3ToFreshConsTail operates on t1,t2,t3, recursively pops heads off, and fails if at least one but not all of the input lists are empty. It then reports the lengths of the current lists, not the lengths of the original lists.
A fix:
try
map3ToFreshConsTail cons f t1 t2 t3
cons
with _ ->
invalidArg3ListsDifferent "list1" "list2" "list3" xs1.Length xs2.Length xs3.Length
Activity
baronfel commentedon May 29, 2019
That comes at the cost of incurring three length checks to report the error, each of which are
O(n). I'd suggest alteringmap3ToFreshConstTailto take tuples of(currentLength, list)for the t1, t2, t3 parameters to enable fast error reporting.charlesroddie commentedon May 29, 2019
Is fast error reporting important? More important not to make valid cases slower surely?
cartermp commentedon May 29, 2019
Any performance considerations here should be benchmarked, as we don't know what the actual performance of either approach will be. Though I would prefer incurring the cost on the error.
charlesroddie commentedon May 29, 2019
A faster version of this would be to add a single (alreadyCut:int) parameter to map3ToFreshConstTail to add to all the lengths in the error case.