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

Convert public API lists to arrays #1395

Draft
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Draft

Convert public API lists to arrays #1395

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3956398
Convert public API list properties to arrays
drewhamilton Jul 28, 2023
394aa84
Update consumers of changed classes
drewhamilton Aug 7, 2023
73ef326
Add EventSerializer to handle empty arrays
drewhamilton Aug 8, 2023
b3680f1
Fix JS reflection error
drewhamilton Aug 9, 2023
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
2 changes: 1 addition & 1 deletion ...compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolWidgetChildren.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ internal class ProtocolWidgetChildren(

override fun remove(index: Int, count: Int) {
val removingIds = ids.subList(index, index + count)
val removedIds = removingIds.toList()
val removedIds = removingIds.toTypedArray()
removingIds.clear()

for (removedId in removedIds) {
Expand Down
6 changes: 3 additions & 3 deletions ...se/src/commonTest/kotlin/app/cash/redwood/protocol/compose/GeneratedProtocolBridgeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class GeneratedProtocolBridgeTest {
Create(Id(1), WidgetTag(4)),
ModifierChange(
Id(1),
listOf(
arrayOf(
ModifierElement(
ModifierTag(3),
buildJsonObject {
Expand Down Expand Up @@ -106,7 +106,7 @@ class GeneratedProtocolBridgeTest {
Create(Id(1), WidgetTag(4)),
ModifierChange(
Id(1),
listOf(
arrayOf(
ModifierElement(
ModifierTag(5),
buildJsonObject {
Expand Down Expand Up @@ -135,7 +135,7 @@ class GeneratedProtocolBridgeTest {
argument = it
}

protocolWidget.sendEvent(Event(Id(1), EventTag(4), listOf(JsonPrimitive("PT10S"))))
protocolWidget.sendEvent(Event(Id(1), EventTag(4), arrayOf(JsonPrimitive("PT10S"))))

assertThat(argument).isEqualTo(10.seconds)
}
Expand Down
4 changes: 2 additions & 2 deletions ...-protocol-widget/src/commonMain/kotlin/app/cash/redwood/protocol/widget/ProtocolBridge.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ public class ProtocolBridge<W : Any>(
private class RootProtocolNode<W : Any>(
private val children: Widget.Children<W>,
) : ProtocolNode<W>, Widget<W> {
override fun updateModifier(elements: List<ModifierElement>) {
throw AssertionError("unexpected: $elements")
override fun updateModifier(elements: Array<ModifierElement>) {
throw AssertionError("unexpected: ${elements.contentToString()}")
}

override fun apply(change: PropertyChange, eventSink: EventSink) {
Expand Down
2 changes: 1 addition & 1 deletion ...od-protocol-widget/src/commonMain/kotlin/app/cash/redwood/protocol/widget/ProtocolNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public interface ProtocolNode<W : Any> {

public fun apply(change: PropertyChange, eventSink: EventSink)

public fun updateModifier(elements: List<ModifierElement>)
public fun updateModifier(elements: Array<ModifierElement>)

/**
* Return one of this node's children groups by its [tag].
Expand Down
2 changes: 1 addition & 1 deletion ...tocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/ProtocolBridgeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class ProtocolBridgeTest {
tag = ChildrenTag.Root,
index = 0,
count = 1,
removedIds = listOf(Id(1)),
removedIds = arrayOf(Id(1)),
),
),
)
Expand Down
10 changes: 5 additions & 5 deletions ...-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/ProtocolNodeFactoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class ProtocolNodeFactoryTest {
val textInput = factory.create(WidgetTag(5))!!

textInput.updateModifier(
listOf(
arrayOf(
ModifierElement(
tag = ModifierTag(3),
value = buildJsonObject {
Expand Down Expand Up @@ -132,7 +132,7 @@ class ProtocolNodeFactoryTest {
val textInput = factory.create(WidgetTag(5))!!

textInput.updateModifier(
listOf(
arrayOf(
ModifierElement(
tag = ModifierTag(5),
value = buildJsonObject {
Expand Down Expand Up @@ -164,7 +164,7 @@ class ProtocolNodeFactoryTest {

val t = assertFailsWith<IllegalArgumentException> {
button.updateModifier(
listOf(
arrayOf(
ModifierElement(
tag = ModifierTag(345432),
value = JsonObject(mapOf()),
Expand Down Expand Up @@ -197,7 +197,7 @@ class ProtocolNodeFactoryTest {

val textInput = factory.create(WidgetTag(5))!!
textInput.updateModifier(
listOf(
arrayOf(
ModifierElement(
tag = ModifierTag(345432),
value = buildJsonArray {
Expand Down Expand Up @@ -342,7 +342,7 @@ class ProtocolNodeFactoryTest {
recordingTextInput.onChangeCustomType!!.invoke(10.seconds)

assertThat(eventSink.events.single())
.isEqualTo(Event(Id(1), EventTag(4), listOf(JsonPrimitive("PT10S"))))
.isEqualTo(Event(Id(1), EventTag(4), arrayOf(JsonPrimitive("PT10S"))))
}

class RecordingTextInput : TextInput<Nothing> {
Expand Down
70 changes: 66 additions & 4 deletions redwood-protocol/src/commonMain/kotlin/app/cash/redwood/protocol/protocol.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,21 @@
*/
package app.cash.redwood.protocol

import dev.drewhamilton.poko.ArrayContentBased
import dev.drewhamilton.poko.ArrayContentSupport
import dev.drewhamilton.poko.Poko
import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.KSerializer
import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable
import kotlinx.serialization.builtins.ArraySerializer
import kotlinx.serialization.descriptors.SerialDescriptor
import kotlinx.serialization.descriptors.buildClassSerialDescriptor
import kotlinx.serialization.descriptors.element
import kotlinx.serialization.encoding.Decoder
import kotlinx.serialization.encoding.Encoder
import kotlinx.serialization.encoding.decodeStructure
import kotlinx.serialization.encoding.encodeStructure
import kotlinx.serialization.json.JsonDecoder
import kotlinx.serialization.json.JsonElement
import kotlinx.serialization.json.JsonEncoder
Expand All @@ -32,16 +39,69 @@ import kotlinx.serialization.json.buildJsonArray
import kotlinx.serialization.json.jsonArray
import kotlinx.serialization.json.jsonPrimitive

@Serializable
@OptIn(ArrayContentSupport::class)
@Serializable(EventSerializer::class)
@Poko
public class Event(
/** Identifier for the widget from which this event originated. */
public val id: Id,
/** Identifies which event occurred on the widget with [id]. */
public val tag: EventTag,
public val args: List<JsonElement> = emptyList(),
@ArrayContentBased public val args: Array<JsonElement> = emptyArray(),
)

private object EventSerializer : KSerializer<Event> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah this is unfortunate. I initially envisioned that we would be able to do this with a property-local serializer. But I guess that's too late because the enclosing serializer for the type is what determines whether or not the value needs to be included. Once we're inside a serializer for the property we are always going to write out the value.

I'll look into this more. Maybe ask the serialization folks if they'd be open to a similar annotation to treat arrays as values and not references.

Another potential option in the future is to use a value class when custom equals gets supported. Then we could have a ValueBasedArray wrapper around an array. This assumes that it doesn't somehow incur boxing, which is not yet clear since there's no implementation of this to play with yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially envisioned that we would be able to do this with a property-local serializer. But I guess that's too late because the enclosing serializer for the type is what determines whether or not the value needs to be included.

Yep, that appears to be the case.

I filed Kotlin/kotlinx.serialization#2409, let's see what they think.

private val argsSerializer: KSerializer<Array<JsonElement>> =
@OptIn(ExperimentalSerializationApi::class)
ArraySerializer(JsonElement.serializer())

override val descriptor: SerialDescriptor = buildClassSerialDescriptor(
serialName = Event::class.qualifiedName!!,
) {
element<Id>("id")
element<EventTag>("tag")
element(
elementName = "args",
descriptor = argsSerializer.descriptor,
isOptional = true,
)
}

override fun deserialize(decoder: Decoder): Event {
return decoder.decodeStructure(descriptor) {
var id: Id? = null
var tag: EventTag? = null
var args: Array<JsonElement> = emptyArray()

var nextIndex = decodeElementIndex(descriptor)
while (nextIndex >= 0) {
when (nextIndex) {
0 -> id = decodeSerializableElement(descriptor, 0, Id.serializer())
1 -> tag = decodeSerializableElement(descriptor, 1, EventTag.serializer())
2 -> args = decodeSerializableElement(descriptor, 2, argsSerializer)
else -> throw IllegalStateException("Invalid index $nextIndex")
}
nextIndex = decodeElementIndex(descriptor)
}
Event(
id = checkNotNull(id),
tag = checkNotNull(tag),
args = args,
)
}
}

override fun serialize(encoder: Encoder, value: Event) {
encoder.encodeStructure(descriptor) {
encodeSerializableElement(descriptor, 0, Id.serializer(), value.id)
encodeSerializableElement(descriptor, 1, EventTag.serializer(), value.tag)
if (value.args.isNotEmpty()) {
encodeSerializableElement(descriptor, 2, argsSerializer, value.args)
}
}
}
}

@Serializable
public sealed interface Change {
/** Identifier for the widget which is the subject of this change. */
Expand All @@ -68,12 +128,13 @@ public class PropertyChange(
public val value: JsonElement = JsonNull,
) : ValueChange

@OptIn(ArrayContentSupport::class)
@Serializable
@SerialName("modifier")
@Poko
public class ModifierChange(
override val id: Id,
public val elements: List<ModifierElement> = emptyList(),
@ArrayContentBased public val elements: Array<ModifierElement> = emptyArray(),
) : ValueChange

@Serializable(with = ModifierElementSerializer::class)
Expand Down Expand Up @@ -143,6 +204,7 @@ public sealed interface ChildrenChange : Change {
public val count: Int,
) : ChildrenChange

@OptIn(ArrayContentSupport::class)
@Serializable
@SerialName("remove")
@Poko
Expand All @@ -151,7 +213,7 @@ public sealed interface ChildrenChange : Change {
override val tag: ChildrenTag,
public val index: Int,
public val count: Int,
public val removedIds: List<Id>,
@ArrayContentBased public val removedIds: Array<Id>,
) : ChildrenChange {
init {
require(count == removedIds.size) {
Expand Down
10 changes: 5 additions & 5 deletions redwood-protocol/src/commonTest/kotlin/app/cash/redwood/protocol/ProtocolTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ class ProtocolTest {
}

@Test fun eventNonEmptyArgs() {
val model = Event(Id(1), EventTag(2), listOf(JsonPrimitive("Hello"), JsonPrimitive(2)))
val model = Event(Id(1), EventTag(2), arrayOf(JsonPrimitive("Hello"), JsonPrimitive(2)))
val json = """{"id":1,"tag":2,"args":["Hello",2]}"""
assertJsonRoundtrip(Event.serializer(), model, json)
}

@Test fun eventEmptyArgs() {
val model = Event(Id(1), EventTag(2), listOf())
val model = Event(Id(1), EventTag(2), arrayOf())
val json = """{"id":1,"tag":2}"""
assertJsonRoundtrip(Event.serializer(), model, json)
}
Expand All @@ -57,10 +57,10 @@ class ProtocolTest {
Create(Id(1), WidgetTag(2)),
ChildrenChange.Add(Id(1), ChildrenTag(2), Id(3), 4),
ChildrenChange.Move(Id(1), ChildrenTag(2), 3, 4, 5),
ChildrenChange.Remove(Id(1), ChildrenTag(2), 3, 4, listOf(Id(5), Id(6), Id(7), Id(8))),
ChildrenChange.Remove(Id(1), ChildrenTag(2), 3, 4, arrayOf(Id(5), Id(6), Id(7), Id(8))),
ModifierChange(
Id(1),
listOf(
arrayOf(
ModifierElement(
ModifierTag(1),
buildJsonObject { },
Expand Down Expand Up @@ -100,7 +100,7 @@ class ProtocolTest {

@Test fun removeCountMustMatchListSize() {
val t = assertFailsWith<IllegalArgumentException> {
ChildrenChange.Remove(Id(1), ChildrenTag(2), 3, 4, listOf(Id(5), Id(6), Id(7)))
ChildrenChange.Remove(Id(1), ChildrenTag(2), 3, 4, arrayOf(Id(5), Id(6), Id(7)))
}
assertThat(t).hasMessage("Count 4 != Removed ID list size 3")
}
Expand Down
6 changes: 3 additions & 3 deletions redwood-protocol/src/commonTest/kotlin/app/cash/redwood/protocol/SnapshotChangeListTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class SnapshotChangeListTest {
val expected = SnapshotChangeList(
listOf(
Create(Id(1), WidgetTag(1)),
ModifierChange(Id(1), emptyList()),
ModifierChange(Id(1), emptyArray()),
PropertyChange(Id(1), PropertyTag(1), JsonPrimitive("Hello")),
Add(Id.Root, ChildrenTag.Root, Id(1), 0),
),
Expand All @@ -58,11 +58,11 @@ class SnapshotChangeListTest {
SnapshotChangeList(
listOf(
Create(Id(1), WidgetTag(1)),
ModifierChange(Id(1), emptyList()),
ModifierChange(Id(1), emptyArray()),
Move(Id.Root, ChildrenTag.Root, 1, 2, 3),
PropertyChange(Id(1), PropertyTag(1), JsonPrimitive("Hello")),
Add(Id.Root, ChildrenTag.Root, Id(1), 0),
Remove(Id.Root, ChildrenTag.Root, 1, 2, listOf(Id(3), Id(4))),
Remove(Id.Root, ChildrenTag.Root, 1, 2, arrayOf(Id(3), Id(4))),
),
)
}.hasMessage(
Expand Down
14 changes: 7 additions & 7 deletions redwood-testing/src/commonTest/kotlin/app/cash/redwood/testing/ViewTreesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,26 +69,26 @@ class ViewTreesTest {

val expected = listOf(
Create(Id(1), WidgetTag(1)),
ModifierChange(Id(1), emptyList()),
ModifierChange(Id(1), emptyArray()),
Create(Id(2), WidgetTag(1)),
ModifierChange(Id(2), emptyList()),
ModifierChange(Id(2), emptyArray()),
Create(Id(3), WidgetTag(3)),
ModifierChange(Id(3), emptyList()),
ModifierChange(Id(3), emptyArray()),
PropertyChange(Id(3), PropertyTag(1), JsonPrimitive("One Fish")),
Add(Id(2), ChildrenTag(1), Id(3), 0),
Create(Id(4), WidgetTag(3)),
ModifierChange(Id(4), emptyList()),
ModifierChange(Id(4), emptyArray()),
PropertyChange(Id(4), PropertyTag(1), JsonPrimitive("Two Fish")),
Add(Id(2), ChildrenTag(1), Id(4), 1),
Add(Id(1), ChildrenTag(1), Id(2), 0),
Create(Id(5), WidgetTag(1)),
ModifierChange(Id(5), emptyList()),
ModifierChange(Id(5), emptyArray()),
Create(Id(6), WidgetTag(3)),
ModifierChange(Id(6), emptyList()),
ModifierChange(Id(6), emptyArray()),
PropertyChange(Id(6), PropertyTag(1), JsonPrimitive("Red Fish")),
Add(Id(5), ChildrenTag(1), Id(6), 0),
Create(Id(7), WidgetTag(3)),
ModifierChange(Id(7), emptyList()),
ModifierChange(Id(7), emptyArray()),
PropertyChange(Id(7), PropertyTag(1), JsonPrimitive("Blue Fish")),
Add(Id(5), ChildrenTag(1), Id(7), 1),
Add(Id(1), ChildrenTag(1), Id(5), 1),
Expand Down
4 changes: 3 additions & 1 deletion ...ing-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/composeProtocolGeneration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import app.cash.redwood.tooling.schema.ProtocolWidget
import app.cash.redwood.tooling.schema.ProtocolWidget.ProtocolChildren
import app.cash.redwood.tooling.schema.ProtocolWidget.ProtocolEvent
import app.cash.redwood.tooling.schema.ProtocolWidget.ProtocolProperty
import com.squareup.kotlinpoet.ARRAY
import com.squareup.kotlinpoet.AnnotationSpec
import com.squareup.kotlinpoet.BOOLEAN
import com.squareup.kotlinpoet.BYTE
Expand Down Expand Up @@ -704,14 +705,15 @@ internal fun generateComposeProtocolModifierSerialization(
.addModifiers(INTERNAL)
.receiver(Redwood.Modifier)
.addParameter("json", KotlinxSerialization.Json)
.returns(LIST.parameterizedBy(Protocol.ModifierElement))
.returns(ARRAY.parameterizedBy(Protocol.ModifierElement))
.beginControlFlow("return %M", Stdlib.buildList)
.addStatement(
"this@%L.forEach { element -> add(element.%M(json)) }",
name,
schema.modifierToProtocol,
)
.endControlFlow()
.addCode("⇥.toTypedArray()⇤")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not convinced that just adding this actually helps perf since it's still building a list first. But I think to build an array directly, Modifier would need a size and an iterator.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We control Modifier so we could add behavior to expose the size if we need to.

.build(),
)
.addFunction(
Expand Down
8 changes: 4 additions & 4 deletions ...ling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/widgetProtocolGeneration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import app.cash.redwood.tooling.schema.ProtocolWidget
import app.cash.redwood.tooling.schema.ProtocolWidget.ProtocolChildren
import app.cash.redwood.tooling.schema.ProtocolWidget.ProtocolEvent
import app.cash.redwood.tooling.schema.ProtocolWidget.ProtocolProperty
import com.squareup.kotlinpoet.ARRAY
import com.squareup.kotlinpoet.AnnotationSpec
import com.squareup.kotlinpoet.ClassName
import com.squareup.kotlinpoet.CodeBlock
Expand All @@ -29,7 +30,6 @@ import com.squareup.kotlinpoet.FunSpec
import com.squareup.kotlinpoet.KModifier.INTERNAL
import com.squareup.kotlinpoet.KModifier.OVERRIDE
import com.squareup.kotlinpoet.KModifier.PRIVATE
import com.squareup.kotlinpoet.LIST
import com.squareup.kotlinpoet.ParameterSpec
import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy
import com.squareup.kotlinpoet.PropertySpec
Expand Down Expand Up @@ -281,7 +281,7 @@ internal fun generateProtocolNode(
beginControlFlow("{ %L ->", trait.parameterTypes.indices.map { CodeBlock.of("arg$it") }.joinToCode())
}
addStatement(
"eventSink.sendEvent(%T(change.id, %T(%L), listOf(%L)))",
"eventSink.sendEvent(%T(change.id, %T(%L), arrayOf(%L)))",
Protocol.Event,
Protocol.EventTag,
trait.tag,
Expand Down Expand Up @@ -360,7 +360,7 @@ internal fun generateProtocolNode(
.addFunction(
FunSpec.builder("updateModifier")
.addModifiers(OVERRIDE)
.addParameter("elements", LIST.parameterizedBy(Protocol.ModifierElement))
.addParameter("elements", ARRAY.parameterizedBy(Protocol.ModifierElement))
.addStatement("widget.modifier = elements.%M(json, mismatchHandler)", host.toModifier)
.addStatement("container?.onModifierUpdated()")
.build(),
Expand Down Expand Up @@ -444,7 +444,7 @@ internal fun generateProtocolModifierImpls(
private fun generateJsonArrayToModifier(schema: ProtocolSchema): FunSpec {
return FunSpec.builder("toModifier")
.addModifiers(INTERNAL)
.receiver(LIST.parameterizedBy(Protocol.ModifierElement))
.receiver(ARRAY.parameterizedBy(Protocol.ModifierElement))
.addParameter("json", KotlinxSerialization.Json)
.addParameter("mismatchHandler", WidgetProtocol.ProtocolMismatchHandler)
.addStatement(
Expand Down