Consistency issues with equality in list-like NbtTag types

[BenWoodworth]

History

Currently (v0.11), the NbtList/Array classes all implement the List interface, following the example that kotlinx-serialization-json set with JsonArray (implementing List for convenience).
https://github.com/Kotlin/kotlinx.serialization/blob/8a2c1c0e05ac9c77746141837f6d53d923e24d8a/formats/json/commonMain/src/kotlinx/serialization/json/JsonElement.kt#L191-L195

Its equals implementation simply compares the content, following the contract for list equality. From the List.equals() javadoc:

...two lists are defined to be equal if they contain the same elements in the same order. This definition ensures that the equals method works properly across different implementations of the List interface.

However, there's a problem that NBT is faced with, having four list-like types. Comparing NbtList == NbtIntArray, you would always expect that to be false, as they represent two different serialized forms. But, according to the List contract, an empty NbtList should equal an empty NbtIntArray, since they have the same content.

In v0.11, this is worked around by special-casing NbtTag comparisons, and only checking for the same NbtTag type if both are NbtTags. E.g. NbtList.equals():

knbt/src/commonMain/kotlin/NbtTag.kt

Lines 125 to 129 in dba146f

	override fun equals(other: Any?): Boolean = when {
	this === other -> true
	other is NbtTag -> other is NbtByteArray && content.contentEquals(other.content)
	else -> list == other
	}

So, for empty list/nbtList/nbtIntArrays:

• The List contract is satisfied for non-NBT types: list == nbtList and list == nbtIntArray
• And content-equal NBT tags of different types are unequal: nbtList != nbtIntArray

The remaining problem

This approach satisfies all of the requirements in the Any.equals() contract, except transitive:

• Reflexive: for any non-null value x, x.equals(x) should return true.
• Symmetric: for any non-null values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.
• Transitive: for any non-null values x, y, and z, if x.equals(y) returns true and y.equals(z) returns true, then x.equals(z) should return true.
• Consistent: for any non-null values x and y, multiple invocations of x.equals(y) consistently return true or consistently return false, provided no information used in equals comparisons on the objects is modified.
• Never equal to null: for any non-null value x, x.equals(null) should return false.

Example:

• nbtList == list, and
• list == NbtIntArray, so ideally
• nbtList == nbtIntArray, but it doesn't.

Potential solutions

• Have the list-like NBT types implement Collection instead (or nothing at all, similar to Kotlin's Array classes)
  • Pros:
    • Unbeholden to the List equality contract
    • Allows for more NBT-specific collection semantics (e.g. how empty TAG_Lists can have a different element type encoded)
  • Cons: (inconvenience, which can be mostly alleviated by NbtList/Nbt*Array.asList() extensions)
    • Can't directly use as Lists
    • Lose access to some of Kotlin's list-specific stdlib extensions
    • May have to add member functions to mirror the methods in List
    • Have to consider making NbtCompound not implement Map for similar reasons
• Add an equals(NbtTag?) overload to NbtTag
  • Pros:
    • Can retain the convenience of the types implementing List
  • Cons:
    • May not be possible to work with kotlin's Any?.equals() stdlib extension (since a similar NbtTag?.equals() would need to be explicitly imported, and that seems like a potentially major source of issue)
    • Overloading equals in general feels like a design smell, since values are either equal or they aren't, and it doesn't seem like something the caller should be concerned with
• Leave the implementation how it is, accepting the broken transitivity requirement
  • Pros:
    • Keeps the same convenience
  • Cons:
    • Breaks assumptions made by List consumers, albeit in potentially uncommon cases

Possible conclusion

After having thought on this for admittedly way too much time, I'm leaning towards the first option and implementing nothing for the list-like types (and NbtCompound as well for consistency), leaving richer functionality to the kotlin stdlib List/Maps, and bridging the gap in convenience with asList() and asMap() extensions. Plus adding basic members like size and get() that are often used at the deserialization boundary.