From 7b97a1443a6f135c4bdb85f544f80ce00440a65a Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Fri, 21 Jun 2024 13:34:38 -0400 Subject: [PATCH 1/6] KSP port of ForwardingClassGenerator --- .../codegen/api/ForwardingClassGenerator.java | 238 ++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java new file mode 100644 index 000000000..68e68e3a5 --- /dev/null +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java @@ -0,0 +1,238 @@ +/* + * Copyright 2022 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.squareup.moshi.kotlin.codegen.api; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.ksp.symbol.*; +import com.squareup.kotlinpoet.ClassName; +import com.squareup.kotlinpoet.ksp.KsClassDeclarationsKt; +import kotlin.NotImplementedError; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.MethodVisitor; + +import static com.squareup.kotlinpoet.TypeNames.DOUBLE; +import static com.squareup.kotlinpoet.TypeNames.FLOAT; +import static com.squareup.kotlinpoet.TypeNames.LONG; +import static com.squareup.kotlinpoet.TypeNames.*; +import static java.util.stream.Collectors.joining; +import static org.objectweb.asm.ClassWriter.COMPUTE_MAXS; +import static org.objectweb.asm.Opcodes.*; + +/** + * Generates a class that invokes the constructor of another class. + * + *

The point here is that the constructor might be synthetic, in which case it can't be called + * directly from Java source code. Say we want to call the constructor {@code ConstructMe(int, + * String, long)} with parameters {@code 1, "2", 3L}. If the constructor is synthetic, then Java + * source code can't just do {@code new ConstructMe(1, "2", 3L)}. So this class allows you to + * generate a class file, say {@code Forwarder}, that is basically what you would get if you could + * compile this: + * + * 

+ * final class Forwarder {
+ *   private Forwarder() {}
+ *
+ *   static ConstructMe of(int a, String b, long c) {
+ *     return new ConstructMe(a, b, c);
+ *   }
+ * }
+ *
+ * + *

Because the class file is assembled directly, rather than being produced by the Java compiler, + * it can call the synthetic constructor. Then regular Java source code can do {@code + * Forwarder.of(1, "2", 3L)} to call the constructor. + */ +final class ForwardingClassGenerator { + /** + * Assembles a class with a static method {@code of} that calls the constructor of another class + * with the same parameters. + * + *

It would be simpler if we could just pass in an {@code ExecutableElement} representing the + * constructor, but if it is synthetic then it won't be visible to the {@code javax.lang.model} + * APIs. So we have to pass the constructed type and the constructor parameter types separately. + * + * @param forwardingClassName the fully-qualified name of the class to generate + * @param classToConstruct the type whose constructor will be invoked ({@code ConstructMe} in the + * example above) + * @param constructorParameters the erased types of the constructor parameters, which will also be + * the types of the generated {@code of} method. We require the types to be erased so as not + * to require an instance of the {@code Types} interface to erase them here. Having to deal + * with generics would complicate things unnecessarily. + * @return a byte array making up the new class file + */ + static byte[] makeConstructorForwarder( + String forwardingClassName, + KSClassDeclaration classToConstruct, + ImmutableList constructorParameters) { + + ClassWriter classWriter = new ClassWriter(COMPUTE_MAXS); + classWriter.visit( + V1_8, + ACC_FINAL | ACC_SUPER, + internalName(forwardingClassName), + null, + "java/lang/Object", + null); + classWriter.visitSource(forwardingClassName, null); + + // Generate the `of` method. + // TODO(emcmanus): cleaner generics. If we're constructing Foo then we should + // generate a generic signature for the `of` method, as if the Java declaration were this: + // static Foo of(...) + // Currently we just generate: + // static Foo of(...) + // which returns the raw Foo type. + String parameterSignature = + constructorParameters.stream() + .map(ForwardingClassGenerator::signatureEncoding) + .collect(joining("")); + String internalClassToConstruct = internalName(KsClassDeclarationsKt.toClassName(classToConstruct)); + String ofMethodSignature = "(" + parameterSignature + ")L" + internalClassToConstruct + ";"; + MethodVisitor ofMethodVisitor = + classWriter.visitMethod(ACC_STATIC, "of", ofMethodSignature, null, null); + ofMethodVisitor.visitCode(); + + // The remaining instructions are basically what ASMifier generates for a class like the + // `Forwarder` class in the example above. + ofMethodVisitor.visitTypeInsn(NEW, internalClassToConstruct); + ofMethodVisitor.visitInsn(DUP); + + int local = 0; + for (KSType type : constructorParameters) { + ofMethodVisitor.visitVarInsn(loadInstruction(type), local); + local += localSize(type); + } + String constructorToCallSignature = "(" + parameterSignature + ")V"; + ofMethodVisitor.visitMethodInsn( + INVOKESPECIAL, + internalClassToConstruct, + "", + constructorToCallSignature, + /* isInterface= */ false); + + ofMethodVisitor.visitInsn(ARETURN); + ofMethodVisitor.visitMaxs(0, 0); + ofMethodVisitor.visitEnd(); + classWriter.visitEnd(); + return classWriter.toByteArray(); + } + + /** The bytecode instruction that copies a parameter of the given type onto the JVM stack. */ + private static int loadInstruction(KSType type) { + if (type.isMarkedNullable()) { + // Always using a boxed type + return ALOAD; + } + ClassName rawType = rawType(type); + if (rawType.equals(ARRAY)) { + return ALOAD; + } else if (rawType.equals(LONG)) { + return LLOAD; + } else if (rawType.equals(FLOAT)) { + return FLOAD; + } else if (rawType.equals(DOUBLE)) { + return DLOAD; + } else if (rawType.equals(BYTE)) { + // These are all represented as int local variables. + return ILOAD; + } else if (rawType.equals(SHORT)) { + return ILOAD; + } else if (rawType.equals(INT)) { + return ILOAD; + } else if (rawType.equals(CHAR)) { + return ILOAD; + } else if (rawType.equals(BOOLEAN)) { + return ILOAD; + } else { + // Declared type + return ALOAD; + } + } + + /** + * The size in the local variable array of a value of the given type. A quirk of the JVM means + * that long and double variables each take up two consecutive slots in the local variable array. + * (The first n local variables are the parameters, so we need to know their sizes when iterating + * over them.) + */ + private static int localSize(KSType type) { + // Nullable always uses the boxed type + if (type.isMarkedNullable()) return 1; + ClassName rawType = rawType(type); + if (rawType.equals(LONG) || rawType.equals(DOUBLE)) { + return 2; + } else { + return 1; + } + } + + private static String internalName(String className) { + return className.replace('.', '/'); + } + + /** + * Given a class like {@code foo.bar.Outer.Inner}, produces a string like {@code + * "foo/bar/Outer$Inner"}, which is the way the class is referenced in the JVM. + */ + private static String internalName(ClassName className) { + return className.reflectionName(); + } + + private static ClassName rawType(KSType type) { + // TODO handle other KSTypes + return KsClassDeclarationsKt.toClassName((KSClassDeclaration) type.getDeclaration()); + } + + private static String signatureEncoding(KSType type) { + KSDeclaration declaration = type.getDeclaration(); + if (declaration instanceof KSClassDeclaration) { + ClassName rawType = KsClassDeclarationsKt.toClassName((KSClassDeclaration) declaration); + if (rawType.equals(ARRAY)) { + KSType componentType = type.getArguments().get(0).getType().resolve(); + return "[" + signatureEncoding(componentType); + } else if (rawType.equals(BYTE)) { + return "B"; + } else if (rawType.equals(SHORT)) { + return "S"; + } else if (rawType.equals(INT)) { + return "I"; + } else if (rawType.equals(LONG)) { + return "J"; + } else if (rawType.equals(FLOAT)) { + return "F"; + } else if (rawType.equals(DOUBLE)) { + return "D"; + } else if (rawType.equals(CHAR)) { + return "C"; + } else if (rawType.equals(BOOLEAN)) { + return "Z"; + } else { + // Declared type + return "L" + internalName(rawType) + ";"; + } + } else if (declaration instanceof KSTypeAlias) { + // TODO + throw new NotImplementedError("Type alias not supported"); + } else if (declaration instanceof KSTypeParameter) { + // TODO + throw new NotImplementedError("Type parameter not supported"); + } else { + throw new IllegalArgumentException("Unexpected type " + type); + } + } + + private ForwardingClassGenerator() {} +} From b2c85fa2a2ac0405926210c3f87609d4370335ed Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 22 Jun 2024 17:45:00 -0400 Subject: [PATCH 2/6] Small inspection tweak --- .../java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt index de58602ac..65738c28b 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt @@ -180,7 +180,7 @@ private fun List<*>.toArray(method: Method, valueProvider: (Any) -> Any): Array< method.returnType.componentType, this.size, ) as Array - for (r in 0 until this.size) { + for (r in indices) { array[r] = this[r]?.let { valueProvider.invoke(it) } } return array From 20dd1004fc241dc845896692b3e955159376cef1 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 22 Jun 2024 17:48:41 -0400 Subject: [PATCH 3/6] Update for Moshi needs --- .../codegen/api/ForwardingClassGenerator.java | 188 ++++++------------ 1 file changed, 66 insertions(+), 122 deletions(-) diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java index 68e68e3a5..1ea9d422a 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ForwardingClassGenerator.java @@ -15,21 +15,25 @@ */ package com.squareup.moshi.kotlin.codegen.api; -import com.google.common.collect.ImmutableList; -import com.google.devtools.ksp.symbol.*; -import com.squareup.kotlinpoet.ClassName; -import com.squareup.kotlinpoet.ksp.KsClassDeclarationsKt; -import kotlin.NotImplementedError; +import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Type; -import static com.squareup.kotlinpoet.TypeNames.DOUBLE; -import static com.squareup.kotlinpoet.TypeNames.FLOAT; -import static com.squareup.kotlinpoet.TypeNames.LONG; -import static com.squareup.kotlinpoet.TypeNames.*; -import static java.util.stream.Collectors.joining; import static org.objectweb.asm.ClassWriter.COMPUTE_MAXS; -import static org.objectweb.asm.Opcodes.*; +import static org.objectweb.asm.Opcodes.ACC_FINAL; +import static org.objectweb.asm.Opcodes.ACC_STATIC; +import static org.objectweb.asm.Opcodes.ACC_SUPER; +import static org.objectweb.asm.Opcodes.ALOAD; +import static org.objectweb.asm.Opcodes.ARETURN; +import static org.objectweb.asm.Opcodes.DLOAD; +import static org.objectweb.asm.Opcodes.DUP; +import static org.objectweb.asm.Opcodes.FLOAD; +import static org.objectweb.asm.Opcodes.ILOAD; +import static org.objectweb.asm.Opcodes.INVOKESPECIAL; +import static org.objectweb.asm.Opcodes.LLOAD; +import static org.objectweb.asm.Opcodes.NEW; +import static org.objectweb.asm.Opcodes.V1_8; /** * Generates a class that invokes the constructor of another class. @@ -65,18 +69,18 @@ final class ForwardingClassGenerator { * APIs. So we have to pass the constructed type and the constructor parameter types separately. * * @param forwardingClassName the fully-qualified name of the class to generate - * @param classToConstruct the type whose constructor will be invoked ({@code ConstructMe} in the + * @param targetClass the internal name of the type whose constructor will be invoked ({@code ConstructMe} in the * example above) - * @param constructorParameters the erased types of the constructor parameters, which will also be - * the types of the generated {@code of} method. We require the types to be erased so as not - * to require an instance of the {@code Types} interface to erase them here. Having to deal - * with generics would complicate things unnecessarily. - * @return a byte array making up the new class file + * @param parameterDescriptor the jvm parameters descriptor of the target class constructor to invoke. Should include the bit fields and default constructor marker. + * @param creatorSignature the jvm signature (not descriptor) of the to-be-generated "of" creator method. Should include generics information. + * @return a byte array making up the new class file. */ static byte[] makeConstructorForwarder( String forwardingClassName, - KSClassDeclaration classToConstruct, - ImmutableList constructorParameters) { + String targetClass, + String parameterDescriptor, + String creatorSignature + ) { ClassWriter classWriter = new ClassWriter(COMPUTE_MAXS); classWriter.visit( @@ -88,21 +92,21 @@ static byte[] makeConstructorForwarder( null); classWriter.visitSource(forwardingClassName, null); - // Generate the `of` method. - // TODO(emcmanus): cleaner generics. If we're constructing Foo then we should - // generate a generic signature for the `of` method, as if the Java declaration were this: - // static Foo of(...) - // Currently we just generate: - // static Foo of(...) - // which returns the raw Foo type. - String parameterSignature = - constructorParameters.stream() - .map(ForwardingClassGenerator::signatureEncoding) - .collect(joining("")); - String internalClassToConstruct = internalName(KsClassDeclarationsKt.toClassName(classToConstruct)); - String ofMethodSignature = "(" + parameterSignature + ")L" + internalClassToConstruct + ";"; + String internalClassToConstruct = internalName(targetClass); + String ofMethodDescriptor = "(" + parameterDescriptor + ")L" + internalClassToConstruct + ";"; MethodVisitor ofMethodVisitor = - classWriter.visitMethod(ACC_STATIC, "of", ofMethodSignature, null, null); + classWriter.visitMethod( + ACC_STATIC, + /* name */ "of", + /* descriptor */ ofMethodDescriptor, + /* signature */ creatorSignature, + null + ); + + // Tell Kotlin we're always returning non-null and avoid a platform type warning. + AnnotationVisitor av = ofMethodVisitor.visitAnnotation("Lorg/jetbrains/annotations/NotNull;", true); + av.visitEnd(); + ofMethodVisitor.visitCode(); // The remaining instructions are basically what ASMifier generates for a class like the @@ -110,12 +114,13 @@ static byte[] makeConstructorForwarder( ofMethodVisitor.visitTypeInsn(NEW, internalClassToConstruct); ofMethodVisitor.visitInsn(DUP); + String constructorToCallSignature = "(" + parameterDescriptor + ")V"; + Type[] paramTypes = Type.getArgumentTypes(constructorToCallSignature); int local = 0; - for (KSType type : constructorParameters) { + for (Type type : paramTypes) { ofMethodVisitor.visitVarInsn(loadInstruction(type), local); local += localSize(type); } - String constructorToCallSignature = "(" + parameterSignature + ")V"; ofMethodVisitor.visitMethodInsn( INVOKESPECIAL, internalClassToConstruct, @@ -131,34 +136,25 @@ static byte[] makeConstructorForwarder( } /** The bytecode instruction that copies a parameter of the given type onto the JVM stack. */ - private static int loadInstruction(KSType type) { - if (type.isMarkedNullable()) { - // Always using a boxed type - return ALOAD; - } - ClassName rawType = rawType(type); - if (rawType.equals(ARRAY)) { - return ALOAD; - } else if (rawType.equals(LONG)) { - return LLOAD; - } else if (rawType.equals(FLOAT)) { - return FLOAD; - } else if (rawType.equals(DOUBLE)) { - return DLOAD; - } else if (rawType.equals(BYTE)) { - // These are all represented as int local variables. - return ILOAD; - } else if (rawType.equals(SHORT)) { - return ILOAD; - } else if (rawType.equals(INT)) { - return ILOAD; - } else if (rawType.equals(CHAR)) { - return ILOAD; - } else if (rawType.equals(BOOLEAN)) { - return ILOAD; - } else { - // Declared type - return ALOAD; + private static int loadInstruction(Type type) { + switch (type.getSort()) { + case Type.BOOLEAN: + case Type.CHAR: + case Type.BYTE: + case Type.SHORT: + case Type.INT: + return ILOAD; + case Type.FLOAT: + return FLOAD; + case Type.LONG: + return LLOAD; + case Type.DOUBLE: + return DLOAD; + case Type.ARRAY: + case Type.OBJECT: + return ALOAD; + default: + throw new IllegalArgumentException("Unexpected type " + type); } } @@ -168,14 +164,13 @@ private static int loadInstruction(KSType type) { * (The first n local variables are the parameters, so we need to know their sizes when iterating * over them.) */ - private static int localSize(KSType type) { - // Nullable always uses the boxed type - if (type.isMarkedNullable()) return 1; - ClassName rawType = rawType(type); - if (rawType.equals(LONG) || rawType.equals(DOUBLE)) { - return 2; - } else { - return 1; + private static int localSize(Type type) { + switch (type.getSort()) { + case Type.LONG: + case Type.DOUBLE: + return 2; + default: + return 1; } } @@ -183,56 +178,5 @@ private static String internalName(String className) { return className.replace('.', '/'); } - /** - * Given a class like {@code foo.bar.Outer.Inner}, produces a string like {@code - * "foo/bar/Outer$Inner"}, which is the way the class is referenced in the JVM. - */ - private static String internalName(ClassName className) { - return className.reflectionName(); - } - - private static ClassName rawType(KSType type) { - // TODO handle other KSTypes - return KsClassDeclarationsKt.toClassName((KSClassDeclaration) type.getDeclaration()); - } - - private static String signatureEncoding(KSType type) { - KSDeclaration declaration = type.getDeclaration(); - if (declaration instanceof KSClassDeclaration) { - ClassName rawType = KsClassDeclarationsKt.toClassName((KSClassDeclaration) declaration); - if (rawType.equals(ARRAY)) { - KSType componentType = type.getArguments().get(0).getType().resolve(); - return "[" + signatureEncoding(componentType); - } else if (rawType.equals(BYTE)) { - return "B"; - } else if (rawType.equals(SHORT)) { - return "S"; - } else if (rawType.equals(INT)) { - return "I"; - } else if (rawType.equals(LONG)) { - return "J"; - } else if (rawType.equals(FLOAT)) { - return "F"; - } else if (rawType.equals(DOUBLE)) { - return "D"; - } else if (rawType.equals(CHAR)) { - return "C"; - } else if (rawType.equals(BOOLEAN)) { - return "Z"; - } else { - // Declared type - return "L" + internalName(rawType) + ";"; - } - } else if (declaration instanceof KSTypeAlias) { - // TODO - throw new NotImplementedError("Type alias not supported"); - } else if (declaration instanceof KSTypeParameter) { - // TODO - throw new NotImplementedError("Type parameter not supported"); - } else { - throw new IllegalArgumentException("Unexpected type " + type); - } - } - private ForwardingClassGenerator() {} } From 68bb597ebf8970ba440d960dc12335a10d2714aa Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 22 Jun 2024 17:49:15 -0400 Subject: [PATCH 4/6] Remove reflective construction bits from proguard rule gen --- .../moshi/kotlin/codegen/api/ProguardRules.kt | 31 ------------------- 1 file changed, 31 deletions(-) diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ProguardRules.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ProguardRules.kt index 76806fcba..e07679ef1 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ProguardRules.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/ProguardRules.kt @@ -22,8 +22,6 @@ import com.squareup.kotlinpoet.ClassName * - Keeping the target class name to Moshi's reflective lookup of the adapter. * - Keeping the generated adapter class name + public constructor for reflective lookup. * - Keeping any used JsonQualifier annotations and the properties they are attached to. - * - If the target class has default parameter values, also keeping the associated synthetic - * constructor as well as the DefaultConstructorMarker type Kotlin adds to it. * * Each rule is intended to be as specific and targeted as possible to reduce footprint, and each is * conditioned on usage of the original target type. @@ -37,8 +35,6 @@ public data class ProguardConfig( val targetClass: ClassName, val adapterName: String, val adapterConstructorParams: List, - val targetConstructorHasDefaults: Boolean, - val targetConstructorParams: List, ) { public fun outputFilePathWithoutExtension(canonicalName: String): String { return "META-INF/proguard/moshi-$canonicalName" @@ -66,33 +62,6 @@ public data class ProguardConfig( val constructorArgs = adapterConstructorParams.joinToString(",") appendLine(" public ($constructorArgs);") appendLine("}") - - if (targetConstructorHasDefaults) { - // If the target class has default parameter values, keep its synthetic constructor - // - // -keepnames class kotlin.jvm.internal.DefaultConstructorMarker - // -keepclassmembers @com.squareup.moshi.JsonClass @kotlin.Metadata class * { - // synthetic (...); - // } - // - appendLine("-if class $targetName") - appendLine("-keepnames class kotlin.jvm.internal.DefaultConstructorMarker") - appendLine("-if class $targetName") - appendLine("-keepclassmembers class $targetName {") - val allParams = targetConstructorParams.toMutableList() - val maskCount = if (targetConstructorParams.isEmpty()) { - 0 - } else { - (targetConstructorParams.size + 31) / 32 - } - repeat(maskCount) { - allParams += "int" - } - allParams += "kotlin.jvm.internal.DefaultConstructorMarker" - val params = allParams.joinToString(",") - appendLine(" public synthetic ($params);") - appendLine("}") - } } } From 7a71f1a3f6405827835102920531f68acc3fbf25 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 22 Jun 2024 17:50:36 -0400 Subject: [PATCH 5/6] Don't default primitives to null --- .../squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt index 62b58e133..f04bdc00d 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt @@ -56,10 +56,11 @@ public class PropertyGenerator( } internal fun generateLocalProperty(): PropertySpec { - return PropertySpec.builder(localName, target.type.copy(nullable = true)) + val isPrimitive = target.type.isPrimitive + return PropertySpec.builder(localName, target.type.copy(nullable = !isPrimitive)) .mutable(true) .apply { - if (hasConstructorDefault) { + if (hasConstructorDefault || isPrimitive) { // We default to the primitive default type, as reflectively invoking the constructor // without this (even though it's a throwaway) will fail argument type resolution in // the reflective invocation. From fbea5e69d469f82639278401d5682f8b74fab83d Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 22 Jun 2024 17:51:16 -0400 Subject: [PATCH 6/6] Call through to generated bridge classes instead --- .../kotlin/codegen/api/AdapterGenerator.kt | 132 ++++------------- .../kotlin/codegen/api/TargetConstructor.kt | 3 +- .../moshi/kotlin/codegen/api/kotlintypes.kt | 17 +++ .../ksp/JsonClassSymbolProcessorProvider.kt | 11 +- .../moshi/kotlin/codegen/ksp/TargetTypes.kt | 137 +++++++++++++++++- 5 files changed, 188 insertions(+), 112 deletions(-) diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt index 2fbadba52..31eda1367 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt @@ -18,15 +18,14 @@ package com.squareup.moshi.kotlin.codegen.api import com.squareup.kotlinpoet.ARRAY import com.squareup.kotlinpoet.AnnotationSpec import com.squareup.kotlinpoet.AnnotationSpec.UseSiteTarget.FILE +import com.squareup.kotlinpoet.ClassName import com.squareup.kotlinpoet.CodeBlock import com.squareup.kotlinpoet.FileSpec import com.squareup.kotlinpoet.FunSpec -import com.squareup.kotlinpoet.INT import com.squareup.kotlinpoet.KModifier import com.squareup.kotlinpoet.MemberName import com.squareup.kotlinpoet.NameAllocator import com.squareup.kotlinpoet.ParameterSpec -import com.squareup.kotlinpoet.ParameterizedTypeName import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy import com.squareup.kotlinpoet.PropertySpec import com.squareup.kotlinpoet.TypeName @@ -42,8 +41,8 @@ import com.squareup.moshi.Moshi import com.squareup.moshi.kotlin.codegen.api.FromJsonComponent.ParameterOnly import com.squareup.moshi.kotlin.codegen.api.FromJsonComponent.ParameterProperty import com.squareup.moshi.kotlin.codegen.api.FromJsonComponent.PropertyOnly -import java.lang.reflect.Constructor import java.lang.reflect.Type +import kotlin.jvm.internal.DefaultConstructorMarker import org.objectweb.asm.Type as AsmType private const val MOSHI_UTIL_PACKAGE = "com.squareup.moshi.internal" @@ -55,10 +54,10 @@ private const val TO_STRING_SIZE_BASE = TO_STRING_PREFIX.length + 1 // 1 is the public class AdapterGenerator( private val target: TargetType, private val propertyList: List, + private val generateForwardingClass: ((ClassName, ByteArray) -> Unit), ) { private companion object { - private val INT_TYPE_BLOCK = CodeBlock.of("%T::class.javaPrimitiveType!!", INT) private val DEFAULT_CONSTRUCTOR_MARKER_TYPE_BLOCK = CodeBlock.of( "%M!!", MemberName(MOSHI_UTIL_PACKAGE, "DEFAULT_CONSTRUCTOR_MARKER"), @@ -161,16 +160,6 @@ public class AdapterGenerator( ) .build() - private val constructorProperty = PropertySpec.builder( - nameAllocator.newName("constructorRef"), - Constructor::class.asClassName().parameterizedBy(originalTypeName).copy(nullable = true), - KModifier.PRIVATE, - ) - .addAnnotation(Volatile::class) - .mutable(true) - .initializer("null") - .build() - public fun prepare(generateProguardRules: Boolean, typeHook: (TypeSpec) -> TypeSpec = { it }): PreparedAdapter { val reservedSimpleNames = mutableSetOf() for (property in nonTransientProperties) { @@ -207,24 +196,10 @@ public class AdapterGenerator( else -> error("Unexpected number of arguments on primary constructor: $primaryConstructor") } - var hasDefaultProperties = false - var parameterTypes = emptyList() - target.constructor.signature?.let { constructorSignature -> - if (constructorSignature.startsWith("constructor-impl")) { - // Inline class, we don't support this yet. - // This is a static method with signature like 'constructor-impl(I)I' - return@let - } - hasDefaultProperties = propertyList.any { it.hasDefault } - parameterTypes = AsmType.getArgumentTypes(constructorSignature.removePrefix("")) - .map { it.toReflectionString() } - } return ProguardConfig( targetClass = className, adapterName = adapterName, adapterConstructorParams = adapterConstructorParams, - targetConstructorHasDefaults = hasDefaultProperties, - targetConstructorParams = parameterTypes, ) } @@ -287,7 +262,7 @@ public class AdapterGenerator( } result.addFunction(generateToStringFun()) - result.addFunction(generateFromJsonFun(result)) + result.addFunction(generateFromJsonFun()) result.addFunction(generateToJsonFun()) return result.build() @@ -321,7 +296,7 @@ public class AdapterGenerator( .build() } - private fun generateFromJsonFun(classBuilder: TypeSpec.Builder): FunSpec { + private fun generateFromJsonFun(): FunSpec { val result = FunSpec.builder("fromJson") .addModifiers(KModifier.OVERRIDE) .addParameter(readerParam) @@ -510,81 +485,35 @@ public class AdapterGenerator( CodeBlock.of("return·") } - // Used to indicate we're in an if-block that's assigning our result value and - // needs to be closed with endControlFlow - var closeNextControlFlowInAssignment = false - if (useDefaultsConstructor) { - // Happy path - all parameters with defaults are set - val allMasksAreSetBlock = maskNames.withIndex() - .map { (index, maskName) -> - CodeBlock.of("$maskName·== 0x${Integer.toHexString(maskAllSetValues[index])}.toInt()") - } - .joinToCode("·&& ") - result.beginControlFlow("if (%L)", allMasksAreSetBlock) - result.addComment("All parameters with defaults are set, invoke the constructor directly") - result.addCode("«%L·%T(", returnOrResultAssignment, originalTypeName) - var localSeparator = "\n" - val paramsToSet = components.filterIsInstance() - .filterNot { it.property.isTransient } - - // Set all non-transient property parameters - for (input in paramsToSet) { - result.addCode(localSeparator) - val property = input.property - result.addCode("%N = %N", property.name, property.localName) - if (property.isRequired) { - result.addMissingPropertyCheck(property, readerParam) - } else if (!input.type.isNullable) { - // Unfortunately incurs an intrinsic null-check even though we know it's set, but - // maybe in the future we can use contracts to omit them. - result.addCode("·as·%T", input.type) - } - localSeparator = ",\n" - } - result.addCode("\n»)\n") - result.nextControlFlow("else") - closeNextControlFlowInAssignment = true - - classBuilder.addProperty(constructorProperty) - result.addComment("Reflectively invoke the synthetic defaults constructor") + val forwardingClassName = ClassName(originalRawTypeName.packageName, "${adapterName}Forwarder") + + generateForwardingClass(forwardingClassName, + ForwardingClassGenerator.makeConstructorForwarder( + forwardingClassName.canonicalName, + originalRawTypeName.reflectionName(), + // TODO omit the marker? + target.constructor.descriptor!!.let { + val parameterSignature = it.substringAfter('(').substringBeforeLast(')') + parameterSignature + maskNames.joinToString("") { "I" } + "L${DefaultConstructorMarker::class.asClassName().reflectionName().replace('.', '/')};" + }, + target.constructor.creatorSignature, + )) + + result.addComment("Invoke the synthetic defaults constructor") // Dynamic default constructor call - val nonNullConstructorType = constructorProperty.type.copy(nullable = false) - val args = constructorPropertyTypes - .plus(0.until(maskCount).map { INT_TYPE_BLOCK }) // Masks, one every 32 params - .plus(DEFAULT_CONSTRUCTOR_MARKER_TYPE_BLOCK) // Default constructor marker is always last - .joinToCode(", ") - val coreLookupBlock = CodeBlock.of( - "%T::class.java.getDeclaredConstructor(%L)", - originalRawTypeName, - args, - ) - val lookupBlock = if (originalTypeName is ParameterizedTypeName) { - CodeBlock.of("(%L·as·%T)", coreLookupBlock, nonNullConstructorType) + // TODO is this necessary? Think the IDE can just infer this directly + val possibleGeneric = if (typeVariables.isNotEmpty()) { + // TODO support TypeParameterName as %N + CodeBlock.of("<%L>", typeVariables.joinToCode { CodeBlock.of(it.name) }) } else { - coreLookupBlock + CodeBlock.of("") } - val initializerBlock = CodeBlock.of( - "this.%1N·?: %2L.also·{ this.%1N·= it }", - constructorProperty, - lookupBlock, - ) - val localConstructorProperty = PropertySpec.builder( - nameAllocator.newName("localConstructor"), - nonNullConstructorType, - ) - .addAnnotation( - AnnotationSpec.builder(Suppress::class) - .addMember("%S", "UNCHECKED_CAST") - .build(), - ) - .initializer(initializerBlock) - .build() - result.addCode("%L", localConstructorProperty) result.addCode( - "«%L%N.newInstance(", + "«%L%T.of%L(", returnOrResultAssignment, - localConstructorProperty, + forwardingClassName, + possibleGeneric, ) } else { // Standard constructor call. Don't omit generics for parameterized types even if they can be @@ -632,11 +561,6 @@ public class AdapterGenerator( result.addCode("\n»)\n") - // Close the result assignment control flow, if any - if (closeNextControlFlowInAssignment) { - result.endControlFlow() - } - // Assign properties not present in the constructor. for (property in nonTransientProperties) { if (property.hasConstructorParameter) { diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/TargetConstructor.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/TargetConstructor.kt index 926d0c818..a7697d980 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/TargetConstructor.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/TargetConstructor.kt @@ -22,7 +22,8 @@ import com.squareup.kotlinpoet.KModifier public data class TargetConstructor( val parameters: LinkedHashMap, val visibility: KModifier, - val signature: String?, + val descriptor: String?, + val creatorSignature: String?, ) { init { visibility.checkIsVisibility() diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt index a43d52d82..58ca634e3 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt @@ -69,6 +69,17 @@ internal fun TypeName.findRawType(): ClassName? { } } +internal val PRIMITIVES = mapOf( + BOOLEAN to 'Z', + CHAR to 'C', + BYTE to 'B', + SHORT to 'S', + INT to 'I', + FLOAT to 'F', + LONG to 'J', + DOUBLE to 'D', +) + internal fun TypeName.defaultPrimitiveValue(): CodeBlock = when (this) { BOOLEAN -> CodeBlock.of("false") @@ -83,6 +94,9 @@ internal fun TypeName.defaultPrimitiveValue(): CodeBlock = else -> CodeBlock.of("null") } +internal val TypeName.isPrimitive: Boolean + get() = this in PRIMITIVES + @OptIn(DelicateKotlinPoetApi::class) internal fun TypeName.asTypeBlock(): CodeBlock { if (annotations.isNotEmpty()) { @@ -243,3 +257,6 @@ internal fun List.toTypeVariableResolver( return resolver } + +internal val ClassName.internalName: String + get() = reflectionName().replace('.', '/') diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt index 3df24d63a..b1d0202bc 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt @@ -142,7 +142,16 @@ private class JsonClassSymbolProcessor( } } - return AdapterGenerator(type, sortedProperties) + return AdapterGenerator(type, sortedProperties) { className, bytes -> + codeGenerator.createNewFile( + dependencies = Dependencies(aggregating = false, originalType.containingFile!!), + packageName = className.packageName, + fileName = className.simpleName, + extensionName = "class", + ).buffered().use { writer -> + writer.write(bytes) + } + } } } diff --git a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt index 7f5aa8643..105ce0aec 100644 --- a/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt +++ b/moshi-kotlin-codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt @@ -33,12 +33,16 @@ import com.google.devtools.ksp.symbol.KSType import com.google.devtools.ksp.symbol.KSTypeParameter import com.google.devtools.ksp.symbol.Modifier import com.google.devtools.ksp.symbol.Origin +import com.squareup.kotlinpoet.ANY import com.squareup.kotlinpoet.AnnotationSpec import com.squareup.kotlinpoet.ClassName import com.squareup.kotlinpoet.KModifier +import com.squareup.kotlinpoet.ParameterizedTypeName import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy import com.squareup.kotlinpoet.PropertySpec import com.squareup.kotlinpoet.TypeName +import com.squareup.kotlinpoet.TypeVariableName +import com.squareup.kotlinpoet.WildcardTypeName import com.squareup.kotlinpoet.ksp.TypeParameterResolver import com.squareup.kotlinpoet.ksp.toClassName import com.squareup.kotlinpoet.ksp.toKModifier @@ -51,7 +55,13 @@ import com.squareup.moshi.kotlin.codegen.api.TargetConstructor import com.squareup.moshi.kotlin.codegen.api.TargetParameter import com.squareup.moshi.kotlin.codegen.api.TargetProperty import com.squareup.moshi.kotlin.codegen.api.TargetType +import com.squareup.moshi.kotlin.codegen.api.internalName +import com.squareup.moshi.kotlin.codegen.api.rawType import com.squareup.moshi.kotlin.codegen.api.unwrapTypeAlias +import kotlin.jvm.internal.DefaultConstructorMarker +import org.objectweb.asm.Type +import org.objectweb.asm.signature.SignatureWriter + /** Returns a target type for [type] or null if it cannot be used with code gen. */ internal fun targetType( @@ -164,6 +174,42 @@ private fun ClassName.withTypeArguments(arguments: List): TypeName { } } +private fun TypeVariableName.isImplicitBound(): Boolean { + return bounds.size == 1 && bounds[0] == ANY.copy(nullable = true) +} + +@OptIn(KspExperimental::class) +private fun TypeName.mapToJava(resolver: Resolver): TypeName { + return when (this) { + is ClassName -> { + resolver.mapKotlinNameToJava(resolver.getKSNameFromString(canonicalName))?.asString()?.let(ClassName::bestGuess) ?: this + } + + is ParameterizedTypeName -> { + (rawType.mapToJava(resolver) as ClassName).parameterizedBy(typeArguments.map { it.mapToJava(resolver) }) + } + + is TypeVariableName -> { + if (isImplicitBound()) { + return this + } + this.copy(bounds = bounds.map { it.mapToJava(resolver) }) + } + + is WildcardTypeName -> { + if (outTypes.isNotEmpty() && inTypes.isEmpty()) { + WildcardTypeName.producerOf(outTypes[0].mapToJava(resolver)) + } else if (inTypes.isNotEmpty()) { + WildcardTypeName.consumerOf(inTypes[0].mapToJava(resolver)) + } else { + this + } + } + + else -> throw UnsupportedOperationException("Parameter with type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, or type variables are allowed.") + } +} + @OptIn(KspExperimental::class) internal fun primaryConstructor( resolver: Resolver, @@ -173,8 +219,73 @@ internal fun primaryConstructor( ): TargetConstructor? { val primaryConstructor = targetType.primaryConstructor ?: return null + val signatureWriter: SignatureWriter? = if (targetType.typeParameters.isEmpty()) { + null + } else { + SignatureWriter() + .apply { + for (typeParameter in targetType.typeParameters) { + val typeVarName = typeParameter.toTypeVariableName(typeParameterResolver) + visitFormalTypeParameter(typeVarName.name) + if (typeVarName.bounds.isNotEmpty()) { + visitClassBound().apply { + for (bound in typeVarName.bounds) { + // TODO what about parmameterized types? + visitClassType(bound.mapToJava(resolver).rawType().internalName) + } + visitEnd() + } + } + } + } + } + + val constructorSignature: String = resolver.mapToJvmSignature(primaryConstructor) + ?: run { + logger.error("No primary constructor found.", primaryConstructor) + return null + } + + val asmParamTypes = Type.getArgumentTypes(constructorSignature) + + var hasDefaultParams = false val parameters = LinkedHashMap() for ((index, parameter) in primaryConstructor.parameters.withIndex()) { + if (parameter.hasDefault) { + hasDefaultParams = true + } + + val type = parameter.type.toTypeName(typeParameterResolver) + val asmType = asmParamTypes[index] + if (type is TypeVariableName) { + // Add the name to the signature + signatureWriter?.visitParameterType()?.visitTypeVariable(type.name) + } else { + // Add the type from asm types to the signature + signatureWriter?.visitParameterType()?.apply { + when (asmType.sort) { + Type.ARRAY -> { + // TODO +// visitArrayType() +// visitTypeArgument() +// visitClassType(asmType.elementType) +// visitEnd() + } + + Type.OBJECT -> { + // TODO generics? Not sure it's necessary though + visitClassType(asmType.internalName) + visitEnd() + } + + else -> { + // Primitive + visitBaseType(asmType.descriptor[0]) + } + } + } + } + val name = parameter.name!!.getShortName() parameters[name] = TargetParameter( name = name, @@ -185,16 +296,30 @@ internal fun primaryConstructor( jsonName = parameter.jsonName(), ) } - - val kmConstructorSignature: String = resolver.mapToJvmSignature(primaryConstructor) - ?: run { - logger.error("No primary constructor found.", primaryConstructor) - return null + if (hasDefaultParams) { + repeat((parameters.size + 31) / 32) { + signatureWriter?.visitParameterType()?.visitBaseType('I') } + signatureWriter?.visitParameterType()?.apply { + visitClassType(Type.getInternalName(DefaultConstructorMarker::class.java)) + visitEnd() + } + } + signatureWriter?.visitReturnType()?.apply { + visitClassType(targetType.toClassName().rawType().internalName) + // TODO nesting? + for (typeParameter in targetType.typeParameters) { + visitTypeArgument('=') + visitTypeVariable(typeParameter.name.asString()) + } + visitEnd() + } + return TargetConstructor( parameters, primaryConstructor.getVisibility().toKModifier() ?: KModifier.PUBLIC, - kmConstructorSignature, + constructorSignature, + signatureWriter?.toString(), ) }