From b8d90eddec740f9aeaae823554597b46000f774a Mon Sep 17 00:00:00 2001 From: translatenix <119817707+translatenix@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:22:11 -0800 Subject: [PATCH] codegen-kotlin: Generate toString() methods consistent with data classes (#793) Motivation: codegen-kotlin generates a mix of data classes and regular classes. For regular classes, toString() methods are also generated. However, the output of generated toString() methods differs from the output of default toString() methods of data classes. Changes: Generate toString() methods that produce the same output as default toString() methods of data classes. Also: rename KotlinCodegenOptions to KotlinCodeGeneratorOptions The new name is consistent with existing names KotlinCodeGenerator and CliKotlinCodeGeneratorOptions. Backward compatibility is ensured by turning KotlinCodegenOptions into a (deprecated) type alias. --- .../kotlin/CliKotlinCodeGeneratorOptions.kt | 4 +- .../pkl/codegen/kotlin/KotlinCodeGenerator.kt | 116 ++++-------------- .../codegen/kotlin/KotlinCodeGeneratorTest.kt | 115 ++++++++--------- .../org/pkl/codegen/kotlin/Inheritance.kotlin | 49 +------- .../org/pkl/codegen/kotlin/Kdoc.kotlin | 24 +--- .../pkl/codegen/kotlin/PropertyTypes.kotlin | 57 +-------- 6 files changed, 86 insertions(+), 279 deletions(-) diff --git a/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGeneratorOptions.kt b/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGeneratorOptions.kt index 5024d4c3..1a1ef607 100644 --- a/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGeneratorOptions.kt +++ b/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGeneratorOptions.kt @@ -47,8 +47,8 @@ data class CliKotlinCodeGeneratorOptions( */ val renames: Map = emptyMap() ) { - fun toKotlinCodegenOptions(): KotlinCodegenOptions = - KotlinCodegenOptions( + fun toKotlinCodegenOptions(): KotlinCodeGeneratorOptions = + KotlinCodeGeneratorOptions( indent, generateKdoc, generateSpringBootConfig, diff --git a/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/KotlinCodeGenerator.kt b/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/KotlinCodeGenerator.kt index 6e1de95e..1cccb7cb 100644 --- a/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/KotlinCodeGenerator.kt +++ b/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/KotlinCodeGenerator.kt @@ -25,7 +25,10 @@ import org.pkl.core.* import org.pkl.core.util.CodeGeneratorUtils import org.pkl.core.util.IoUtils -data class KotlinCodegenOptions( +@Deprecated("renamed to KotlinCodeGeneratorOptions", ReplaceWith("KotlinCodeGeneratorOptions")) +typealias KotlinCodegenOptions = KotlinCodeGeneratorOptions + +data class KotlinCodeGeneratorOptions( /** The characters to use for indenting generated Kotlin code. */ val indent: String = " ", @@ -55,22 +58,9 @@ class KotlinCodeGenerator( private val moduleSchema: ModuleSchema, /** The options to use for the code generator */ - private val options: KotlinCodegenOptions, + private val options: KotlinCodeGeneratorOptions, ) { companion object { - // Prevent class name from being replaced with shaded name - // when pkl-codegen-kotlin is shaded and embedded in pkl-tools - // (requires circumventing kotlinc constant folding). - private val KOTLIN_TEXT_PACKAGE_NAME = buildString { - append("kot") - append("lin.") - append("text") - } - - // `StringBuilder::class.asClassName()` generates "java.lang.StringBuilder", - // apparently because `StringBuilder` is an `expect class`. - private val STRING_BUILDER = ClassName(KOTLIN_TEXT_PACKAGE_NAME, "StringBuilder") - private val STRING = String::class.asClassName() private val ANY_NULL = ANY.copy(nullable = true) private val NOTHING = Nothing::class.asClassName() @@ -151,36 +141,6 @@ class KotlinCodeGenerator( TypeSpec.companionObjectBuilder() } - // generate append method for module classes w/o parent class; - // reuse in subclasses and nested classes - val isGenerateAppendPropertyMethod = - isModuleType && - // check if we inherit another module's append method - pModuleClass.superclass!!.info == PClassInfo.Module && - // check if anyone is (potentially) going to use our append method - (pModuleClass.isOpen || - pModuleClass.isAbstract || - (isGenerateModuleClass && !builder.modifiers.contains(KModifier.DATA)) || - builder.typeSpecs.any { !it.modifiers.contains(KModifier.DATA) }) - - if (isGenerateAppendPropertyMethod) { - val appendPropertyMethodModifier = - if (pModuleClass.isOpen || pModuleClass.isAbstract) { - // alternative is `@JvmStatic protected` - // (`protected` alone isn't sufficient as of Kotlin 1.6) - KModifier.PUBLIC - } else KModifier.PRIVATE - if (isGenerateModuleClass) { - companionObjectBuilder.value.addFunction( - appendPropertyMethod().addModifiers(appendPropertyMethodModifier).build() - ) - } else { // kotlin object - builder.addFunction( - appendPropertyMethod().addModifiers(appendPropertyMethodModifier).build() - ) - } - } - // generate serialization code if ( options.implementSerializable && @@ -296,14 +256,8 @@ class KotlinCodeGenerator( } val codeBuilder = CodeBlock.builder().add("return %T(", kotlinPoetClassName) - var firstProperty = true - for (name in allProperties.keys) { - if (firstProperty) { - codeBuilder.add("%N", name) - firstProperty = false - } else { - codeBuilder.add(", %N", name) - } + for ((index, name) in allProperties.keys.withIndex()) { + codeBuilder.add(if (index == 0) "%N" else ", %N", name) } codeBuilder.add(")\n") @@ -384,35 +338,27 @@ class KotlinCodeGenerator( return builder.build() } + // produce same output as default toString() method of data classes fun generateToStringMethod(): FunSpec { - val builder = FunSpec.builder("toString").addModifiers(KModifier.OVERRIDE).returns(STRING) - - var builderSize = 50 - val appendBuilder = CodeBlock.builder() - for (propertyName in allProperties.keys) { - builderSize += 50 - appendBuilder.addStatement( - "appendProperty(builder, %S, this.%N)", - propertyName, - propertyName - ) - } - - builder - .addStatement("val builder = %T(%L)", STRING_BUILDER, builderSize) + return FunSpec.builder("toString") + .addModifiers(KModifier.OVERRIDE) + .returns(STRING) .addStatement( - // generate `::class.java.simpleName` instead of `::class.simpleName` - // to avoid making user code depend on kotlin-reflect - "builder.append(%T::class.java.simpleName).append(\" {\")", - kotlinPoetClassName + "return %P", + CodeBlock.builder() + .apply { + add("%L", pClass.toKotlinPoetName().simpleName) + add("(") + for ((index, propertyName) in allProperties.keys.withIndex()) { + add(if (index == 0) "%L" else ", %L", propertyName) + add("=$") + add("%N", propertyName) + } + add(")") + } + .build() ) - .addCode(appendBuilder.build()) - // not using %S here because it generates `"\n" + "{"` - // with a line break in the generated code after `+` - .addStatement("builder.append(\"\\n}\")") - .addStatement("return builder.toString()") - - return builder.build() + .build() } fun generateDeprecation( @@ -633,18 +579,6 @@ class KotlinCodeGenerator( // generating idiomatic KDoc would require parsing doc comments, converting member links, etc. private fun renderAsKdoc(docComment: String): String = docComment - private fun appendPropertyMethod() = - FunSpec.builder("appendProperty") - .addParameter("builder", STRING_BUILDER) - .addParameter("name", STRING) - .addParameter("value", ANY_NULL) - .addStatement("builder.append(\"\\n \").append(name).append(\" = \")") - .addStatement("val lines = value.toString().split(\"\\n\")") - .addStatement("builder.append(lines[0])") - .beginControlFlow("for (i in 1..lines.lastIndex)") - .addStatement("builder.append(\"\\n \").append(lines[i])") - .endControlFlow() - private fun PClass.toKotlinPoetName(): ClassName { val (packageName, moduleTypeName) = nameMapper.map(moduleName) return if (isModuleClass) { diff --git a/pkl-codegen-kotlin/src/test/kotlin/org/pkl/codegen/kotlin/KotlinCodeGeneratorTest.kt b/pkl-codegen-kotlin/src/test/kotlin/org/pkl/codegen/kotlin/KotlinCodeGeneratorTest.kt index 1c0486fa..8a1c7077 100644 --- a/pkl-codegen-kotlin/src/test/kotlin/org/pkl/codegen/kotlin/KotlinCodeGeneratorTest.kt +++ b/pkl-codegen-kotlin/src/test/kotlin/org/pkl/codegen/kotlin/KotlinCodeGeneratorTest.kt @@ -147,7 +147,7 @@ class KotlinCodeGeneratorTest { val generator = KotlinCodeGenerator( module, - KotlinCodegenOptions( + KotlinCodeGeneratorOptions( generateKdoc = generateKdoc, generateSpringBootConfig = generateSpringBootConfig, implementSerializable = implementSerializable @@ -195,65 +195,52 @@ class KotlinCodeGeneratorTest { assertThat(propertyTypes.toString()) .isEqualTo( + """PropertyTypes(boolean=true, int=42, float=42.3, string=string, duration=5.min, """ + + """durationUnit=min, dataSize=3.gb, dataSizeUnit=gb, nullable=idea, nullable2=null, """ + + """pair=(1, 2), pair2=(pigeon, Other(name=pigeon)), coll=[1, 2], """ + + """coll2=[Other(name=pigeon), Other(name=pigeon)], list=[1, 2], """ + + """list2=[Other(name=pigeon), Other(name=pigeon)], set=[1, 2], """ + + """set2=[Other(name=pigeon)], map={1=one, 2=two}, map2={one=Other(name=pigeon), """ + + """two=Other(name=pigeon)}, container={1=one, 2=two}, container2={one=Other(name=pigeon), """ + + """two=Other(name=pigeon)}, other=Other(name=pigeon), regex=(i?)\w*, any=Other(name=pigeon), """ + + """nonNull=Other(name=pigeon), enum=north)""" + ) + } + + @Test + fun `quoted identifiers`() { + val kotlinCode = + generateKotlinCode( """ - PropertyTypes { - boolean = true - int = 42 - float = 42.3 - string = string - duration = 5.min - durationUnit = min - dataSize = 3.gb - dataSizeUnit = gb - nullable = idea - nullable2 = null - pair = (1, 2) - pair2 = (pigeon, Other { - name = pigeon - }) - coll = [1, 2] - coll2 = [Other { - name = pigeon - }, Other { - name = pigeon - }] - list = [1, 2] - list2 = [Other { - name = pigeon - }, Other { - name = pigeon - }] - set = [1, 2] - set2 = [Other { - name = pigeon - }] - map = {1=one, 2=two} - map2 = {one=Other { - name = pigeon - }, two=Other { - name = pigeon - }} - container = {1=one, 2=two} - container2 = {one=Other { - name = pigeon - }, two=Other { - name = pigeon - }} - other = Other { - name = pigeon - } - regex = (i?)\w* - any = Other { - name = pigeon - } - nonNull = Other { - name = pigeon - } - enum = north - } - """ + open class `A Person` { + `first name`: String + } + """ .trimIndent() ) + + assertThat(kotlinCode) + .compilesSuccessfully() + .contains( + """ + | open class `A Person`( + | open val `first name`: String + | ) + """ + .trimMargin() + ) + .contains( + """ + | override fun toString(): String = ""${'"'}A Person(first name=${'$'}`first name`)""${'"'} + """ + .trimMargin() + ) + .contains( + """ + | open fun copy(`first name`: String = this.`first name`): `A Person` = `A Person`(`first name`) + """ + .trimMargin() + ) } @Test @@ -1816,7 +1803,7 @@ class KotlinCodeGeneratorTest { @Test fun `override names in a standalone module`() { val files = - KotlinCodegenOptions( + KotlinCodeGeneratorOptions( renames = mapOf("a.b.c" to "x.y.z", "d.e.f.AnotherModule" to "u.v.w.RenamedModule") ) .generateFiles( @@ -1851,7 +1838,7 @@ class KotlinCodeGeneratorTest { @Test fun `override names based on the longest prefix`() { val files = - KotlinCodegenOptions( + KotlinCodeGeneratorOptions( renames = mapOf("com.foo.bar." to "x.", "com.foo." to "y.", "com." to "z.", "" to "w.") ) .generateFiles( @@ -1897,7 +1884,7 @@ class KotlinCodeGeneratorTest { @Test fun `override names in multiple modules using each other`() { val files = - KotlinCodegenOptions( + KotlinCodeGeneratorOptions( renames = mapOf( "org.foo" to "com.foo.x", @@ -1980,7 +1967,7 @@ class KotlinCodeGeneratorTest { @Test fun `do not capitalize names of renamed classes`() { val files = - KotlinCodegenOptions( + KotlinCodeGeneratorOptions( renames = mapOf("a.b.c.MyModule" to "x.y.z.renamed_module", "d.e.f." to "u.v.w.") ) .generateFiles( @@ -2024,7 +2011,7 @@ class KotlinCodeGeneratorTest { assertThat(files).isEmpty() } - private fun KotlinCodegenOptions.generateFiles( + private fun KotlinCodeGeneratorOptions.generateFiles( vararg pklModules: PklModule ): Map { val pklFiles = pklModules.map { it.writeToDisk(tempDir.resolve("pkl/${it.name}.pkl")) } @@ -2036,13 +2023,13 @@ class KotlinCodeGeneratorTest { } } - private fun KotlinCodegenOptions.generateFiles( + private fun KotlinCodeGeneratorOptions.generateFiles( vararg pklModules: kotlin.Pair ): Map = generateFiles(*pklModules.map { (name, text) -> PklModule(name, text) }.toTypedArray()) private fun generateFiles(vararg pklModules: PklModule): Map = - KotlinCodegenOptions().generateFiles(*pklModules).mapValues { KotlinSourceCode(it.value) } + KotlinCodeGeneratorOptions().generateFiles(*pklModules).mapValues { KotlinSourceCode(it.value) } private fun instantiateOtherAndPropertyTypes(): kotlin.Pair { val otherCtor = propertyTypesClasses.getValue("Other").constructors.first() diff --git a/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Inheritance.kotlin b/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Inheritance.kotlin index d560090a..dbe5d9d2 100644 --- a/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Inheritance.kotlin +++ b/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Inheritance.kotlin @@ -6,23 +6,9 @@ import kotlin.Boolean import kotlin.Int import kotlin.Long import kotlin.String -import kotlin.text.StringBuilder import org.pkl.core.Duration object Mod { - private fun appendProperty( - builder: StringBuilder, - name: String, - value: Any? - ) { - builder.append("\n ").append(name).append(" = ") - val lines = value.toString().split("\n") - builder.append(lines[0]) - for (i in 1..lines.lastIndex) { - builder.append("\n ").append(lines[i]) - } - } - open class Foo( open val one: Long ) { @@ -42,13 +28,7 @@ object Mod { return result } - override fun toString(): String { - val builder = StringBuilder(100) - builder.append(Foo::class.java.simpleName).append(" {") - appendProperty(builder, "one", this.one) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = """Foo(one=$one)""" } open class None( @@ -70,13 +50,7 @@ object Mod { return result } - override fun toString(): String { - val builder = StringBuilder(100) - builder.append(None::class.java.simpleName).append(" {") - appendProperty(builder, "one", this.one) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = """None(one=$one)""" } open class Bar( @@ -103,14 +77,7 @@ object Mod { return result } - override fun toString(): String { - val builder = StringBuilder(150) - builder.append(Bar::class.java.simpleName).append(" {") - appendProperty(builder, "one", this.one) - appendProperty(builder, "two", this.two) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = """Bar(one=$one, two=$two)""" } class Baz( @@ -146,14 +113,6 @@ object Mod { return result } - override fun toString(): String { - val builder = StringBuilder(200) - builder.append(Baz::class.java.simpleName).append(" {") - appendProperty(builder, "one", this.one) - appendProperty(builder, "two", this.two) - appendProperty(builder, "three", this.three) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = """Baz(one=$one, two=$two, three=$three)""" } } diff --git a/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Kdoc.kotlin b/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Kdoc.kotlin index f1284de6..a8cb60b1 100644 --- a/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Kdoc.kotlin +++ b/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/Kdoc.kotlin @@ -5,7 +5,6 @@ import kotlin.Any import kotlin.Boolean import kotlin.Int import kotlin.String -import kotlin.text.StringBuilder /** * type alias comment. @@ -51,13 +50,7 @@ data class Mod( return result } - override fun toString(): String { - val builder = StringBuilder(100) - builder.append(Product::class.java.simpleName).append(" {") - appendProperty(builder, "price", this.price) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = """Product(price=$price)""" } /** @@ -71,19 +64,4 @@ data class Mod( */ val name: String ) - - companion object { - private fun appendProperty( - builder: StringBuilder, - name: String, - value: Any? - ) { - builder.append("\n ").append(name).append(" = ") - val lines = value.toString().split("\n") - builder.append(lines[0]) - for (i in 1..lines.lastIndex) { - builder.append("\n ").append(lines[i]) - } - } - } } diff --git a/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/PropertyTypes.kotlin b/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/PropertyTypes.kotlin index a3bca435..df94e480 100644 --- a/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/PropertyTypes.kotlin +++ b/pkl-codegen-kotlin/src/test/resources/org/pkl/codegen/kotlin/PropertyTypes.kotlin @@ -13,26 +13,12 @@ import kotlin.collections.List import kotlin.collections.Map import kotlin.collections.Set import kotlin.text.Regex -import kotlin.text.StringBuilder import org.pkl.core.DataSize import org.pkl.core.DataSizeUnit import org.pkl.core.Duration import org.pkl.core.DurationUnit object Mod { - private fun appendProperty( - builder: StringBuilder, - name: String, - value: Any? - ) { - builder.append("\n ").append(name).append(" = ") - val lines = value.toString().split("\n") - builder.append(lines[0]) - for (i in 1..lines.lastIndex) { - builder.append("\n ").append(lines[i]) - } - } - open class PropertyTypes( open val boolean: Boolean, open val int: Long, @@ -160,39 +146,8 @@ object Mod { return result } - override fun toString(): String { - val builder = StringBuilder(1400) - builder.append(PropertyTypes::class.java.simpleName).append(" {") - appendProperty(builder, "boolean", this.boolean) - appendProperty(builder, "int", this.int) - appendProperty(builder, "float", this.float) - appendProperty(builder, "string", this.string) - appendProperty(builder, "duration", this.duration) - appendProperty(builder, "durationUnit", this.durationUnit) - appendProperty(builder, "dataSize", this.dataSize) - appendProperty(builder, "dataSizeUnit", this.dataSizeUnit) - appendProperty(builder, "nullable", this.nullable) - appendProperty(builder, "nullable2", this.nullable2) - appendProperty(builder, "pair", this.pair) - appendProperty(builder, "pair2", this.pair2) - appendProperty(builder, "coll", this.coll) - appendProperty(builder, "coll2", this.coll2) - appendProperty(builder, "list", this.list) - appendProperty(builder, "list2", this.list2) - appendProperty(builder, "set", this.set) - appendProperty(builder, "set2", this.set2) - appendProperty(builder, "map", this.map) - appendProperty(builder, "map2", this.map2) - appendProperty(builder, "container", this.container) - appendProperty(builder, "container2", this.container2) - appendProperty(builder, "other", this.other) - appendProperty(builder, "regex", this.regex) - appendProperty(builder, "any", this.any) - appendProperty(builder, "nonNull", this.nonNull) - appendProperty(builder, "enum", this.enum) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = + """PropertyTypes(boolean=$boolean, int=$int, float=$float, string=$string, duration=$duration, durationUnit=$durationUnit, dataSize=$dataSize, dataSizeUnit=$dataSizeUnit, nullable=$nullable, nullable2=$nullable2, pair=$pair, pair2=$pair2, coll=$coll, coll2=$coll2, list=$list, list2=$list2, set=$set, set2=$set2, map=$map, map2=$map2, container=$container, container2=$container2, other=$other, regex=$regex, any=$any, nonNull=$nonNull, enum=$enum)""" } open class Other( @@ -214,13 +169,7 @@ object Mod { return result } - override fun toString(): String { - val builder = StringBuilder(100) - builder.append(Other::class.java.simpleName).append(" {") - appendProperty(builder, "name", this.name) - builder.append("\n}") - return builder.toString() - } + override fun toString(): String = """Other(name=$name)""" } enum class Direction(