Support null elements in sets

[jcornaz]

Hello,

I noticed that sets don't support null elements.

java.lang.NullPointerException
	at com.github.andrewoma.dexx.collection.internal.hashmap.CompactHashMap.put(CompactHashMap.java:69)
	at com.github.andrewoma.dexx.collection.HashSet.add(HashSet.java:88)
	at com.github.andrewoma.dexx.collection.HashSet.add(HashSet.java:36)
	[...]

java.lang.NullPointerException
	at com.github.andrewoma.dexx.collection.internal.hashmap.CompactHashMap.put(CompactHashMap.java:69)
	at com.github.andrewoma.dexx.collection.HashSet.add(HashSet.java:88)
	at com.github.andrewoma.dexx.collection.Sets.copyOf(Sets.java:133)
	[...]

As there is no @NotNull annotation for it, I assume it is a bug.

[jcornaz]

I see that this project provide first class support of Kotlin.

So I would like to argue that when using the module kollection from Kotlin the following code, should either not compile or work.

fun main(args: Array<String>) {
  val set1: Set<Int?> = Sets.of(1, 2, null, 3)
  val set2: Set<Int?> = set1.add(null)
  set2.forEach { println(it) }
}

And because, it compiles it should work.

Exception in thread "main" java.lang.NullPointerException
	at com.github.andrewoma.dexx.collection.internal.hashmap.CompactHashMap.elemHashCode(CompactHashMap.java:78)
	at com.github.andrewoma.dexx.collection.internal.hashmap.CompactHashMap.computeHash(CompactHashMap.java:89)
	at com.github.andrewoma.dexx.collection.internal.hashmap.CompactHashMap.put(CompactHashMap.java:70)
	at com.github.andrewoma.dexx.collection.HashSet.add(HashSet.java:88)
	at com.github.andrewoma.dexx.collection.HashSet.add(HashSet.java:36)
	at com.github.andrewoma.dexx.collection.Sets.construct(Sets.java:124)
	at com.github.andrewoma.dexx.collection.Sets.of(Sets.java:72)
	at MainKt.main(Main.kt:4)

[andrewoma]

It's more a limitation at the moment. There's no reason it can't be made to support nulls.

Although it does create a few logical issues around things like SortedSet - should a null come first or last?

[jcornaz]

It's more a limitation at the moment. There's no reason it can't be made to support nulls.

Sure. But I think from Kotlin it should not compile then. Instead of having a null pointer exception at runtime. (can be done by adding the @NotNull annotation)

Although it does create a few logical issues around things like SortedSet - should a null come first or last?

I don't see the problem here. First null is not an instance of Comparable. Second, in case of sorted collections, the collection should rely on a comparator and don't care about null elements or any special sorting the user would want.

Example of factory functions:

/** Here elements can be null. But the user have to give a comparator capable handling the elements (which means the user take care of null if the elements are nullable) */
fun <E> emptySortedSet(comparator: Comparator<E>): Set<E> = TODO()

/** Here the user doesn't have to provide a comparator, but elements have to be comparable. And `null` is not comparable */
fun <E : Comparable<E>> emptySortedSet(): Set<E> = emptySortedSet(compareBy<E> { it })

And the user can do:

// Here I chose to put null first by default, because it is the behavior of the standard kotlin library.
// But there could be no default, and force the user to choose.
fun <E : Comparable<E>?> comparator(nullFirst: Boolean = true): Comparator<E?> = Comparator { o1, o2 ->
  when {
    o1 === o2 -> 0 // The compared object have the same reference (either null or not)
    o1 === null -> if (nullFirst) -1 else 1
    o2 === null -> if (nullFirst) 1 else -1
    else -> o1.compareTo(o2)
  }
}

fun main(args: Array<String>) {
  val nonNullElementSortedSet = emptySortedSet<Int>()
  val nullsFirstSortedSet = emptySortedSet<Int?>(comparator(true))
  val nullsLastSortedSet = emptySortedSet<Int?>(comparator(false))
}

[jcornaz]

Actually dexx already provide:

fun <E : Any> immutableCustomSortedSetOf(selector: (E) -> Comparable<*>?, vararg elements: E): ImmutableSet<E>
fun <E : Any> Iterable<E>.toImmutableSortedSet(selector: (E) -> Comparable<*>?): ImmutableSet<E>
fun <E : Any> Sequence<E>.toImmutableSortedSet(selector: (E) -> Comparable<*>?): ImmutableSet<E>

Beside the naming inconsistency (one is named "custom" but not the others), Dexx actually already support comparison for nullable comparable and answered the question: "should a null come first or last?" by delegating it to kotlin standard library (it is null comes first).