suspicious code in Array

[wishfoundry]

I noticed this line

var slot = new createNode(a.height - 1, 0);

in:
https://github.com/elm-lang/core/blob/master/src/Native/Array.js#L730

createNode() is the factory here, it should not be new'ed up itself

I'm entirely sure where this will manifest though. If the being appended are balanced it probably can't reach this code.

[process-bot]

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[process-bot]

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[wishfoundry]

actually this looks like the source of the problem in #474

[wishfoundry]

actually this looks like the source of the problem in #474

[turboMaCk]

+1

[turboMaCk]

+1

[mgold]

Great detective work, but it won't be needed: there's a fully-Elm version of Array in the final stages of testing.

[mgold]

Great detective work, but it won't be needed: there's a fully-Elm version of Array in the final stages of testing.

[evancz]

I'm interested in this fix in the meantime. That new stuff needs a performance review, and if this is an easy fix, it won't hurt!

So can someone do a PR with the change and a test that demonstrates what it fixes?

[evancz]

I'm interested in this fix in the meantime. That new stuff needs a performance review, and if this is an easy fix, it won't hurt!

So can someone do a PR with the change and a test that demonstrates what it fixes?

[lukewestby]

@wishfoundry how did you demonstrate that this is the cause of the issue in #474? i'm setting a breakpoint at that line while running the examples from that issue and the line in question isn't being reached.

while the use of new is semantically incorrect here, it doesn't change the behavior of pure functions like createNode.

[lukewestby]

@wishfoundry how did you demonstrate that this is the cause of the issue in #474? i'm setting a breakpoint at that line while running the examples from that issue and the line in question isn't being reached.

while the use of new is semantically incorrect here, it doesn't change the behavior of pure functions like createNode.

[wishfoundry]

@lukewestby newing up a function can prevent it from returning a value:

I discovered the bug while attempting to port the rrb code to plain js, during a battery of performance comps against other immutable libs.
The rrb version would fail on any tests with lengths > 32, < 1024. I removed the "new", and they ran again. but I'm still a little unclear the exact reason. but if there is still a future in the current Array code, I'll update here as soon as I get a few moment to rerun them

[wishfoundry]

@lukewestby newing up a function can prevent it from returning a value:

I discovered the bug while attempting to port the rrb code to plain js, during a battery of performance comps against other immutable libs.
The rrb version would fail on any tests with lengths > 32, < 1024. I removed the "new", and they ran again. but I'm still a little unclear the exact reason. but if there is still a future in the current Array code, I'll update here as soon as I get a few moment to rerun them

[wishfoundry]

Finally got it. the issue has to do with the final balancing act done in:

https://github.com/elm-lang/core/blob/master/src/Native/Array.js#L644

assumes that both are never leaves, which isn't always true due to RRBs unbalanced nature. We just need to add a node.height != 0 check in here. (PR to follow)

[wishfoundry]

Finally got it. the issue has to do with the final balancing act done in:

https://github.com/elm-lang/core/blob/master/src/Native/Array.js#L644

assumes that both are never leaves, which isn't always true due to RRBs unbalanced nature. We just need to add a node.height != 0 check in here. (PR to follow)

[wishfoundry]

after looking further through this code, I've come to realize it too many issues to quickly repair.

I'm going to see if I can help out with the new HAMT version instead of working on this.

[wishfoundry]

after looking further through this code, I've come to realize it too many issues to quickly repair.

I'm going to see if I can help out with the new HAMT version instead of working on this.