Arrow: Bug: NonEmptyList derived directly from MutableList propagates changes to #all property, but not #size, #head and #tail

[billwanjohi]

I created a NonEmptyList by passing a standard Kotlin MutableList to it. I had assumed that this new variable was fully immutable, implicitly making a copy of the mutable list and severing any connection to it. Instead, what I found is that it continues to reference the mutable list for at least one property (all), but not for others.

val mutableList = mutableListOf(5, 8, 13)
NonEmptyList.fromList(mutableList).map { nonEmptyList ->
    assert(nonEmptyList.all == listOf(5, 8, 13))
    mutableList.clear()
    assert(nonEmptyList.all == emptyList<Int>())  // Yikes
    assert(nonEmptyList.size == 3)
    assert(nonEmptyList.head == 5)
    assert(nonEmptyList.tail == listOf(8, 13))
}                                                       

My workaround is to call #toList on the MutableList first, but hopefully you agree that this is a bug, and the mutable list should either not be rejected or have consistent effects in all properties and elements. As it is, it makes NonEmptyList more dangerous to use in my context than the theoretically looser Kotlin List.

I'm running arrow v0.8.2

[pakoito]

Can you please add a use of toList() in the constructors here to fix this problem? https://github.com/arrow-kt/arrow/blob/master/modules/core/arrow-extras/src/main/kotlin/arrow/data/NonEmptyList.kt#L19-L20

[pablisco]

We could use NonEmptyList internally with a terminal case for a singleton. As well as a special case for lazy copy of the list to support the current behaviour too and avoid eager (which could verify that we don't send in a MutableList).

[osquitoOfTheNorth]

Are you still looking for help on this? Fairly new to open source and looking to contribute for the first time.

[billwanjohi]

@osquitoOfTheNorth I started to solve this, but got distracted before I could write any tests. If you wanted to extend the PR I just opened, or take it in another direction, I'm sure people would appreciate it. As it is, the class is a bit dangerous to use.

[pablisco]

I also started looking into this and got distracted 😅
Also, wasn't sure how to balance having something efficient vs more pure FP 🤔