search

Kotlin / kotlinx.collections.immutable

Immutable persistent collections for Kotlin
Apache License 2.0
1.16k stars 59 forks source link

Fix for #109 #111

Closed belyaev-mikhail closed 3 years ago

belyaev-mikhail commented 3 years ago

Github does not allow me to reopen #110, so a new PR

Comments from #110:

Seems like some logic here is broken: mutableUpdateNodeAtIndex expects newNode to be newly-created, but mutableReplaceNode does not check for it properly (newNode may be equal to targetNode, which may be any node of any builder previously created)

Maybe the idea was to check newNode.ownedBy instead of ownedBy? Anyway, removing the check fixes the test.

I'd love to provide a regression test, but have no idea how to reproduce this outside the node implementation. Even random test triggers this exclusively on JVM, because JVM implementation of these tests (and no other platform) actually calls builder's remove(key, value) method due to overloading witchcrafting, no other tests seem to call it under such stressful conditions.

qurbonzoda commented 3 years ago

An assert(newNode.ownedBy === owner) // newNode is newly created by this builder can also be added to check the assumption that if newNode is not the targetNode, than it must have been newly created.

belyaev-mikhail commented 3 years ago

Rebased onto master + regression test (lousy naming) + assertion

qurbonzoda commented 3 years ago

Thank you!