bugs: list returns wrong results (likely bug in drop function)

[emmanueltouzery]

noticed this in uses from my prelude.ts library :-(

list appears to get confused by a series of operations, let's say its internal structures get corrupted, and it returns wrong results.

I made a reproduction script:

const L = require("list")

let f = L.list()
for (let i=0;i<1329;i++) {
    f = L.append(i, f);
}

const b = L.drop(L.length(L.takeWhile(l=>l<=200, f)), f)
const c = L.takeWhile(l=>l<=203, b)
const d = L.drop(L.length(c), b)

console.log("FIRST ISSUE first() doesn't agree with nth(0)")
console.log(L.first(d))
console.log(L.nth(0, d))

const c1 = L.append(L.nth(0, d), c)
console.log("SECOND ISSUE list of 4 drop first and last, wrong result")
console.log(L.toArray(c1))
console.log(L.toArray(L.dropLast(1, L.drop(1, c1))))

The output is:

FIRST ISSUE first() doesn't agree with nth(0)
201
204
SECOND ISSUE list of 4 drop first and last, wrong result
[ 201, 202, 203, 204 ]
[ 201, 202 ]

[emmanueltouzery]

simplified the first bug to just two drops:

const L = require("list")

let f = L.list()
for (let i=0;i<1329;i++) {
    f = L.append(i, f);
}

const xx = L.drop(3, L.drop(201, f))
console.log(L.first(xx))
console.log(L.nth(0, xx))

-- sanity check
const xx1 = L.fromArray(L.toArray(xx));
console.log(L.first(xx1))
console.log(L.nth(0, xx1))

output is =>

201
204
204
204

[paldepind]

Thank you for reporting the issue @emmanueltouzery. I'm looking into it!

[paldepind]

See #65. I've found the bug and reproduced it with a smaller test case. I'd love to get your feedback on the alternative solution I mention in the PR 😄

Thanks again for reporting the issue with a nice and simple test case 👍

[emmanueltouzery]

did you check that your fix fixes both issues reproduced through the first script? The second issue from the first script may be independent because drop and dropLast use only slice. In that second issue I don't see first and last playing any role?

[paldepind]

No, I missed that there were two parts and I only tested with the second code. I'll take a look at the other problem as well 😄

[emmanueltouzery]

yes unfortunately there are two bugs, I ran the test script from your PR branch:

FIRST ISSUE first() doesn't agree with nth(0)
204
204
SECOND ISSUE list of 4 drop first and last, wrong result
[ 201, 202, 203, 204 ]
[ 201, 202 ]

This kind of bugs seem like showstoppers to me :-(

[emmanueltouzery]

You didn't forget about other bug did you? Closing the bug makes it unclear

[paldepind]

You didn't forget about other bug did you? Closing the bug makes it unclear

Don't worry, I didn't 😄

I actually didn't close the issue myself. GitHub automatically closed the issue because I merged the PR #65. Since the PR contained the text "Fixes #64" GitHub thought the issue was fixed.

This kind of bugs seem like showstoppers to me :-(

I agree. List shouldn't have any bugs. See #66. That should fix the issue.

[emmanueltouzery]

Yes in general in the commits i say "ref #bug" if it's related to the bug without fixing it. Great otherwise! Hope you make a bugfix release soon :-)

[paldepind]

Yes in general in the commits i say "ref #bug" if it's related to the bug without fixing it.

That's a great tip!

Both issues should be fixed now so I'll close 😄

[emmanueltouzery]

Sure let's close. Just lobbying for a bugfix release when possible :-)

[paldepind]

Just lobbying for a bugfix release when possible :-)

I've just released a new version 👍