Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that all empty PersistentLists are the same instance. #91

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Ensure that all empty PersistentLists are the same instance. #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 58 additions & 19 deletions src/com/amazon/ionelement/api/Ion.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.amazon.ionelement.impl.TimestampElementImpl
import java.math.BigInteger
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.persistentListOf
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

Expand Down Expand Up @@ -124,7 +125,7 @@
metas: MetaContainer = emptyMetaContainer()
): StringElement = StringElementImpl(
value = s,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)
/** Creates a [StringElement] that represents an Ion `symbol`. */
Expand All @@ -141,7 +142,7 @@
metas: MetaContainer = emptyMetaContainer()
): SymbolElement = SymbolElementImpl(
value = s,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -159,7 +160,7 @@
metas: MetaContainer = emptyMetaContainer()
): TimestampElement = TimestampElementImpl(
timestampValue = Timestamp.valueOf(s),
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -177,7 +178,7 @@
metas: MetaContainer = emptyMetaContainer()
): TimestampElement = TimestampElementImpl(
timestampValue = timestamp,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -196,7 +197,7 @@
): IntElement =
LongIntElementImpl(
longValue = l,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -214,7 +215,7 @@
metas: MetaContainer = emptyMetaContainer()
): IntElement = BigIntIntElementImpl(
bigIntegerValue = bigInt,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -232,7 +233,7 @@
metas: MetaContainer = emptyMetaContainer()
): BoolElement = BoolElementImpl(
booleanValue = b,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -250,7 +251,7 @@
metas: MetaContainer = emptyMetaContainer()
): FloatElement = FloatElementImpl(
doubleValue = d,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -268,7 +269,7 @@
metas: MetaContainer = emptyMetaContainer()
): DecimalElement = DecimalElementImpl(
decimalValue = bigDecimal,
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -290,7 +291,7 @@
metas: MetaContainer = emptyMetaContainer()
): BlobElement = BlobElementImpl(
bytes = bytes.clone(),
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand Down Expand Up @@ -319,7 +320,7 @@
metas: MetaContainer = emptyMetaContainer()
): ClobElement = ClobElementImpl(
bytes = bytes.clone(),
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)
/**
Expand All @@ -343,8 +344,8 @@
metas: MetaContainer = emptyMetaContainer()
): ListElement =
ListElementImpl(
values = iterable.map { it.asAnyElement() }.toPersistentList(),
annotations = annotations.toPersistentList(),
values = iterable.mapToEmptyOrPersistentList { it.asAnyElement() },
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand Down Expand Up @@ -392,8 +393,8 @@
metas: MetaContainer = emptyMetaContainer()
): SexpElement =
SexpElementImpl(
values = iterable.map { it.asAnyElement() }.toPersistentList(),
annotations = annotations.toPersistentList(),
values = iterable.mapToEmptyOrPersistentList { it.asAnyElement() },
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand Down Expand Up @@ -434,8 +435,8 @@
metas: MetaContainer = emptyMetaContainer()
): StructElement =
StructElementImpl(
allFields = fields.toPersistentList(),
annotations = annotations.toPersistentList(),
allFields = fields.toEmptyOrPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas.toPersistentMap()
)

Expand All @@ -454,7 +455,7 @@
): StructElement =
ionStructOf(
fields = fields.asIterable(),
annotations = annotations.toPersistentList(),
annotations = annotations.toEmptyOrPersistentList(),
metas = metas
)

Expand Down Expand Up @@ -482,8 +483,46 @@
return ionStructOf(fields, annotations, metas)
}

/**
* Converts an `Iterable` to a `PersistentList`.
*
* ### Why is this needed?
* `kotlinx.collections.immutable` <= 0.3.7 has a bug that causes it to unnecessarily allocate empty `PersistentList`s
* instead of using a singleton instance. The fix is in
* [kotlinx.collections.immutable#176](https://github.com/Kotlin/kotlinx.collections.immutable/pull/176),
* but we cannot use it because the version of `kotlinx.collections.immutable` that will have (or has) the fix
* requires Kotlin stdlib >=1.9.2, and `ion-element-kotlin` supports consumers using Kotlin >= 1.3.0.
*/
internal fun <E> Iterable<E>.toEmptyOrPersistentList(): PersistentList<E> {
val isEmpty = if (this is Collection<*>) {
this.isEmpty()
} else {
!this.iterator().hasNext()
}
return if (isEmpty) EMPTY_PERSISTENT_LIST else toPersistentList()
}

