Merge branch 'trunk' into colin/density

* trunk: Ensure children lambda type returns Unit (#932) Respect event type nullability (#931)
cashapp · Apr 19, 2023 · 5cb656a · 5cb656a
2 parents a244fc5 + 02fdaa6
commit 5cb656a
Show file tree

Hide file tree

Showing 13 changed files with 69 additions and 29 deletions.
diff --git a/...et/src/commonTest/kotlin/app/cash/redwood/protocol/widget/DiffConsumingNodeFactoryTest.kt b/...et/src/commonTest/kotlin/app/cash/redwood/protocol/widget/DiffConsumingNodeFactoryTest.kt
@@ -342,14 +342,14 @@ class DiffConsumingNodeFactoryTest {
     var onChange: ((String) -> Unit)? = null
       private set
 
-    override fun onChange(onChange: ((String) -> Unit)?) {
+    override fun onChange(onChange: (String) -> Unit) {
       this.onChange = onChange
     }
 
     var onChangeCustomType: ((Duration) -> Unit)? = null
       private set
 
-    override fun onChangeCustomType(onChangeCustomType: ((Duration) -> Unit)?) {
+    override fun onChangeCustomType(onChangeCustomType: (Duration) -> Unit) {
       this.onChangeCustomType = onChangeCustomType
     }
 

diff --git a/...ood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/composeGeneration.kt b/...ood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/composeGeneration.kt
@@ -107,7 +107,9 @@ internal fun generateComposable(
                 }
                 is Event -> {
                   ParameterSpec.builder(trait.name, trait.lambdaType)
-                    .defaultValue(trait.defaultExpression ?: "null")
+                    .apply {
+                      trait.defaultExpression?.let { defaultValue(it) }
+                    }
                     .build()
                 }
                 is Children -> {

diff --git a/...ing-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/composeProtocolGeneration.kt b/...ing-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/composeProtocolGeneration.kt
@@ -350,7 +350,7 @@ internal fun generateProtocolWidget(
               }
               is ProtocolEvent -> {
                 addProperty(
-                  PropertySpec.builder(trait.name, trait.lambdaType, PRIVATE)
+                  PropertySpec.builder(trait.name, trait.lambdaType.copy(nullable = true), PRIVATE)
                     .mutable()
                     .initializer("null")
                     .build(),
@@ -359,16 +359,24 @@ internal fun generateProtocolWidget(
                   FunSpec.builder(trait.name)
                     .addModifiers(OVERRIDE)
                     .addParameter(trait.name, trait.lambdaType)
-                    .addStatement("val %1NSet = %1N != null", trait.name)
-                    .beginControlFlow("if (%1NSet != (this.%1N != null))", trait.name)
-                    .addStatement(
-                      "this.state.append(%T(this.id, %T(%L), %M(%NSet)))",
-                      Protocol.PropertyDiff,
-                      Protocol.PropertyTag,
-                      trait.tag,
-                      KotlinxSerialization.JsonPrimitive,
-                      trait.name,
-                    )
+                    .apply {
+                      val newValue = if (trait.isNullable) {
+                        addStatement("val %1NSet = %1N != null", trait.name)
+                        beginControlFlow("if (%1NSet != (this.%1N != null))", trait.name)
+                        trait.name + "Set"
+                      } else {
+                        beginControlFlow("if (this.%1N == null)", trait.name)
+                        "true"
+                      }
+                      addStatement(
+                        "this.state.append(%T(this.id, %T(%L), %M(%L)))",
+                        Protocol.PropertyDiff,
+                        Protocol.PropertyTag,
+                        trait.tag,
+                        KotlinxSerialization.JsonPrimitive,
+                        newValue,
+                      )
+                    }
                     .endControlFlow()
                     .addStatement("this.%1N = %1N", trait.name)
                     .build(),

diff --git a/...oling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/diffConsumingGeneration.kt b/...oling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/diffConsumingGeneration.kt
@@ -264,7 +264,11 @@ internal fun generateDiffConsumingWidget(
                         )
                       }
                       nextControlFlow("else")
-                      addStatement("null")
+                      if (trait.isNullable) {
+                        addStatement("null")
+                      } else {
+                        addStatement("throw %T()", Stdlib.AssertionError)
+                      }
                       endControlFlow()
                       addStatement("widget.%1N(%1N)", trait.name)
                       endControlFlow()

diff --git a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/sharedHelpers.kt b/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/sharedHelpers.kt
@@ -51,14 +51,16 @@ internal val FqType.flatName: String
 
 internal val Event.lambdaType: TypeName
   get() {
-    parameterType?.let { parameterType ->
-      return LambdaTypeName.get(null, parameterType.asTypeName(), returnType = UNIT)
-        .copy(nullable = true)
-    }
-    return noArgumentEventLambda
+    val lambda = parameterType
+      ?.let { parameterType ->
+        LambdaTypeName.get(null, parameterType.asTypeName(), returnType = UNIT)
+      }
+      ?: noArgumentEventLambda
+
+    return lambda.copy(nullable = isNullable)
   }
 
-private val noArgumentEventLambda = LambdaTypeName.get(returnType = UNIT).copy(nullable = true)
+private val noArgumentEventLambda = LambdaTypeName.get(returnType = UNIT)
 
 internal fun Schema.composePackage(host: Schema? = null): String {
   return if (host == null) {

diff --git a/...tooling-codegen/src/test/kotlin/app/cash/redwood/tooling/codegen/ComposeGenerationTest.kt b/...tooling-codegen/src/test/kotlin/app/cash/redwood/tooling/codegen/ComposeGenerationTest.kt
@@ -90,7 +90,7 @@ class ComposeGenerationTest {
     val fileSpec = generateComposable(schema, schema.widgets.single())
     assertThat(fileSpec.toString()).apply {
       contains("trait: String = \"test\"")
-      contains("onEvent: (() -> Unit)? = { error(\"test\") }")
+      contains("onEvent: () -> Unit = { error(\"test\") }")
       contains("block: @Composable () -> Unit = {}")
     }
   }

diff --git a/...tooling-codegen/src/test/kotlin/app/cash/redwood/tooling/codegen/TestingGenerationTest.kt b/...tooling-codegen/src/test/kotlin/app/cash/redwood/tooling/codegen/TestingGenerationTest.kt
@@ -98,15 +98,15 @@ class TestingGenerationTest {
       |    this.trait = trait
       |  }
       |
-      |  public override fun onEvent(onEvent: (() -> Unit)?): Unit {
+      |  public override fun onEvent(onEvent: () -> Unit): Unit {
       |    this.onEvent = onEvent
       |  }
       |
       |  public override fun snapshot(): TestingGenerationTestBasicWidgetValue =
       |      TestingGenerationTestBasicWidgetValue(
       |    layoutModifiers = layoutModifiers,
       |    trait = trait!!,
-      |    onEvent = onEvent,
+      |    onEvent = onEvent!!,
       |    block = block.map { it.`value`.snapshot() },
       |  )
       |}
@@ -119,7 +119,7 @@ class TestingGenerationTest {
       |public class TestingGenerationTestBasicWidgetValue(
       |  public override val layoutModifiers: LayoutModifier = LayoutModifier,
       |  public val trait: String = "test",
-      |  public val onEvent: (() -> Unit)? = { error("test") },
+      |  public val onEvent: () -> Unit = { error("test") },
       |  public val block: List<WidgetValue> = listOf(),
       |) : WidgetValue {
       |  public override val childrenLists: List<List<WidgetValue>>

diff --git a/redwood-tooling-schema/src/main/kotlin/app/cash/redwood/tooling/schema/schema.kt b/redwood-tooling-schema/src/main/kotlin/app/cash/redwood/tooling/schema/schema.kt
@@ -84,6 +84,7 @@ public interface Widget {
 
   public interface Event : Trait {
     public val parameterType: FqType?
+    public val isNullable: Boolean
   }
 
   public interface Children : Trait {

diff --git a/redwood-tooling-schema/src/main/kotlin/app/cash/redwood/tooling/schema/schemaClasses.kt b/redwood-tooling-schema/src/main/kotlin/app/cash/redwood/tooling/schema/schemaClasses.kt
@@ -94,6 +94,7 @@ internal data class ParsedProtocolEvent(
   override val tag: Int,
   override val name: String,
   override val parameterType: FqType?,
+  override val isNullable: Boolean,
   override val defaultExpression: String? = null,
   override val deprecation: ParsedDeprecation? = null,
 ) : ProtocolEvent

diff --git a/redwood-tooling-schema/src/main/kotlin/app/cash/redwood/tooling/schema/schemaParser.kt b/redwood-tooling-schema/src/main/kotlin/app/cash/redwood/tooling/schema/schemaParser.kt
@@ -34,6 +34,7 @@ import kotlin.reflect.full.memberProperties
 import kotlin.reflect.full.primaryConstructor
 import kotlin.reflect.full.starProjectedType
 import kotlin.reflect.full.withNullability
+import kotlin.reflect.typeOf
 
 private val childrenType = Function::class.starProjectedType
 private val eventType = Function::class.starProjectedType
@@ -230,6 +231,7 @@ private fun parseWidget(
             tag = property.tag,
             name = name,
             parameterType = arguments.singleOrNull()?.type?.toFqType(),
+            isNullable = type.isMarkedNullable,
             defaultExpression = defaultExpression,
             deprecation = deprecation,
           )
@@ -243,11 +245,11 @@ private fun parseWidget(
           )
         }
       } else if (children != null) {
-        require(type.isSubtypeOf(childrenType)) {
+        require(type.isSubtypeOf(childrenType) && type.arguments.last().type == typeOf<Unit>()) {
           "@Children ${memberType.qualifiedName}#$name must be of type '() -> Unit'"
         }
         var scope: FqType? = null
-        var arguments = type.arguments.dropLast(1) // Drop return type.
+        var arguments = type.arguments.dropLast(1) // Drop Unit return type.
         if (type.annotations.any(ExtensionFunctionType::class::isInstance)) {
           val receiverType = type.arguments.first().type
           val receiverClassifier = receiverType?.classifier

diff --git a/redwood-tooling-schema/src/test/kotlin/app/cash/redwood/tooling/schema/SchemaParserTest.kt b/redwood-tooling-schema/src/test/kotlin/app/cash/redwood/tooling/schema/SchemaParserTest.kt
@@ -309,6 +309,26 @@ class SchemaParserTest {
     )
   }
 
+  @Schema(
+    [
+      InvalidChildrenLambdaReturnTypeWidget::class,
+    ],
+  )
+  interface InvalidChildrenLambdaReturnTypeSchema
+
+  @Widget(1)
+  data class InvalidChildrenLambdaReturnTypeWidget(
+    @Children(1) val children: () -> String,
+  )
+
+  @Test fun invalidChildrenLambdaReturnTypeThrows() {
+    assertThrows<IllegalArgumentException> {
+      parseProtocolSchema(InvalidChildrenLambdaReturnTypeSchema::class)
+    }.hasMessageThat().isEqualTo(
+      "@Children app.cash.redwood.tooling.schema.SchemaParserTest.InvalidChildrenLambdaReturnTypeWidget#children must be of type '() -> Unit'",
+    )
+  }
+
   @Schema(
     [
       ChildrenArgumentsInvalidWidget::class,

diff --git a/samples/emoji-search/schema/src/main/kotlin/com/example/redwood/emojisearch/schema.kt b/samples/emoji-search/schema/src/main/kotlin/com/example/redwood/emojisearch/schema.kt
@@ -47,7 +47,7 @@ data class TextInput(
   val hint: String,
   @Property(3)
   @Default("null")
-  val onChange: (TextFieldState) -> Unit,
+  val onChange: ((TextFieldState) -> Unit)?,
 )
 
 @Widget(2)

diff --git a/test-schema/src/main/kotlin/example/redwood/schema.kt b/test-schema/src/main/kotlin/example/redwood/schema.kt
@@ -66,7 +66,7 @@ public data class Text(
 @Widget(4)
 public data class Button(
   @Property(1) val text: String?,
-  @Property(2) val onClick: () -> Unit,
+  @Property(2) val onClick: (() -> Unit)?,
 )
 
 @Widget(5)