Kotlin / kotlinx.collections.immutable

Immutable persistent collections for Kotlin
Apache License 2.0
1.12k stars 56 forks source link

Missing empty constructors #140

Open desgraci opened 1 year ago

desgraci commented 1 year ago

Can we have something like:

private const val INVALID_INDEX = -1

fun <T> emptyImmutableList(): ImmutableList<T> = EmptyImmutableList

internal object EmptyImmutableIterator : ListIterator<Nothing> {
    override fun hasNext(): Boolean = false
    override fun hasPrevious(): Boolean = false
    override fun nextIndex(): Int = 0
    override fun previousIndex(): Int = INVALID_INDEX
    override fun next(): Nothing = throw NoSuchElementException()
    override fun previous(): Nothing = throw NoSuchElementException()
}

@kotlinx.serialization.Serializable
internal object EmptyImmutableList : ImmutableList<Nothing>, RandomAccess {
    override val size: Int get() = 0
    override fun isEmpty(): Boolean = true

    override fun contains(element: Nothing): Boolean = false
    override fun containsAll(elements: Collection<Nothing>): Boolean = false
    override fun get(index: Int): Nothing =
        throw IndexOutOfBoundsException("Empty immutable list doesn't contain element at index $index.")

    override fun indexOf(element: Nothing): Int = INVALID_INDEX
    override fun lastIndexOf(element: Nothing): Int = INVALID_INDEX

    override fun iterator(): Iterator<Nothing> = EmptyImmutableIterator
    override fun listIterator(): ListIterator<Nothing> = EmptyImmutableIterator
    override fun listIterator(index: Int): ListIterator<Nothing> = EmptyImmutableIterator
}
qwwdfsad commented 1 year ago

Could you please elaborate on why you need a separate implementation though?

desgraci commented 1 year ago

I can think quickly on 3 reasons:

  1. I want to create as few instances as I can, e.g. emptyImmutableList <Int>() === emptyImmutableList<Double>(), will be still the same.
  2. I don't wanna use e.g. persistantListOf, cause I could be using a ImmutableListAdapter or any other implementation of the ImmutableList in this example
  3. This is the standard in kotlin as they work the emptySet, emptyList, etc...
  4. This is a personal opinion, emptyImmutableList reads, for me, much clearer on my intention than persistenListOf()

Actually, I would be interested in reading your thoughts on which approach would you take and which advantages? Problably there is something really nice that I'm missing or could be improved on my side.

dimsuz commented 1 year ago

I second what is said in 3.

As a new user of this library, the first thing I intuitively typed when I wanted to create an empty immutable list was "empty", as in stdlib collections.

When no completions appeared, I had to find (and subscribe to) this issue to actually discover how to properly create an empty list.

mcpiroman commented 1 year ago

I think @qwwdfsad's point is that the impl could be simply

fun <T> emptyImmutableList(): ImmutableList<T> = EmptyImmutableList

internal val EmptyImmutableList = persistentListOf()

unless that's what you say in 2. which I don't really understand.


But here's another thing:

It's possible that in the future (about that of valhalla) persistent collections will be made value classes which lack identity (so you can't do === on them) and so persistenListOf() would then behave the exact same as emptyImmutableList(). Having two ways of doing the same thing is IMHO not nice either.

desgraci commented 1 year ago

I think @qwwdfsad's point is that the impl could be simply

fun <T> emptyImmutableList(): ImmutableList<T> = EmptyImmutableList

internal val EmptyImmutableList = persistentListOf()

unless that's what you say in 2. which I don't really understand.

But here's another thing:

It's possible that in the future (about that of valhalla) persistent collections will be made value classes which lack identity (so you can't do === on them) and so persistenListOf() would then behave the exact same as emptyImmutableList(). Having two ways of doing the same thing is IMHO not nice either.

What I meant with point 2, is that I will loose the type, in case I had something mutable passed, similar to what we have in the List/MutableList. i.e. I cannot cast to PersistantList but with your approach I could. Something like

        val emptyMutableList: ImmutableList<String> = emptyImmutableList()
        val emptyMutableList2: ImmutableList<String> = persistentListOf()
        val testA = (emptyMutableList as? PersistentList)     // null
        val testB = (emptyMutableList2 as? PersistentList)  // Persistant, or SmthVector, I think

I think then that your suggestion in on point, cause unlike the old traditional lists, I could do a successful cast to PersistedList and still won't be able to modify (correct me if I'm wrong), cause the add here returns a new instance, unlike the List/MutableList case, which is what I had in mind to prevent, didn't notice!

So, about in the future, I wasn't aware of the migration to value classes, I usually have them for primitives, dunno how will it work, but in that case the persistentListOf will become quite the equivalent of this snippet for those reasons, but again, I coded with what I knew last week, still reading this topic and this was one of the first questions that I had, cause at the current state is not the same.

Anyway, I think we can go with persistentListOf() when that happens then cause it will invalidate point 1, feel free to resolve if this makes sense as we won't have any benefits in the future besides the wording.

EDIT: about 3/4, btw, I still think is what a lot of people like me will try to find before having to understand the mechanism behind this, I'm fine with switching back, but maybe something to consider for the new people jumping on the topic to ease the learning process.

mcpiroman commented 1 year ago

I wasn't aware of the migration to value classes

I'm neither, I only expect this may be sensible to do, at least for some classes like SmallPersistentVector, but not necessarily.

As to point 2. yes, it will be safe, even more than stdlib, as all implementations are immutable. Even if they weren't they are internal so you can't cast to them anyway.

The problem of discoverability remains though, so it may be sensible to have emptyCollection functions even just for that. Maybe make them instantly deprecated pointing to persistentXOf()?

desgraci commented 1 year ago

I wasn't aware of the migration to value classes

I'm neither, I only expect this may be sensible to do, at least for some classes like SmallPersistentVector, but not necessarily.

As to point 2. yes, it will be safe, even more than stdlib, as all implementations are immutable. Even if they weren't they are internal so you can't cast to them anyway.

The problem of discoverability remains though, so it may be sensible to have emptyCollection functions even just for that. Maybe make them instantly deprecated pointing to persistentXOf()?

Ok then I will keep my original suggestion, because 1st. if it is not know if the value is happening or not, I was assuming this was a fact, then creating instances won't be ideal, second, not all the implementations of ImmutableList are internal, you can have the adapters, and finally the fact of this being the standard on kotlin for empty collections remains.