/**
* Converts an `Iterable` to a `PersistentList`, transforming each element using [block].
*
* ### Why is this needed?
* `kotlinx.collections.immutable` <= 0.3.7 has a bug that causes it to unnecessarily allocate empty `PersistentList`s
* instead of using a singleton instance. The fix is in
* [kotlinx.collections.immutable#176](https://github.com/Kotlin/kotlinx.collections.immutable/pull/176),
* but we cannot use it because the version of `kotlinx.collections.immutable` that will have (or has) the fix
* requires Kotlin stdlib >=1.9.2, and `ion-element-kotlin` supports consumers using Kotlin >= 1.3.0.
*/
internal inline fun <T, R> Iterable<T>.mapToEmptyOrPersistentList(block: (T) -> R): PersistentList<R> {
val isEmpty = if (this is Collection<*>) {
this.isEmpty()

Check warning on line 517 in src/com/amazon/ionelement/api/Ion.kt

View check run for this annotation

Codecov / codecov/patch

src/com/amazon/ionelement/api/Ion.kt#L517 

Added line #L517 was not covered by tests
} else {
!this.iterator().hasNext()
}
return if (isEmpty) EMPTY_PERSISTENT_LIST else mapTo(persistentListOf<R>().builder(), block).build()
}

// Memoized empty PersistentList for the memoized container types and null values
private val EMPTY_PERSISTENT_LIST: PersistentList<Nothing> = emptyList<Nothing>().toPersistentList()
internal val EMPTY_PERSISTENT_LIST: PersistentList<Nothing> = persistentListOf()

