Work around a bug where LazyList.placeholders crash on remove (#1948)

* Work around a bug where LazyList.placeholders crash on remove

Unfortunately this requires a change to generated code. But it only
changes the behavior when talking to older apps that don't have the
proper fix to the memory leak.

* Changelog

* Fix println

* apiDump

CHANGELOG.md

@@ -10,7 +10,7 @@ Changed:
 - Nothing yet!
 
 Fixed:
-- Nothing yet!
+- Work around a problem with our memory-leak fix where our old LazyList code would crash when its placeholders were unexpectedly removed.
 
 
 ## [0.10.0] - 2024-04-05

redwood-protocol-guest/api/android/redwood-protocol-guest.api

@@ -29,6 +29,7 @@ public final class app/cash/redwood/protocol/guest/ProtocolState {
 	public final fun addWidget (Lapp/cash/redwood/protocol/guest/ProtocolWidget;)V
 	public final fun append (Lapp/cash/redwood/protocol/Change;)V
 	public final fun getChangesOrNull ()Ljava/util/List;
+	public final fun getSynthesizeSubtreeRemoval ()Z
 	public final fun getWidget-ou3jOuA (I)Lapp/cash/redwood/protocol/guest/ProtocolWidget;
 	public final fun nextId-0HhLjSo ()I
 	public final fun removeWidget-ou3jOuA (I)V

redwood-protocol-guest/api/jvm/redwood-protocol-guest.api

@@ -29,6 +29,7 @@ public final class app/cash/redwood/protocol/guest/ProtocolState {
 	public final fun addWidget (Lapp/cash/redwood/protocol/guest/ProtocolWidget;)V
 	public final fun append (Lapp/cash/redwood/protocol/Change;)V
 	public final fun getChangesOrNull ()Ljava/util/List;
+	public final fun getSynthesizeSubtreeRemoval ()Z
 	public final fun getWidget-ou3jOuA (I)Lapp/cash/redwood/protocol/guest/ProtocolWidget;
 	public final fun nextId-0HhLjSo ()I
 	public final fun removeWidget-ou3jOuA (I)V

redwood-protocol-guest/api/redwood-protocol-guest.klib.api

@@ -42,6 +42,8 @@ final class app.cash.redwood.protocol.guest/ProtocolState { // app.cash.redwood.
     final fun getWidget(app.cash.redwood.protocol/Id): app.cash.redwood.protocol.guest/ProtocolWidget? // app.cash.redwood.protocol.guest/ProtocolState.getWidget|getWidget(app.cash.redwood.protocol.Id){}[0]
     final fun nextId(): app.cash.redwood.protocol/Id // app.cash.redwood.protocol.guest/ProtocolState.nextId|nextId(){}[0]
     final fun removeWidget(app.cash.redwood.protocol/Id) // app.cash.redwood.protocol.guest/ProtocolState.removeWidget|removeWidget(app.cash.redwood.protocol.Id){}[0]
+    final val synthesizeSubtreeRemoval // app.cash.redwood.protocol.guest/ProtocolState.synthesizeSubtreeRemoval|{}synthesizeSubtreeRemoval[0]
+        final fun <get-synthesizeSubtreeRemoval>(): kotlin/Boolean // app.cash.redwood.protocol.guest/ProtocolState.synthesizeSubtreeRemoval.<get-synthesizeSubtreeRemoval>|<get-synthesizeSubtreeRemoval>(){}[0]
 }
 final class app.cash.redwood.protocol.guest/ProtocolWidgetChildren : app.cash.redwood.widget/Widget.Children<kotlin/Unit> { // app.cash.redwood.protocol.guest/ProtocolWidgetChildren|null[0]
     constructor <init>(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/ChildrenTag, app.cash.redwood.protocol.guest/ProtocolState) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.<init>|<init>(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.ChildrenTag;app.cash.redwood.protocol.guest.ProtocolState){}[0]

redwood-protocol-guest/build.gradle

@@ -26,6 +26,7 @@ kotlin {
         implementation libs.kotlin.test
         implementation libs.assertk
         implementation libs.kotlinx.coroutines.test
+        implementation projects.redwoodLazylayoutCompose
         implementation projects.redwoodTesting
         implementation projects.testApp.schema.compose
         implementation projects.testApp.schema.composeProtocol

redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolState.kt

@@ -34,7 +34,7 @@ public class ProtocolState(
    * from the protocol map which leaked any child views of a removed node. We can work around this
    * on the guest side by synthesizing removes for every node in the subtree.
    */
-  internal val synthesizeSubtreeRemoval = hostVersion < RedwoodVersion("0.10.0-SNAPSHOT")
+  public val synthesizeSubtreeRemoval: Boolean = hostVersion < RedwoodVersion("0.10.0-SNAPSHOT")
 
   public fun nextId(): Id {
     val value = nextValue

redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt

@@ -22,6 +22,7 @@ import androidx.compose.runtime.setValue
 import app.cash.redwood.compose.WidgetVersion
 import app.cash.redwood.layout.compose.Column
 import app.cash.redwood.layout.compose.Row
+import app.cash.redwood.lazylayout.compose.LazyColumn
 import app.cash.redwood.protocol.Change
 import app.cash.redwood.protocol.ChildrenChange
 import app.cash.redwood.protocol.ChildrenTag
@@ -301,35 +302,72 @@ class ProtocolTest {
       )
   }
 
-  private suspend fun TestScope.removeSubtree(hostVersion: RedwoodVersion): List<Change> {
+  @Test fun entireSubtreeRemovedForLazyListPlaceholders() = runTest {
+    assertThat(removeSubtree(latestVersion, lazyList = true))
+      .containsExactly(
+        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1),
+      )
+  }
+
+  /**
+   * Our LazyList binding on host platforms incorrectly assumed that the placeholders children was
+   * append-only. When we fixed a host-side memory leak by traversing guest children, that
+   * introduced a crash. Special-case this by not synthesizing subtree removal for these children.
+   */
+  @Test fun entireSubtreeNotRemovedForLazyListPlaceholders() = runTest {
+    assertThat(removeSubtree(RedwoodVersion("0.9.0"), lazyList = true))
+      .containsExactly(
+        ChildrenChange.Remove(Id(2), ChildrenTag(2), 0, 1, listOf(Id(23))),
+        ChildrenChange.Remove(Id(1), ChildrenTag(1), 0, 1, listOf(Id(2))),
+        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1, listOf(Id(1))),
+      )
+  }
+
+  private suspend fun TestScope.removeSubtree(
+    hostVersion: RedwoodVersion,
+    lazyList: Boolean = false,
+  ): List<Change> {
     val (composition, bridge) = testProtocolComposition(hostVersion)
 
     var clicks = 0
     var remove by mutableStateOf(false)
     composition.setContent {
       if (!remove) {
         Row {
-          Column {
-            Button("Click?", onClick = { clicks++ })
+          if (lazyList) {
+            LazyColumn(
+              placeholder = {
+                Text("placeholder")
+              },
+            ) {
+              item {
+                Button("Click?", onClick = { clicks++ })
+              }
+            }
+          } else {
+            Column {
+              Button("Click?", onClick = { clicks++ })
+            }
           }
         }
       }
     }
-    composition.awaitSnapshot()
+    val initialSnapshot = composition.awaitSnapshot()
+    val button = initialSnapshot.first { it is Create && it.tag.value == 4 }
     assertThat(clicks).isEqualTo(0)
 
     // Ensure the button is present and receiving clicks.
-    bridge.sendEvent(Event(Id(3), EventTag(2)))
+    bridge.sendEvent(Event(button.id, EventTag(2)))
     assertThat(clicks).isEqualTo(1)
 
     remove = true
     val removeChanges = composition.awaitSnapshot()
 
     // If the whole tree was removed, we cannot target the button anymore.
-    assertFailure { bridge.sendEvent(Event(Id(3), EventTag(2))) }
+    assertFailure { bridge.sendEvent(Event(button.id, EventTag(2))) }
       .isInstanceOf<IllegalArgumentException>()
       .message()
-      .isEqualTo("Unknown node ID 3 for event with tag 2")
+      .isEqualTo("Unknown node ID ${button.id.value} for event with tag 2")
     assertThat(clicks).isEqualTo(1)
 
     return removeChanges

redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt

@@ -541,7 +541,14 @@ internal fun generateProtocolWidget(
             .apply {
               for (trait in widget.traits) {
                 if (trait is ProtocolChildren) {
-                  addStatement("%N.depthFirstWalk(this, block)", trait.name)
+                  if (workAroundLazyListPlaceholderRemoveCrash(widget, trait)) {
+                    addComment("Work around the LazyList.placeholder remove crash.")
+                    beginControlFlow("if (!state.synthesizeSubtreeRemoval)")
+                    addStatement("%N.depthFirstWalk(this, block)", trait.name)
+                    endControlFlow()
+                  } else {
+                    addStatement("%N.depthFirstWalk(this, block)", trait.name)
+                  }
                 }
               }
             }
@@ -552,6 +559,22 @@ internal fun generateProtocolWidget(
     .build()
 }
 
+private val lazyListTypeNames = listOf("app.cash.redwood.lazylayout", "LazyList")
+
+/**
+ * Returns true if this is the `LazyList.placeholder` trait, which had a severe bug in host code
+ * by assuming `Widget.Children.remove()` is never be called. (This started crashing when we fixed
+ * host-side memory leaks by removing guest-side children.)
+ *
+ * We work around this by not attempting to fix the host-side memory leak. This turns out to not
+ * be a problem in practice anyway, because we never remove placeholders until we remove the
+ * entire LazyList.
+ */
+private fun workAroundLazyListPlaceholderRemoveCrash(
+  widget: ProtocolWidget,
+  trait: ProtocolWidget.ProtocolTrait,
+): Boolean = widget.type.names == lazyListTypeNames && trait.name == "placeholder"
+
 /*
 internal object GrowSerializer : KSerializer<Grow> {
   override val descriptor =