Repository: Microsoft/thrifty (kotlin)

Issue:
Switch the default Kotlin struct style to builderless
Per the readme, Kotlin structs are still generated with `Builder` classes for backwards compatibility.  Since we're bumping from 2.x to 3.x, this is a fine opportunity to give people smaller code by default.

Pull request description:
Fixes #409

Patch:
diff --git a/README.md b/README.md
index 81feafa78..5e1c45f37 100644
--- a/README.md
+++ b/README.md
@@ -412,7 +412,7 @@ and subject to change in future releases*.  It is a legacy from the time before
 
 ## Kotlin Support
 
-As of version 1.0.0-RC1, Thrifty supports generated Kotlin code that is (almost) as small as its Java counterpart.  Instead of classes with final fields, Thrifty Kotlin structs are represented as `data class` structures whose members are annotated as `@JvmField`.  This produces classes which, when compiled, are nearly as small as their Java counterparts; the `Query` class above looks like this in Kotlin:
+Thrifty supports generated Kotlin code that is (almost) as small as its Java counterpart.  Instead of classes with final fields, Thrifty Kotlin structs are represented as `data class` structures whose members are annotated as `@JvmField`.  This produces classes which, when compiled, are nearly as small as their Java counterparts; the `Query` class above looks like this in Kotlin:
 
 ```kotlin
 data class Query(
@@ -429,7 +429,7 @@ data class Query(
     ADAPTER.write(protocol, this)
   }
 
-  private class QueryAdapter : Adapter<Query, Builder> {
+  private class QueryAdapter : Adapter<Query> {
     override fun read(protocol: Protocol): Query {
       // deserialization code as above
     }
@@ -461,9 +461,9 @@ There are a few new command-line options to control Kotlin code generation:
 java -jar thrifty-compiler.jar \
     --lang=kotlin \
     --kt-coroutine-clients \
-    --experimental-kt-builderless-structs \
     --kt-file-per-type \
     --omit-file-comments \
+    --kt-struct-builders \
     ...
 ```
 
@@ -485,7 +485,7 @@ interface Google {
 }
 ```
 
-Although builders are no longer strictly necessary, for compatibility with existing code, Thrifty will still generate them by default.  You can suppress them with the `--experimental-kt-builderless-structs` flag, which (as its name suggests) is currently considered "experimental".
+Builders are no longer necessary, and are not included by default.  For compatibility with existing code, you can use the `--kt-struct-builders` flag, which will result in Java-style classes with Builders.
 
 The final new flag is `--kt-file-per-type`.  Thrifty's convention is to generate a single Kotlin file per distinct JVM namespace.  For particularly large .thrift inputs, this is suboptimal.  Outlook Mobile's single, large, Kotlin file took up to one minute just to typecheck, using Kotlin 1.2.51!  For these cases, `--kt-file-per-type` will tell Thrifty to generate one single file per top-level class - just like the Java code.
 
diff --git a/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt b/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
index bf4733978..21579a19b 100644
--- a/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
+++ b/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
@@ -57,13 +57,12 @@ import java.util.ArrayList
  * [--map-type=java.util.HashMap]
  * [--lang=[java|kotlin]]
  * [--kt-file-per-type]
+ * [--kt-struct-builders]
  * [--parcelable]
  * [--use-android-annotations]
  * [--nullability-annotation-type=[none|android-support|androidx]]
  * [--omit-service-clients]
  * [--omit-file-comments]
- * [--omit-generated-annotations]
- * [--generated-annotation-type=[jdk8|jdk9|native]]
  * file1.thrift
  * file2.thrift
  * ...
@@ -94,6 +93,11 @@ import java.util.ArrayList
  * types in a single package will go in one file named `ThriftTypes.kt`.  Implies
  * `--lang=kotlin`.
  *
+ * `--kt-struct-builders` is optional.  When specified, Kotlin structs will be generated
+ * with inner 'Builder' classes, in the same manner as Java code.  The Kotlin default is
+ * to use pure data classes.  This option is for retaining compatibility in codebases using
+ * older Thrifty versions, and should be avoided in new code.
+ *
  * `--parcelable` is optional.  When provided, generated types will contain a
  * `Parcelable` implementation.  Kotlin types will use the `@Parcelize` extension.
  *
@@ -203,8 +207,8 @@ class ThriftyCompiler {
                     help = "Generate struct Builder constructor with required parameters, and mark empty Builder constructor as deprecated")
                 .flag(default = false)
 
-        val kotlinBuilderlessDataClasses: Boolean by option("--experimental-kt-builderless-structs")
-                .flag(default = false)
+        val kotlinStructBuilders: Boolean by option("--kt-struct-builders")
+                .flag("--kt-no-struct-builders", default = false)
 
         val kotlinCoroutineClients: Boolean by option("--kt-coroutine-clients")
                 .flag(default = false)
@@ -244,9 +248,11 @@ class ThriftyCompiler {
             }
 
             val impliedLanguage = when {
-                kotlinBuilderlessDataClasses -> Language.KOTLIN
+                kotlinStructBuilders -> Language.KOTLIN
                 kotlinBuilderRequiredConstructor -> Language.KOTLIN
                 kotlinFilePerType -> Language.KOTLIN
+                kotlinCoroutineClients -> Language.KOTLIN
+                kotlinEmitJvmName -> Language.KOTLIN
                 nullabilityAnnotationType != NullabilityAnnotationType.NONE -> Language.JAVA
                 else -> null
             }
@@ -319,8 +325,8 @@ class ThriftyCompiler {
             setTypeName?.let { gen.setClassName(it) }
             mapTypeName?.let { gen.mapClassName(it) }
 
-            if (kotlinBuilderlessDataClasses) {
-                gen.builderlessDataClasses()
+            if (kotlinStructBuilders) {
+                gen.withDataClassBuilders()
             }
 
             if (kotlinBuilderRequiredConstructor) {
diff --git a/thrifty-gradle-plugin/README.md b/thrifty-gradle-plugin/README.md
index 8b1dc1b30..6cb143e24 100644
--- a/thrifty-gradle-plugin/README.md
+++ b/thrifty-gradle-plugin/README.md
@@ -84,9 +84,9 @@ thrifty {
 
         // Kotlin-specific options follow.
 
-        // When true, generated structs will be represented as pure data classes, without
-        // associated nested 'Builder' types.  This is experimental.
-        builderlessDataClasses = false
+        // When true, generated structs will be include 'Builder' inner classes.  This is
+        // for legacy compatibiltity only, and leads to larger code.
+        structBuilders = false
 
         // The Kotlin code generator supports several different service-client API styles.
         // Vaid values are 'default' (the default), 'coroutine', and 'none'.
diff --git a/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyExtension.kt b/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyExtension.kt
index 5540918da..f23dbb556 100644
--- a/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyExtension.kt
+++ b/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyExtension.kt
@@ -211,7 +211,7 @@ open class KotlinThriftOptions : ThriftOptions() {
         private set
 
     @Input
-    var builderlessDataClasses = false
+    var structBuilders = false
 
     fun serviceClientStyle(name: String) {
         val stylesByName = ClientStyle.values()
diff --git a/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyTask.kt b/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyTask.kt
index f8ca3a242..8fed281c7 100644
--- a/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyTask.kt
+++ b/thrifty-gradle-plugin/src/main/kotlin/com/microsoft/thrifty/gradle/ThriftyTask.kt
@@ -147,8 +147,8 @@ open class ThriftyTask @Inject constructor(
 
             failOnUnknownEnumValues(!options.allowUnknownEnumValues)
 
-            if (options.builderlessDataClasses) {
-                builderlessDataClasses()
+            if (options.structBuilders) {
+                withDataClassBuilders()
             }
 
             if (!options.emitServiceClients) {
diff --git a/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt b/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
index 2a76892cf..d3319ce24 100644
--- a/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
+++ b/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
@@ -134,7 +134,7 @@ class KotlinCodeGenerator(
     private var shouldImplementStruct: Boolean = true
 
     private var parcelize: Boolean = false
-    private var builderlessDataClasses: Boolean = false
+    private var builderlessDataClasses: Boolean = true
     private var builderRequiredConstructor: Boolean = false
     private var omitServiceClients: Boolean = false
     private var coroutineServiceClients: Boolean = false
@@ -229,8 +229,8 @@ class KotlinCodeGenerator(
         this.mapClassName = ClassName.bestGuess(name)
     }
 
-    fun builderlessDataClasses(): KotlinCodeGenerator = apply {
-        this.builderlessDataClasses = true
+    fun withDataClassBuilders(): KotlinCodeGenerator = apply {
+        this.builderlessDataClasses = false
     }
 
     fun builderRequiredConstructor(): KotlinCodeGenerator = apply {


Test patch:
diff --git a/thrifty-integration-tests/build.gradle b/thrifty-integration-tests/build.gradle
index 6f04493f8..5a8218673 100644
--- a/thrifty-integration-tests/build.gradle
+++ b/thrifty-integration-tests/build.gradle
@@ -69,6 +69,7 @@ def kompileTestThrift = tasks.register("kompileTestThrift", JavaExec) { t ->
     args = [
             "--out=$projectDir/build/generated-src/thrifty/kotlin",
             "--lang=kotlin",
+            "--kt-struct-builders",
             "--map-type=java.util.LinkedHashMap",
             "--set-type=java.util.LinkedHashSet",
             "--list-type=java.util.ArrayList",
@@ -86,7 +87,6 @@ def kompileCoroutineTestThrift = tasks.register("kompileCoroutineTestThrift", Ja
             "--out=$projectDir/build/generated-src/thrifty/kotlin",
             "--lang=kotlin",
             "--kt-coroutine-clients",
-            "--experimental-kt-builderless-structs",
             "$projectDir/CoroutineClientTest.thrift"
     ]
 }
diff --git a/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt b/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
index f1b1aff05..9f7125b16 100644
--- a/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
+++ b/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
@@ -497,7 +497,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift) { coroutineServiceClients() }
+        val file = generate(thrift) { withDataClassBuilders() }
 
         file.single().toString() should contain("""
             |  class Builder : StructBuilder<Union> {
@@ -539,7 +539,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift) { builderlessDataClasses() }
+        val file = generate(thrift)
 
         file.single().toString() shouldNot contain("""
             |    class Builder
@@ -583,7 +583,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift) //{ shouldImplementStruct() }
+        val file = generate(thrift) { withDataClassBuilders() }
 
         file.single().toString() should contain("""
             |    override fun read(protocol: Protocol) = read(protocol, Builder())
@@ -651,7 +651,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift) { builderlessDataClasses() }
+        val file = generate(thrift)
 
 
         file.single().toString() should contain("""
@@ -719,7 +719,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift)
+        val file = generate(thrift) { withDataClassBuilders() }
 
         file.single().toString() should contain("""
             |  private class UnionAdapter : Adapter<Union, Builder> {
@@ -739,7 +739,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift) { builderlessDataClasses() }
+        val file = generate(thrift)
 
         file.single().toString() should contain("""
             |  private class UnionAdapter : Adapter<Union> {
@@ -777,7 +777,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift) { coroutineServiceClients() }
+        val file = generate(thrift) { withDataClassBuilders() }
 
         file.single().toString() should contain("""
             |sealed class UnionStruct : Struct {
@@ -826,7 +826,7 @@ class KotlinCodeGeneratorTest {
             |}
         """.trimMargin()
 
-        val file = generate(thrift)
+        val file = generate(thrift) { withDataClassBuilders() }
 
         file.single().toString() should contain("""
             |    override fun build(): Bonk = Bonk(message = this.message, type = this.type)
@@ -859,7 +859,7 @@ class KotlinCodeGeneratorTest {
             }
           }"""
 
-        val file = generate(thrift)
+        val file = generate(thrift) { withDataClassBuilders() }
         file.single().toString() shouldContain expected
     }
 
@@ -902,7 +902,10 @@ class KotlinCodeGeneratorTest {
             }
           }"""
 
-        val file = generate(thrift) { failOnUnknownEnumValues(false) }
+        val file = generate(thrift) {
+            withDataClassBuilders()
+            failOnUnknownEnumValues(false)
+        }
         file.single().toString() shouldContain expected
     }
 
@@ -945,7 +948,7 @@ class KotlinCodeGeneratorTest {
             }
           }"""
 
-        val file = generate(thrift) { failOnUnknownEnumValues(false); builderlessDataClasses() }
+        val file = generate(thrift) { failOnUnknownEnumValues(false) }
         file.single().toString() shouldContain expected
     }
 
@@ -966,7 +969,10 @@ class KotlinCodeGeneratorTest {
       this.field2 = null
     }"""
 
-        val file = generate(thrift) { builderRequiredConstructor() }
+        val file = generate(thrift) {
+            withDataClassBuilders()
+            builderRequiredConstructor()
+        }
         file.single().toString() shouldContain expected
     }
 
@@ -987,7 +993,10 @@ class KotlinCodeGeneratorTest {
       replaceWith = ReplaceWith("Builder(field1)")
     )"""
 
-        val file = generate(thrift) { builderRequiredConstructor() }
+        val file = generate(thrift) {
+            withDataClassBuilders()
+            builderRequiredConstructor()
+        }
         file.single().toString() shouldContain expected
     }
 
@@ -1010,7 +1019,10 @@ class KotlinCodeGeneratorTest {
       this.field2 = null
     }"""
 
-        val file = generate(thrift) { builderRequiredConstructor() }
+        val file = generate(thrift) {
+            withDataClassBuilders()
+            builderRequiredConstructor()
+        }
         file.single().toString() shouldContain expected
         file.single().toString() shouldNotContain notExpected
     }