Generate Kotlin-esque build function for using closures in addition to Builders #2674

rynonl · 2023-10-10T21:19:24Z

This adds a static build method on Messages to allow for a more Kotlin-esque style of building up proto objects, rather than forcing everyone into Builders now that constructors are generally deemed unsafe.

Usage:

// .proto
message MyMessage {
  optional int32 property = 1;
}

// Wire generated
companion object {
  ...

  public fun build(body: Builder.() -> Unit): MyMessage = Builder().apply(body).build()
}

// .kt
val message = MyMessage.build {
  property = 5
}

rynonl · 2023-10-10T21:21:13Z

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt

@@ -548,6 +549,7 @@ class KotlinGenerator private constructor(

    addDefaultFields(type, companionBuilder, nameAllocator)
    addAdapter(type, companionBuilder)
+    if (buildersOnly || javaInterOp) addBuildClosure(type, companionBuilder)


I added support for javaInterop since we get Builders there too - but I don't know exactly what the implications would be if Java code tried to use this (not a Java expert). Looking for feedback here.

I probably wouldn't include it for buildersOnly since this technically isn't a classic Java builder pattern, but would for javaInterop if this build closure is going to be default generated for all Kotlin targets.

It probably shouldn't be generated if the target is Java given I don't think Java supports Kotlin-style closures like this.

adrw · 2023-10-10T21:53:30Z

Is it possible to not have to call it for all it.property = value assignments? That would align this implementation with Google's Kotlin support in protobuf and also be a bit more ergonomic. https://protobuf.dev/getting-started/kotlintutorial/

Before

val person = com.protos.Person {
  it.id = "123"
  it.name = "Alice Bob"
}

After

val person = com.protos.Person {
  id = "123"
  name = "Alice Bob"
}

JakeWharton

Please also add a test somewhere that uses this function and compares it to a regular built value to confirm it works.

Like

@Test fun works() {
  val a = Foo.build {
    name = "hey"
    age = 21
  }
  val b = Foo.Builder()
    .name("hey")
    .age(21)
    .build()
  assertThat(a).isEqualTo(b)
}

JakeWharton · 2023-10-11T14:22:20Z

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt

+      addStatement("return builder.build()")
+    }
+    val buildFn = FunSpec.builder("build")
+      .addParameter("buildFn",


Generally we avoid abbreviations in names. I think "builder" or "body" would work here.

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt

rynonl · 2023-10-11T14:38:15Z

Is it possible to not have to call it for all it.property = value assignments?

It is, but it introduces a variable shadowing issue:

val thing = "foo"

Message.build {
  thing = thing // this is a self referential assignment, the outer `thing` is completely inaccessible inside this block
}

Using this.thing also does not work, because thing implicitly refers to this.thing anyway...that's why I took the it approach so that you're explicitly referencing the argument and not the receiver.

The workaround is to rename the outer thing something else that doesn't conflict with a property name on the proto. It's not the end of the world, and I would be happy with that approach if others are.

rynonl · 2023-10-11T15:32:22Z

Using this.thing also does not work, because thing implicitly refers to this.thing anyway...that's why I took the it approach so that you're explicitly referencing the argument and not the receiver.

Actually, hold that thought. I had tested this previously but looks like maybe I had done something wrong. If we pass the builder as the receiver, looks like maybe we can use this. to differentiate when the outer val has a name conflict.

val x = 5

Message.build {
  x = x             // error
  this.x = x     // expected behavior
}

Are folks more into that? Seems cleaner for the happy case with a fairly straightforward workaround when name conflicts arise (which shouldn't actually be all that often since Wire generates snake_case properties and folks typically use camelCase variable names)

rynonl · 2023-10-11T15:40:49Z

Changed to use Builder as receiver in this commit, I think that's probably the right path here.

Usage:

// .proto
message MyMessage {
  optional int32 property = 1;
}

// Wire generated
companion object {
  ...

  public fun build(body: Builder.() -> Unit): MyMessage {
    val builder = Builder()
    builder.body()
    return builder.build()
  }
}

// .kt
val message = MyMessage.build {
  property = 5
}

JakeWharton

LGTM. Just two small tweaks I didn't notice first time around.

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt

…uilders

oldergod

👍

oldergod · 2023-10-11T19:13:01Z

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt

@@ -1968,6 +1971,23 @@ class KotlinGenerator private constructor(
    }
  }

+  private fun addBuildClosure(type: MessageType, companionBuilder: TypeSpec.Builder, builderClassName: ClassName) {
+    val buildFn = FunSpec.builder("build")


Can you rename to buildFunction or whatever explicit name this could be

oldergod · 2023-10-11T19:14:04Z

wire-kotlin-generator/src/test/java/com/squareup/wire/kotlin/KotlinGeneratorTest.kt

+      .generateKotlin("SomeMessage", buildersOnly = false, javaInterop = false)
+    assertThat(code).doesNotContain("inline fun build(body: Builder.() -> Unit): SomeMessage")
+    val buildersOnlyCode = KotlinWithProfilesGenerator(schema)
+      .generateKotlin("SomeMessage", buildersOnly = true)


Suggested change

.generateKotlin("SomeMessage", buildersOnly = true)

.generateKotlin("SomeMessage", buildersOnly = true, javaInterop = false)

oldergod · 2023-10-11T19:14:18Z

wire-kotlin-generator/src/test/java/com/squareup/wire/kotlin/KotlinGeneratorTest.kt

+      .generateKotlin("SomeMessage", buildersOnly = true)
+    assertThat(buildersOnlyCode).contains("inline fun build(body: Builder.() -> Unit): SomeMessage")
+    val javaInteropCode = KotlinWithProfilesGenerator(schema)
+      .generateKotlin("SomeMessage", javaInterop = true)


Suggested change

.generateKotlin("SomeMessage", javaInterop = true)

.generateKotlin("SomeMessage", buildersOnly = false, javaInterop = true)

oldergod · 2023-10-11T19:21:53Z

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt

@@ -1968,6 +1971,23 @@ class KotlinGenerator private constructor(
    }
  }

+  private fun addBuildClosure(type: MessageType, companionBuilder: TypeSpec.Builder, builderClassName: ClassName) {


Suggested change

private fun addBuildClosure(type: MessageType, companionBuilder: TypeSpec.Builder, builderClassName: ClassName) {

/**

* Adds a closure into the [companionBuilder] allowing the creation of an instance via the Kotlin

* DSL.

*

* Example

* ```

* companion object {

* public inline fun build(body: Builder.() -> Unit): AllTypes = Builder().apply(body).build()

* }

* ```

*/

private fun addBuildClosure(type: MessageType, companionBuilder: TypeSpec.Builder, builderClassName: ClassName) {

oldergod · 2023-10-12T10:55:06Z

I think it'd be good to hide it from Java callers using @JvmSynthetic. I'll do that in followup I think

rynonl · 2023-10-12T14:39:08Z

@oldergod Sorry missed your comments! Followup incoming.

rynonl commented Oct 10, 2023

View reviewed changes

rynonl marked this pull request as ready for review October 10, 2023 21:21

JakeWharton approved these changes Oct 11, 2023

View reviewed changes

rynonl force-pushed the add-kotlin-builder branch from ae795fc to 3f91236 Compare October 11, 2023 15:37

JakeWharton approved these changes Oct 11, 2023

View reviewed changes

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt Show resolved Hide resolved

wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt Outdated Show resolved Hide resolved

rynonl force-pushed the add-kotlin-builder branch 6 times, most recently from f6c9499 to c406c41 Compare October 11, 2023 18:22

Generate Kotlin-esque build function for using closures rather than B…

1ba2b30

…uilders

rynonl force-pushed the add-kotlin-builder branch from c406c41 to 1ba2b30 Compare October 11, 2023 18:55

oldergod approved these changes Oct 11, 2023

View reviewed changes

rynonl merged commit 8cc350d into master Oct 12, 2023
14 checks passed

rynonl deleted the add-kotlin-builder branch October 12, 2023 14:38

rynonl mentioned this pull request Oct 12, 2023

Cleanup Kotlin build function #2678

Merged

oldergod mentioned this pull request Oct 16, 2023

Kotlin typesafe builder to help avoid the need for constructor use #2589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate Kotlin-esque build function for using closures in addition to Builders #2674

Generate Kotlin-esque build function for using closures in addition to Builders #2674

rynonl commented Oct 10, 2023 •

edited

Loading

rynonl Oct 10, 2023 •

edited

Loading

adrw Oct 10, 2023

adrw commented Oct 10, 2023 •

edited

Loading

JakeWharton left a comment

JakeWharton Oct 11, 2023

rynonl commented Oct 11, 2023 •

edited

Loading

rynonl commented Oct 11, 2023

rynonl commented Oct 11, 2023 •

edited

Loading

JakeWharton left a comment

oldergod left a comment

oldergod Oct 11, 2023

oldergod Oct 11, 2023

oldergod Oct 11, 2023

oldergod Oct 11, 2023

oldergod commented Oct 12, 2023

rynonl commented Oct 12, 2023

	.generateKotlin("SomeMessage", buildersOnly = true)
	.generateKotlin("SomeMessage", buildersOnly = true, javaInterop = false)

	.generateKotlin("SomeMessage", javaInterop = true)
	.generateKotlin("SomeMessage", buildersOnly = false, javaInterop = true)

-  private fun addBuildClosure(type: MessageType, companionBuilder: TypeSpec.Builder, builderClassName: ClassName) {
+  /**
+   * Adds a closure into the [companionBuilder] allowing the creation of an instance via the Kotlin
+   * DSL.
+   *
+   * Example
+   * ```
+   * companion object {
+   *   public inline fun build(body: Builder.() -> Unit): AllTypes = Builder().apply(body).build()
+   * }
+   * ```
+   */
+  private fun addBuildClosure(type: MessageType, companionBuilder: TypeSpec.Builder, builderClassName: ClassName) {

Generate Kotlin-esque build function for using closures in addition to Builders #2674

Generate Kotlin-esque build function for using closures in addition to Builders #2674

Conversation

rynonl commented Oct 10, 2023 • edited Loading

rynonl Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

adrw Oct 10, 2023

Choose a reason for hiding this comment

adrw commented Oct 10, 2023 • edited Loading

Before

After

JakeWharton left a comment

Choose a reason for hiding this comment

JakeWharton Oct 11, 2023

Choose a reason for hiding this comment

rynonl commented Oct 11, 2023 • edited Loading

rynonl commented Oct 11, 2023

rynonl commented Oct 11, 2023 • edited Loading

JakeWharton left a comment

Choose a reason for hiding this comment

oldergod left a comment

Choose a reason for hiding this comment

oldergod Oct 11, 2023

Choose a reason for hiding this comment

oldergod Oct 11, 2023

Choose a reason for hiding this comment

oldergod Oct 11, 2023

Choose a reason for hiding this comment

oldergod Oct 11, 2023

Choose a reason for hiding this comment

oldergod commented Oct 12, 2023

rynonl commented Oct 12, 2023

rynonl commented Oct 10, 2023 •

edited

Loading

rynonl Oct 10, 2023 •

edited

Loading

adrw commented Oct 10, 2023 •

edited

Loading

rynonl commented Oct 11, 2023 •

edited

Loading

rynonl commented Oct 11, 2023 •

edited

Loading