Skip to content
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

List.dropAt drops the wrong index #7065

Closed
ageron opened this issue Sep 8, 2024 · 3 comments
Closed

List.dropAt drops the wrong index #7065

ageron opened this issue Sep 8, 2024 · 3 comments
Labels
bug Something isn't working builtins P-high High priority/frequency

Comments

@ageron
Copy link
Contributor

ageron commented Sep 8, 2024

It looks like List.dropAt is currently broken:

  The rockin' roc repl
────────────────────────

Enter an expression, or :help, or :q to quit.

» [0,10,20,30,40,50] |> List.dropAt 0

[10, 20, 30, 40, 50] : List (Num *)               <-- OK
» [0,10,20,30,40,50] |> List.dropAt 1

[0, 10, 20, 30, 40] : List (Num *)                <-- WRONG!
» [0,10,20,30,40,50] |> List.dropAt 2

[0, 10, 20, 30, 40] : List (Num *)                <-- WRONG!
» [0,10,20,30,40,50] |> List.dropAt 3

[0, 10, 20, 30, 40] : List (Num *)                <-- WRONG!
» [0,10,20,30,40,50] |> List.dropAt 4

[0, 10, 20, 30, 40] : List (Num *)                <-- WRONG!
» [0,10,20,30,40,50] |> List.dropAt 5

[0, 10, 20, 30, 40] : List (Num *)                <-- OK
» [0,10,20,30,40,50] |> List.dropAt 6

[0, 10, 20, 30, 40, 50] : List (Num *)            <-- OK
@ageron
Copy link
Contributor Author

ageron commented Sep 8, 2024

$ roc version
roc nightly pre-release, built from commit 9a4d556 on Sat Sep  7 09:02:00 UTC 2024

@ageron
Copy link
Contributor Author

ageron commented Sep 8, 2024

I'm really surprised this regression didn't break any tests.
It looks like none of the existing test cases cover the case where you drop at an index in the middle of the list. As a result, none of the tests go past this line in the Zig implementation.

@Anton-4 Anton-4 added the P-high High priority/frequency label Sep 9, 2024
@Anton-4
Copy link
Collaborator

Anton-4 commented Sep 9, 2024

Thanks for the investigation @ageron!
We should definitely:

  • add failing examples to the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins P-high High priority/frequency
Projects
None yet
Development

No branches or pull requests

3 participants