// Memoized empty instances of our container types.
private val EMPTY_LIST = ListElementImpl(EMPTY_PERSISTENT_LIST, EMPTY_PERSISTENT_LIST, EMPTY_METAS)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/BigIntIntElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import com.amazon.ionelement.api.PersistentMetaContainer
import com.amazon.ionelement.api.constraintError
import java.math.BigInteger
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class BigIntIntElementImpl(
Expand All @@ -42,7 +41,7 @@ internal class BigIntIntElementImpl(
}

override fun copy(annotations: List<String>, metas: MetaContainer): BigIntIntElementImpl =
BigIntIntElementImpl(bigIntegerValue, annotations.toPersistentList(), metas.toPersistentMap())
BigIntIntElementImpl(bigIntegerValue, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): BigIntIntElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): BigIntIntElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/BlobElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class BlobElementImpl(
Expand All @@ -33,7 +32,7 @@ internal class BlobElementImpl(
override fun writeContentTo(writer: IonWriter) = writer.writeBlob(bytes)

override fun copy(annotations: List<String>, metas: MetaContainer): BlobElementImpl =
BlobElementImpl(bytes, annotations.toPersistentList(), metas.toPersistentMap())
BlobElementImpl(bytes, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): BlobElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): BlobElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/BoolElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class BoolElementImpl(
Expand All @@ -30,7 +29,7 @@ internal class BoolElementImpl(
override val type: ElementType get() = ElementType.BOOL

override fun copy(annotations: List<String>, metas: MetaContainer): BoolElementImpl =
BoolElementImpl(booleanValue, annotations.toPersistentList(), metas.toPersistentMap())
BoolElementImpl(booleanValue, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): BoolElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): BoolElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/ClobElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class ClobElementImpl(
Expand All @@ -32,7 +31,7 @@ internal class ClobElementImpl(

override fun writeContentTo(writer: IonWriter) = writer.writeClob(bytes)
override fun copy(annotations: List<String>, metas: MetaContainer): ClobElementImpl =
ClobElementImpl(bytes, annotations.toPersistentList(), metas.toPersistentMap())
ClobElementImpl(bytes, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): ClobElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): ClobElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/DecimalElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class DecimalElementImpl(
Expand All @@ -31,7 +30,7 @@ internal class DecimalElementImpl(
override val type get() = ElementType.DECIMAL

override fun copy(annotations: List<String>, metas: MetaContainer): DecimalElementImpl =
DecimalElementImpl(decimalValue, annotations.toPersistentList(), metas.toPersistentMap())
DecimalElementImpl(decimalValue, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): DecimalElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): DecimalElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/FloatElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class FloatElementImpl(
Expand All @@ -30,7 +29,7 @@ internal class FloatElementImpl(
override val type: ElementType get() = ElementType.FLOAT

override fun copy(annotations: List<String>, metas: MetaContainer): FloatElementImpl =
FloatElementImpl(doubleValue, annotations.toPersistentList(), metas.toPersistentMap())
FloatElementImpl(doubleValue, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): FloatElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): FloatElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/ListElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package com.amazon.ionelement.impl
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class ListElementImpl(
Expand All @@ -31,7 +30,7 @@ internal class ListElementImpl(
override val listValues: List<AnyElement> get() = values

override fun copy(annotations: List<String>, metas: MetaContainer): ListElementImpl =
ListElementImpl(values, annotations.toPersistentList(), metas.toPersistentMap())
ListElementImpl(values, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): ListElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): ListElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/LongIntElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import java.math.BigInteger
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class LongIntElementImpl(
Expand All @@ -34,7 +33,7 @@ internal class LongIntElementImpl(
override val bigIntegerValue: BigInteger get() = BigInteger.valueOf(longValue)

override fun copy(annotations: List<String>, metas: MetaContainer): LongIntElementImpl =
LongIntElementImpl(longValue, annotations.toPersistentList(), metas.toPersistentMap())
LongIntElementImpl(longValue, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): LongIntElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): LongIntElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/NullElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class NullElementImpl(
Expand All @@ -31,7 +30,7 @@ internal class NullElementImpl(
override val isNull: Boolean get() = true

override fun copy(annotations: List<String>, metas: MetaContainer): AnyElement =
NullElementImpl(type, annotations.toPersistentList(), metas.toPersistentMap())
NullElementImpl(type, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): AnyElement = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): AnyElement = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/SexpElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package com.amazon.ionelement.impl
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class SexpElementImpl(
Expand All @@ -31,7 +30,7 @@ internal class SexpElementImpl(
override val sexpValues: List<AnyElement> get() = seqValues

override fun copy(annotations: List<String>, metas: MetaContainer): SexpElementImpl =
SexpElementImpl(values, annotations.toPersistentList(), metas.toPersistentMap())
SexpElementImpl(values, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): SexpElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): SexpElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
3 changes: 1 addition & 2 deletions src/com/amazon/ionelement/impl/StringElementImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import com.amazon.ion.IonWriter
import com.amazon.ionelement.api.*
import com.amazon.ionelement.api.PersistentMetaContainer
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.collections.immutable.toPersistentMap

internal class StringElementImpl(
Expand All @@ -32,7 +31,7 @@ internal class StringElementImpl(
override val stringValue: String get() = textValue

override fun copy(annotations: List<String>, metas: MetaContainer): StringElementImpl =
StringElementImpl(textValue, annotations.toPersistentList(), metas.toPersistentMap())
StringElementImpl(textValue, annotations.toEmptyOrPersistentList(), metas.toPersistentMap())

override fun withAnnotations(vararg additionalAnnotations: String): StringElementImpl = _withAnnotations(*additionalAnnotations)
override fun withAnnotations(additionalAnnotations: Iterable<String>): StringElementImpl = _withAnnotations(additionalAnnotations)
Expand Down
Loading
Loading