wrapAsNonEmpty functions

[serras]

The current behavior of the toNonEmpty functions is to create a copy of the given Iterable. This is the safest option, but sometimes brings additional useless copying. For example, if you just want to signal that a given immutable List is non-empty, you don't need such copy.

This PR introduces a family of wrapAsNonEmpty functions which avoid that copy. As the documentation states, the developer is then responsible from keeping the non-emptiness invariant. For example, if you wrap a MutableList and then clearAll, you'll end up with NonEmptyList with no elements. Unfortunately, this is a trade-off we need to make here, since the fact that MutableList extends List means that we cannot prohibit this scenario.

[tKe]

Is it worth including a @DelicateApi-style annotation for these to indicate the unsafe nature (especially given the names don't indicate they're potentially "unsafe" - footguns should ideally be hard to use accidentally)

Side note - any reason not to make the public NonEmptyList constructor take (E, Iterable<E>) rather than (E, List<E>)?

[kyay10]

Maybe an immutable-collections integration module next?

[github-actions]

Kover Report

File	Coverage [38.95%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/NonEmptyCollection.kt	9.09%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/NonEmptyList.kt	42.42%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/NonEmptySet.kt	36.17%

Total Project Coverage	45.57%

[serras]

Is it worth including a @DelicateApi-style annotation for these to indicate the unsafe nature.

I've added special variants which require opt-in for mutable versions.

Any reason not to make the public NonEmptyList constructor take (E, Iterable<E>) rather than (E, List<E>)?

We wanted the constructor to be minimally expensive. With an Iterable you always need a .toList(). It's also to keep full compatibility with the 1.x implementation.

[serras]

Maybe an immutable-collections integration module next?

I've actually been thinking about this, but I could not find anything specific that we can bring to the table in this case. With the new APIs in this PR you'll be able to do:

persistentListOf(1, 2).wrapAsNotEmptyListOrNull()

and use it as NonEmptyList. Are there most things needed here?

[hoc081098]

Nice🙏.

[tKe]

Is it worth including a @DelicateApi-style annotation for these to indicate the unsafe nature.

I've added special variants which require opt-in for mutable versions.

I think the unsafe nature extends to any List<E> given the list could be a view on a mutable list elsewhere which is where the defensive copying comes in. I think the wrapAsNonEmpty methods are useful for times close to creation of a list that you can guarantee (or accept the risk) it won't be mutated but otherwise we should push towards the safer copying toNonEmpty

Any reason not to make the public NonEmptyList constructor take (E, Iterable<E>) rather than (E, List<E>)?

We wanted the constructor to be minimally expensive. With an Iterable you always need a .toList(). It's also to keep full compatibility with the 1.x implementation.

Makes sense, at least when it wasn't a value class. Now we end up copying from tail as a collection anyway with listOf(head) + tail, so Iterable is likely just as performant (unless Collections.addAll has some optimisations for List+List) and would also allow for NonEmptyList(iter.next(), Iterable { iter }), avoiding one copy when working with an iterator.

[tKe]

Maybe an immutable-collections integration module next?

I've actually been thinking about this, but I could not find anything specific that we can bring to the table in this case. With the new APIs in this PR you'll be able to do:

persistentListOf(1, 2).wrapAsNotEmptyListOrNull()

and use it as NonEmptyList. Are there most things needed here?

If we ended up with all wrapAsNonEmpty having the OptIn, then an arrow integration that knows the list is truly immutable and therefore the wrapping is safe (given the non-empty checks performed). It could also provide zero-copy adapters back to PersistentList etc through "unsafe" unwrap functions (i.e. myNonEmptyList.toPersistentList() could operate directly on the wrapped List for checking receiver type and optimizing)

[serras]

There's actually a design which would allow us to have nicer integration with any List type, but unfortunately it's not backwards compatible.

value class NonEmpty<A, C : Collection<A>> internal constructor(
  public val elements: C
): Collection<A> by elements

typealias NonEmptyList<A> = NonEmpty<A, List<A>>
typealias NonEmptySet<A> = NonEmpty<A, Set<A>>
typealias NonEmptyPersistentList<A> = NonEmpty<A, PersistentList<A>>

fun <A, C: Collection<A>> C.wrapAsNonEmpty(): NonEmpty<A, C> {
  require(isNotEmpty())
  return NonEmpty(this)
}

With this approach you can always use .elements to get the exact type you gave at the beginning.

The other problem with that approach is that NonEmptyList<A> is not a list, but just a Collection. We can move the wrapper a bit down the hierarchy and say:

value class NonEmpty<A, C : List<A>>(val elements: C): List<A> by elements

typealias NonEmptyList<A> = NonEmpty<A, List<A>>
typealias NonEmptyPersistentList<A> = NonEmpty<A, PersistentList<A>>

Maybe we can introduce it as a new (experimental) module?

[kyay10]

It's not too bad with PersistentList because one can always call all.toPersistentList(), but that would be interesting. I think https://youtrack.jetbrains.com/issue/KT-51417/Restricted-types is likely the best solution, if it ever gets implemented.

[tKe]

I think my main concern is we have no way to control if the underlying elements is immutable or not, and any direct access may allow accidentally breaching the invariant. I see the wrapping (and unwrapping) as useful for list impls that we know will maintain the invariant (such as PersistentList), and should otherwise be hard for casual use and require explicit "I know what I'm doing here" OptIn in all cases. toNonEmptyX should continue to be the default.

Unwrapping shouldn't really be needed other than for optimisations such as PersistentList "mutations" referencing the original data structure - which is why pushing it behind an OptIn to mitigate casual use and providing an integration module that knows to convert a NonEmptyList to a PersistentList it can unwrap and if the wrapped list was a PersistentList it can safely return it directly as it cannot be mutated such that the originating NonEmptyList's invariant is broken.

I think allowing break-glass access to the underlying list is enough for these cases, and carrying the underlying list type via the NonEmpty<A, C> approach only complicates the type hierarchy (given we cannot do value class NonEmptyCollection<A, C: Collection<A>>(val elements: C): C by elements).

A (poor) example where the OptIn only for Mutable overrides breaks down:

fun handle(messages: List<String>) {
  dispatch(messages.wrapAsNonEmptyListOrNull() ?: return)
}
val collected = mutableListOf<String>("hello")
handle(collected)
collected.clear()

java.util.Arrays.ArrayList would be wrappable while maintaining the invariant of a non-empty-list (once created) despite not being immutable (it's size can never change, so will always be non-empty).