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

Fix List.VNode.removeAfter() bug #2030

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

alexvictoor
Copy link

Hello!
I have added a failing test. I was not sure where to write it, let me know if it should be located elsewhere. I guess the bug comes from a bad copy/paste between VNode.removeBefore() & VNode.removeAfter().
Let me know if there is something missing in this PR.

@jdeniau
Copy link
Member

jdeniau commented Nov 8, 2024

Wow, this is an incredibly specific bug ! 😲
Nice catch !

For the record I did not write that code, and I do not fully understand this method and the left shift operator.

If you did understand this:

  • could you explain what you did understand ?
  • do you have another reproductible cas as I could not reproduce the issue with other cases than 287 and 33

@alexvictoor
Copy link
Author

alexvictoor commented Nov 9, 2024

Hello @jdeniau !
I guess you were right to ask, my PR is far from being perfect. I have just noticed some cases were not handled. I will fix that shortly. Also I might have found another issue in the removeBefore method.
To answer your question, I wanted to understand deeply the algorithm and the data structures used under the cover by ImmutableJS. Hence I have started to reimplement an immutable list from scratch (https://github.com/alexvictoor/ts-immutable).
A few weeks ago I had the feeling that my implementation had the main features and I started to compare the behaviours of the main methods with the original ImmutableJS List using property based testing and wonderful fast-check library (thanks @dubzzz). I found a lot of bugs in my implementation using fast-check and then I have found this bug :)
Might be difficult to explain the code incriminated. If you like, since we are both french, we can schedule a google meet. FYI I am willing to explain how ImmutableJS works under the cover at Paris Typescript meetup in a few months

@jdeniau
Copy link
Member

jdeniau commented Nov 9, 2024

@alexvictoor great, I sent you an invite.
For the record I have the secret dream of converting immutable to TS 🤞

@jdeniau
Copy link
Member

jdeniau commented Nov 19, 2024

@alexvictoor For the record, I did add the same test as v but testing the 10 000 first integers, and here is the list of the number that was failing, and that do pass now with your code:

33, 42, 95, 127, 159, 191, 223, 255, 287, 319, 351, 383, 415, 447, 479, 511, 543, 575, 607, 639, 671, 703, 735, 767, 799, 831, 863, 895, 927, 959, 991, 1023

Tested on 10 000, and the last failing number was 1023
@jdeniau jdeniau merged commit 9548738 into immutable-js:main Nov 19, 2024
5 checks passed
@jdeniau
Copy link
Member

jdeniau commented Nov 19, 2024

released in 5.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants