[proxy] web.archive.org — ← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ fsharp Public
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

List.map3 misleading error when lists of different lengths #6897

Closed
charlesroddie opened this issue May 29, 2019 · 4 comments · Fixed by #6980
Closed

List.map3 misleading error when lists of different lengths #6897

charlesroddie opened this issue May 29, 2019 · 4 comments · Fixed by #6980
Labels
Area-Library Issues for FSharp.Core not covered elsewhere Bug
Milestone
16.3

Comments

Copy link
Contributor

charlesroddie commented May 29, 2019 •
edited 

List.map3 (fun () () () -> ()) [();()] [()] [()]
// System.ArgumentException: The lists had different lengths.
// list1.Length = 1, list2.Length = 0, list3.Length = 0

This 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
Copy link
Member

baronfel commented 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 altering map3ToFreshConstTail to take tuples of (currentLength, list) for the t1, t2, t3 parameters to enable fast error reporting.

Copy link
Contributor Author

charlesroddie commented May 29, 2019

Is fast error reporting important? More important not to make valid cases slower surely?

Copy link
Contributor

cartermp commented 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.

cartermp added Area-Library Issues for FSharp.Core not covered elsewhere Bug labels May 29, 2019
cartermp added this to the Backlog milestone May 29, 2019
Copy link
Contributor Author

charlesroddie commented May 29, 2019 •
edited

I'd suggest altering map3ToFreshConstTail to take tuples of (currentLength, list) for the t1, t2, t3 parameters to enable fast error reporting.

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.

reacheight mentioned this issue Jun 11, 2019
Merged
cartermp modified the milestones: Backlog, 16.3 Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Library Issues for FSharp.Core not covered elsewhere Bug
Projects
None yet
Milestone
16.3
Development

Successfully merging a pull request may close this issue.

fixed List.map3 misleading error message on different length lists reacheight/fsharp
3 participants