List.remove behaving weirdly

[SimonMeskens]

I think there's something wrong with List.remove:

import {List, Mutation} from "collectable";

let list = List.fromArray([1, 2, 3, 4, 5]);

let modify = Mutation.modify(list);

List.remove(0, modify);
List.remove(2, modify);
List.remove(4, modify);

modify = Mutation.commit(modify);
console.log(List.toArray(modify));

let modify2 = Mutation.modify(list);

List.remove(0, modify2);
List.remove(1, modify2);
List.remove(2, modify2);

modify2 = Mutation.commit(modify2);
console.log(List.toArray(modify2));

Output:

[2, 3, 3] Uncaught RangeError: Invalid array length at Slot.cloneWithAdjustedRange (bundle.js:8284) at View.adjustSlotRange (bundle.js:9291) at sliceInternal (bundle.js:8162) at Object.sliceList (bundle.js:8076) at Object.deleteValues (bundle.js:9017) at removeRange (bundle.js:7105) at Object.remove (bundle.js:7100) at Object.1.collectable (bundle.js:14) at s (bundle.js:1) at e (bundle.js:1) cloneWithAdjustedRange @ bundle.js:8284 adjustSlotRange @ bundle.js:9291 sliceInternal @ bundle.js:8162 sliceList @ bundle.js:8076 deleteValues @ bundle.js:9017 removeRange @ bundle.js:7105 remove @ bundle.js:7100 1.collectable @ bundle.js:14 s @ bundle.js:1 e @ bundle.js:1 (anonymous) @ bundle.js:1

[SimonMeskens]

I've narrowed down the issue to the fact that cloneList in values.ts doesn't copy the _left and _right Views, it just passes them. I think that's a bug? Separate lists shouldn't share View objects I think.

[SimonMeskens]

In deleteValues in values.ts, we find the following lines (68 - 71):

var right = cloneList(list, nextId(), true);
  sliceList(list, 0, start);
  sliceList(right, end, right._size);
  list = concatLists(list, right);

If the list is mutable, right and list share their Views and Slots, so that slice and concat doesn't work. I can fix it in deleteValues, but I'm not sure what the use of a clone operator is, if it shares Views and Slots, so I think it's actually cloneList that needs fixing.

[SimonMeskens]

ugh, scratch that, that's not the issue it seems. I'm at a loss what is.

The first remove always works, but the second one fails

[axefrog]

You definitely picked the most complicated structure to attack! Overcomplicated even, as per what we discussed earlier.

Your first code sample is puzzling because you appear to be deleting elements in ascending order without accounting for the decreased size of the list during each subsequent operation. Operations are immediate, the commit nomenclature does not imply latebound resolution or anything like that, it simply flips the mutation flag to "frozen". On that note, you don't need to use the return value of the commit function; it's just provided as a convenience when used as the last call of a function that returns the modified collection instance.

Let me know if that changes your test case at all, and then I'll take a look and see if I can find the cause of the problem.

[SimonMeskens]

Yeah, I was trying a bunch of things figuring out what went wrong, so the example got a bit daft, but I swear I know what I'm doing ;)

Maybe I'll make a PR with my finalized code and we can have a look at a real example, but there's definitely a bug somewhere, as far as I can tell, due to ghosted changes in values.ts, lines (68 - 71)

[axefrog]

If you write a test case and make a preliminary PR which can remain pending until we get the test to pass, I will sync the PR locally and help figure out the problem.

[axefrog]

are you still experiencing issues with this? If not, I'll go ahead and close it.