Kotlin Code Gen module #435

ZacSweers · 2018-02-09T11:04:34Z

This is an initial implementation of a code gen module. Originally from https://github.com/hzsweers/CatchUp/tree/master/tooling/moshkt

There's three main components:

moshi-kotlin-codegen - the actually processor implementation
integration-test - An integration test. This contains ignored KotlinJsonAdapter tests (Jesse mentioned wanting them to be able to pass the same tests is the long term goal, so this gives something to chip at) and some basic data classes tests.
moshi-kotlin-codegen-runtime - A runtime artifact with the @MoshiSerializable annotation and its corresponding MoshiSerializableJsonAdapterFactory. The factory usage is optional, as one could write a factory generator if they wanted to.

Supported:

Data classes
@Json annotations
Kotlin language features like nullability and default values (it generates Kotlin code via KotlinPoet, so it can actually leverage these features)
If a companion object is specified on the source type, it generates an extension jsonAdapter() function onto it
Generics
Good chunk of Kotshi tests

Unimplemented:

Support for more than just data classes
JsonQualifier annotations

For data classes, it's been working swimmingly in CatchUp as well as @rharter's codebases. Code itself could probably use some cleaning up (there's plenty of TODOs left in), but its output seems to be working well so far.

CC @Takhion

Example:

@MoshiSerializable
data class Foo(
   @Json(name = "first_name") val firstName: String,
   @Json(name = "last_name") val lastName: String,
   val age: Int,
   val nationalities: List<String> = emptyList(),
   val weight: Float,
   val tattoos: Boolean = false,
   val race: String?,
   val hasChildren: Boolean = false,
   val favoriteFood: String? = null,
   val favoriteDrink: String? = "Water"
)

Generates

package com.squareup.moshi

import kotlin.Any
import kotlin.Boolean
import kotlin.Float
import kotlin.Int
import kotlin.String
import kotlin.collections.List

class Foo_JsonAdapter(moshi: Moshi) : JsonAdapter<Foo>() {
  private val string_Adapter: JsonAdapter<String> = moshi.adapter(String::class.java)

  private val int_Adapter: JsonAdapter<Int> = moshi.adapter(Int::class.java)

  private val list__string_Adapter: JsonAdapter<List<String>> = moshi.adapter<List<String>>(Types.newParameterizedType(List::class.java, String::class.java))

  private val float_Adapter: JsonAdapter<Float> = moshi.adapter(Float::class.java)

  private val boolean_Adapter: JsonAdapter<Boolean> = moshi.adapter(Boolean::class.java)

  override fun fromJson(reader: JsonReader): Foo? {
    if (reader.peek() == JsonReader.Token.NULL) {
      reader.nextNull<Any>()
    }
    lateinit var firstName: String
    lateinit var lastName: String
    var age = 0
    var nationalities: List<String>? = null
    var weight = 0.0f
    var tattoos: Boolean? = null
    var race: String? = null
    var hasChildren: Boolean? = null
    var favoriteFood: String? = null
    var favoriteDrink: String? = null
    reader.beginObject()
    while (reader.hasNext()) {
      when (reader.selectName(OPTIONS)) {
        0 -> firstName = string_Adapter.fromJson(reader)!!
        1 -> lastName = string_Adapter.fromJson(reader)!!
        2 -> age = int_Adapter.fromJson(reader)!!
        3 -> nationalities = list__string_Adapter.fromJson(reader)!!
        4 -> weight = float_Adapter.fromJson(reader)!!
        5 -> tattoos = boolean_Adapter.fromJson(reader)!!
        6 -> race = string_Adapter.fromJson(reader)
        7 -> hasChildren = boolean_Adapter.fromJson(reader)!!
        8 -> favoriteFood = string_Adapter.fromJson(reader)
        9 -> favoriteDrink = string_Adapter.fromJson(reader)
        -1 -> {
          // Unknown name, skip it
          reader.nextName()
          reader.skipValue()
        }
      }
    }
    reader.endObject()
    return Foo(firstName = firstName,
        lastName = lastName,
        age = age,
        weight = weight,
        race = race).let {
          it.copy(nationalities = nationalities ?: it.nationalities,
              tattoos = tattoos ?: it.tattoos,
              hasChildren = hasChildren ?: it.hasChildren,
              favoriteFood = favoriteFood ?: it.favoriteFood,
              favoriteDrink = favoriteDrink ?: it.favoriteDrink)
        }
  }

  override fun toJson(writer: JsonWriter, value: Foo?) {
    if (value == null) {
      writer.nullValue()
      return
    }
    writer.beginObject()
    writer.name("first_name")
    string_Adapter.toJson(writer, value.firstName)
    writer.name("last_name")
    string_Adapter.toJson(writer, value.lastName)
    writer.name("age")
    int_Adapter.toJson(writer, value.age)
    writer.name("nationalities")
    list__string_Adapter.toJson(writer, value.nationalities)
    writer.name("weight")
    float_Adapter.toJson(writer, value.weight)
    writer.name("tattoos")
    boolean_Adapter.toJson(writer, value.tattoos)
    if (value.race != null) {
      writer.name("race")
      string_Adapter.toJson(writer, value.race)
    }
    writer.name("hasChildren")
    boolean_Adapter.toJson(writer, value.hasChildren)
    if (value.favoriteFood != null) {
      writer.name("favoriteFood")
      string_Adapter.toJson(writer, value.favoriteFood)
    }
    if (value.favoriteDrink != null) {
      writer.name("favoriteDrink")
      string_Adapter.toJson(writer, value.favoriteDrink)
    }
    writer.endObject()
  }

  private companion object SelectOptions {
    private val OPTIONS: JsonReader.Options by lazy { JsonReader.Options.of("first_name", "last_name", "age", "nationalities", "weight", "tattoos", "race", "hasChildren", "favoriteFood", "favoriteDrink") }
  }
}

rharter · 2018-02-09T17:01:19Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+  private fun mavenGeneratedDir(adapterName: String): File {
+    // Hack since the maven plugin doesn't supply `kapt.kotlin.generated` option
+    // Bug filed at https://youtrack.jetbrains.com/issue/KT-22783
+    val file = filer.createSourceFile(adapterName).toUri().let(::File)


I was thinking about this more yesterday and realized that this only needs to be called once. KotlinPoet takes care of the fqcn path, so this is going to return the same file every time.

The only time that might be different is for different source sets (test, main, etc), but I think those will get different processors (please confirm), so that shouldn't matter.

Not sure I follow. Could you elaborate on the "called once" bit? Since we're only calling it "once" per file anyway 🤔

We call it once per file, but we're getting the root source directory from it, so it only needs to be called once per source set. KotlinPoet takes care of the package folders.

FabianTerhorst · 2018-02-11T03:58:49Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+        optionsCN,
+        PRIVATE)
+        .delegate(
+            "lazy { %T.of(${optionsByIndex.map { it.value.key }


Are you sure the not very costly operation of creating some constants is worth an anonymous inner class for each MoshiSerializable data class adapter?

I'm not! But creating options does quite a bit under the hood, and was hoping to gather feedback here

Classloading already makes this lazy. The explicitly lazy isn't needed.

ZacSweers · 2018-02-12T01:04:00Z

Big ol' update above.

Generics support
Lots of various bugfixes
Introduction of Kotshi's test code

Questions that need deciding before moving forward:
1 - Should this support kotshi’s @JsonDefaultValue pattern? Or to what extent? I’m not totally familiar with it, but on the face of it I’m not sure it adds anything we can’t handle with the current approach
2 - Should this default to/support configurating primitive adapters (nextBoolean() vs .adapter(Boolean::class.java))?
3 - Do we want toString() implementations for adapters?
4 - Kotshi will use @get:JvmName over @field:Json if both are present. I don’t think this is idiomatic if we’re reading/generating kotlin code though, and should just keep it simple with @Json. Not sure if I’m missing context here though
5 - Currently we're using lateinit for expected non-nullable/non-defaultable fields. Wondering if we should just make them all nullable properties and then check the non-nullable/non-defaultable ones at the end to collect a list of all the missing ones. Would line up with something Kotshi appears to do as well
6 - Having to use a kotlinpoet snapshot to pick up the Any? bounds fix, so this can't merge till the next release anyway. Opportunistically picked up the PropertySpec literals support as well

ZacSweers · 2018-02-12T01:05:55Z

Also this is up for grabs if anyone has any ideas: d4d4926

Talked with @Takhion a bit, but haven't been able to arrive at a good solution yet to match the constructor from kotlin to java. Right now I fell back to just grabbing the first constructor.

Takhion · 2018-02-12T11:11:04Z

haven't been able to arrive at a good solution yet to match the constructor from kotlin to java

Before this PR gets merged I'll release a new version of kotlin-metadata that fixes all edge cases related to that, thanks to the excellent detective work from @hzsweers 🕵️ 🙌

rharter · 2018-02-12T14:10:01Z

For 1, I would say no. Unless I'm misunderstanding the @JsonDefaultValue duplicates Kotlin default values and is used because those aren't easily exposed in the Java code. IMHO they make the code very messy and verbose, losing some of the benefit of Kotlin.

ZacSweers · 2018-02-12T14:57:57Z

toString - 36522bc

default value handling - Agreed. Talked with Jesse offline too. 881d446

Remove custom names test, per talking with Jesse we'll just go with simple @Json usage - 8126bd1

Decided against configuration for primitive adapters - fb7e514

Just leaves this bit:

5 - Currently we're using lateinit for expected non-nullable/non-defaultable fields. Wondering if we should just make them all nullable properties and then check the non-nullable/non-defaultable ones at the end to collect a list of all the missing ones. Would line up with something Kotshi appears to do as well

Takhion · 2018-02-12T15:43:10Z

About 5, my personal opinion is: absolutely 👍

ZacSweers · 2018-02-12T16:02:09Z

Good enough for me! I'll work on that. Was maybe a bit too excited about using local lateinit from kotlin 1.2 :)

JakeWharton · 2018-02-12T16:10:30Z

They're implemented the same way so it doesn't really matter.

ZacSweers · 2018-02-12T18:06:16Z

Implemented tracking missing props in 3fd6614

This makes all nullable handling for local properties the same, and removes defaults for primitives in the process. It simplifies the handling a lot, and leans on kotlin language features to take care of null handling (null checking and then throwing the lazily evaluated list of missing properties).

One minor change from what kotshi does - this reports the serialized name in the missing properties, not the property name. We could look at supporting this though if we want. It also throws JsonDataException rather than NPEs.

Result is a read block looks like this:

var string_: String? = null
var nullableString: String? = null
var integer: Int? = null
var nullableInt: Int? = null
var isBoolean: Boolean? = null
var isNullableBoolean: Boolean? = null
var aShort: Short? = null
var nullableShort: Short? = null
var aByte: Byte? = null
var nullableByte: Byte? = null
var aChar: Char? = null
var nullableChar: Char? = null
var list: List<String>? = null
var nestedList: List<Map<String, Set<String>>>? = null
var abstractProperty: String? = null
var customName: String? = null
var annotated: String? = null
var anotherAnnotated: String? = null
var genericClass: GenericClass<String, List<String>>? = null
reader.beginObject()
while (reader.hasNext()) {
    when (reader.selectName(OPTIONS)) {
        0 -> string_ = string_Adapter.fromJson(reader)
        1 -> nullableString = string_nullable_Adapter.fromJson(reader)
        2 -> integer = int_Adapter.fromJson(reader)
        3 -> nullableInt = int_nullable_Adapter.fromJson(reader)
        4 -> isBoolean = boolean_Adapter.fromJson(reader)
        5 -> isNullableBoolean = boolean_nullable_Adapter.fromJson(reader)
        6 -> aShort = short_Adapter.fromJson(reader)
        7 -> nullableShort = short_nullable_Adapter.fromJson(reader)
        8 -> aByte = byte_Adapter.fromJson(reader)
        9 -> nullableByte = byte_nullable_Adapter.fromJson(reader)
        10 -> aChar = char_Adapter.fromJson(reader)
        11 -> nullableChar = char_nullable_Adapter.fromJson(reader)
        12 -> list = list__string_Adapter.fromJson(reader)
        13 -> nestedList = list__map__string_set__string_Adapter.fromJson(reader)
        14 -> abstractProperty = string_Adapter.fromJson(reader)
        15 -> customName = string_Adapter.fromJson(reader)
        16 -> annotated = string_Adapter.fromJson(reader)
        17 -> anotherAnnotated = string_Adapter.fromJson(reader)
        18 -> genericClass = genericClass__string_list__string_Adapter.fromJson(reader)
        -1 -> {
            // Unknown name, skip it
            reader.nextName()
            reader.skipValue()
        }
    }
}
reader.endObject()
val missingArguments: () -> JsonDataException = {
        NAMES.filterIndexed { index, _ ->
            when (index) {
                0 -> string_ == null
                2 -> integer == null
                4 -> isBoolean == null
                6 -> aShort == null
                8 -> aByte == null
                10 -> aChar == null
                12 -> list == null
                13 -> nestedList == null
                14 -> abstractProperty == null
                15 -> customName == null
                16 -> annotated == null
                17 -> anotherAnnotated == null
                18 -> genericClass == null
                else -> false
            }
        }.let { JsonDataException("The following required properties were missing: ${it.joinToString()}") }
        }
if (string_ == null) {
    throw missingArguments()
}
if (integer == null) {
    throw missingArguments()
}
if (isBoolean == null) {
    throw missingArguments()
}
if (aShort == null) {
    throw missingArguments()
}
if (aByte == null) {
    throw missingArguments()
}
if (aChar == null) {
    throw missingArguments()
}
if (list == null) {
    throw missingArguments()
}
if (nestedList == null) {
    throw missingArguments()
}
if (abstractProperty == null) {
    throw missingArguments()
}
if (customName == null) {
    throw missingArguments()
}
if (annotated == null) {
    throw missingArguments()
}
if (anotherAnnotated == null) {
    throw missingArguments()
}
if (genericClass == null) {
    throw missingArguments()
}
return TestClass(string = string_,
        nullableString = nullableString,
        integer = integer,
        nullableInt = nullableInt,
        isBoolean = isBoolean,
        isNullableBoolean = isNullableBoolean,
        aShort = aShort,
        nullableShort = nullableShort,
        aByte = aByte,
        nullableByte = nullableByte,
        aChar = aChar,
        nullableChar = nullableChar,
        list = list,
        nestedList = nestedList,
        abstractProperty = abstractProperty,
        customName = customName,
        annotated = annotated,
        anotherAnnotated = anotherAnnotated,
        genericClass = genericClass)

ZacSweers · 2018-02-13T18:04:07Z

Implemented JsonQualifier support in e8427be! I made Types.createJsonQualifierImplementation() public to support it, let me know what you think of that.

Resulting code looks like this:

private val string_Adapter_for_WrappedInArray_WrappedInObject_Adapter: JsonAdapter<String> =
            moshi.adapter<String>(String::class.java, setOf(Types.createJsonQualifierImplementation(WrappedInArray::class.java), Types.createJsonQualifierImplementation(WrappedInObject::class.java))).nullSafe()

Pretty happy with the result - no extra methods, passes the kotshi tests. It does rely on proxies, but I'm not sure there's a way around that right now since kotlin classes don't support subclassing and the only alternative I can think of is to reflectively go yank them out of the constructor params at runtime 😷 .

Not sure what the CI failure is, maybe travis goofed? Works for me locally 🤔

Still no luck!

None of these are data classes tests right now, which is the only thing this supports right now

Egorand · 2018-02-19T21:36:03Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+
+    val originalTypeName = originalElement.asType().asTypeName()
+    val moshiName = "moshi".allocate()
+    val moshiParam = ParameterSpec.builder(moshiName, Moshi::class.asClassName()).build()


asClassName() looks redundant, you should be able to simply pass Moshi::class

Egorand · 2018-02-19T21:42:41Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+                  }
+            }
+            .beginControlFlow("-1 ->")
+            .addCode("// Unknown name, skip it.\n")


addComment() can add \\ and \n for you

Ahh good catch! 7525f89

ansman · 2018-02-21T16:43:37Z

kotlin-codegen/runtime/src/main/java/com/squareup/moshi/MoshiSerializable.kt

+@Target(CLASS)
+annotation class MoshiSerializable
+
+class MoshiSerializableFactory : JsonAdapter.Factory {


Why not make this an object?

I asked higher up about whether or not to make this class a singleton (lazy or otherwise), and we went with just making this instantiable. I don't really have an opinion though!

It’s consistent with what we did for reflection.
KotlinJsonAdapterFactory

NightlyNexus

any particular reason to [separate the Moshi.adapter change]

It's a new API that affects users outside of this change. Project watchers with use cases or opinions who might not be following this change might be interested in the general changes to Moshi's API. Also, this is a huge PR.
But, no big deal in this case.

I'll take a look a the generated code next week and post thoughts on the code gen stuff!

NightlyNexus · 2018-02-23T21:19:24Z

moshi/src/main/java/com/squareup/moshi/Moshi.java

+    for (Class<? extends Annotation> annotationType : annotationTypes) {
+      annotations.add(Types.createJsonQualifierImplementation(annotationType));
+    }
+    return adapter(type, annotations);


Collections.unmodifiableSet

NightlyNexus · 2018-02-23T21:23:19Z

moshi/src/main/java/com/squareup/moshi/Moshi.java

+  @CheckReturnValue
+  public <T> JsonAdapter<T> adapter(Type type, Class<? extends Annotation>... annotationTypes) {
+    if (annotationTypes.length == 1) {
+      return adapter(type,


maybe return adapter(type, annotationTypes[0])
It's interesting that we know annotationTypes.length is never 0 because of the existing method signature.

That's clever, and saves a bit of code duplication - 5ed2a58

NightlyNexus · 2018-02-23T21:39:02Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+                  qualifiers.isEmpty() -> "" to emptyArray()
+                  qualifiers.size == 1 -> {
+                    ", %${standardArgsSize}T::class.java" to arrayOf(
+                        qualifiers.first().annotationType.asTypeName())


this looks like it will run into ansman/kotshi#60 (same below)

Interesting. Definitely interested in seeing Moshi support that case

Moshi does support it. Both the KotlinJsonAdapter and the ClassJsonAdapter hand off the annotation instances to adapter(Type, Set...).

that's via reflectively pulling off the annotation instances, whereas the idea here was to avoid that since we know them upfront

This PR does adapter(Type, Class<?>). This will create a new annotation instance that loses the declared annotation elements.

I see what you mean, but I'm confused on what you mean by Moshi does support it above? One way I could envision that case working here is by reflectively pulling them off the parameters at runtime, as kotlin annotations don't allow for something like AutoAnnotation since they're final :/ .

The other, albeit possibly crazy way, is to generate proxy invocation handlers that include the statically defined data (kind of our own inline kotlin-ified autoannotation with proxies)

NightlyNexus · 2018-02-23T21:47:04Z

moshi/src/main/java/com/squareup/moshi/Moshi.java

@@ -70,6 +71,19 @@
        Collections.singleton(Types.createJsonQualifierImplementation(annotationType)));
  }

+  @CheckReturnValue
+  public <T> JsonAdapter<T> adapter(Type type, Class<? extends Annotation>... annotationTypes) {


in light of my comment on needing the JsonQualifier elements, i'm hoping this isn't needed, at least for this PR.

As in you'd prefer to see this API removed? Or that you're hoping needing jsonqualifier elements support isn't needed for this PR?

I'm thinking that this API won't be needed because the usage of it won't be the intended behavior (honoring the annotation elements).

I don't quite follow. Why doesn't this?

moving to #435 (comment)

Technically more correct since we're defining these

ZacSweers · 2018-02-24T17:09:24Z

Yeah I didn't intend for this PR to become this big (wanted to keep it to the processor and iterate on integration tests), but with the offline requests to add both the kotlinjsonadapter tests and kotshi tests, I don't see how it could have stayed small. Have tried to keep the commits small and digestible.

If the type is a parameterized type, then we know they'll have the two-arg constructor. This way we don't always try and fail the single arg constructor on parameterized types

NightlyNexus · 2018-02-27T02:11:31Z

in toJson, there's a check for non-null types:

if (value == null) {
  throw NullPointerException("value was null! Wrap in .nullSafe() to write nullable values.")
}

should there be a symmetric check in fromJson (peek on the reader for null)?

In DataClassesTest_DefaultValuesJsonAdapter.kt, I got this:

return DataClassesTest.DefaultValues(foo = foo ?: throw JsonDataException("Required property 'foo' missing at ${reader.path}"))
                .let {
                  it.copy(bar = bar ?: it.bar,
                      nullableBar = if (nullableBarSet) nullableBar else it.nullableBar,
                      bazList = bazList ?: it.bazList)
                }

Do we need the copy?

In DataClassesTest_DefaultValuesJsonAdapter, the list__stringAdapter property could lose the duplicate underscore.

NightlyNexus · 2018-02-26T21:25:46Z

kotlin-codegen/runtime/src/main/java/com/squareup/moshi/MoshiSerializable.kt

+    if (!rawType.isAnnotationPresent(MoshiSerializable::class.java)) {
+      return null
+    }
+


throw an error if this is not a Kotlin class (doesn't have that metadata annotation)?

NightlyNexus · 2018-02-27T00:40:49Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+ * If you don't want this though, you can use the runtime [MoshiSerializable] factory implementation.
+ */
+@AutoService(Processor::class)
+class MoshiKotlinCodeGenProcessor : KotlinAbstractProcessor(), KotlinMetadataUtils {


why the inheritance instead of top-level utility functions? can we copy the utilities we need?

Mostly adopted from the example in the repo, but I'm not exactly in favor of just copying in the utilities we need (implicit fork, etc). I don't really see an issue with this if we're ok with otherwise using AbstractProcessor() as a base class

I don’t like that this implements the KotlinMetadataUtils interface. It adds that type to this type’s public API. This isn’t a dealbreaker here because this isn’t an API type, but it’s still bad to have APIs that impose themselves on API signatures.

It’s especially awkward for Kotlin since there are already good ways to expose this kind of behavior. For example, why even have the interface for extension functions? Seems like something @Takhion might be able to fix.

(Ideally we fix in follow-up to this PR.)

Yeah I agree, we can clean it up in a followup. Will talk with @Takhion

NightlyNexus · 2018-02-27T00:53:15Z

kotlin-codegen/runtime/src/main/java/com/squareup/moshi/MoshiSerializable.kt

+
+@Retention(RUNTIME)
+@Target(CLASS)
+annotation class MoshiSerializable


it'd still be neat to generate a JsonAdapter.Factory instead of using this class. or was there an explicit reason not to?

ZacSweers · 2018-02-27T03:06:02Z

Trying to unpack the larger comment above and answering inline, let me know if I've messed up the groupings.

in toJson, there's a check for non-null types:

There was, but not anymore after dea9c0b. Also - the code snippet seems to be from fromJson()? I don't think it ever threw exceptions on null values in toJson()

should there be a symmetric check in fromJson (peek on the reader for null)?

This went through a few iterations in the PR, but the most recent one we landed on after @swankjesse's past pass was to read everything into nullable local variables, then throw eagerly in the event of null/absent values for require properties in 8eb5bb4

Do we need the copy?

Yes. Worked on this with @rharter and @swankjesse a fair bit before the PR, it's basically a clever way to "read" the default values from the object since we can't really read them at compile time and inline them.

the list__stringAdapter property could lose the duplicate underscore.

I agree! Wasn't sure how to differentiate type params in the simplified names, suggestions welcome :). General trend from the other naming cleanups was to move as close as possible to camel casing and general readability.

it'd still be neat to generate a JsonAdapter.Factory instead of using this class. or was there an explicit reason not to?

(I oddly can't respond to this one inline on github 🤔)
This pattern scales pretty horribly in multi module apps. The idea behind the approach here is to keep the library agnostic with the optional runtime factory for basic drop-in use. Someone else could still write their own factory generator if they want too!

Also - assuming you meant to comment on the factory and not the annotation? We definitely need the annotation in some form as a sentinel to the annotation processor.

This is a tiny optimization to make type aliases (which did already work) reuse adapter properties if they already exist for the backing type. What this means is that if you have: typealias Foo = String and properties foo: Foo bar: String you'll only get one adapter property field for String, and both will use it

swankjesse · 2018-03-11T22:17:49Z

kotlin-codegen/compiler/pom.xml

+    <repository>
+      <id>jcenter</id>
+      <url>https://jcenter.bintray.com/</url>
+    </repository>


Added a tracking issue: Takhion/kotlin-metadata#5

swankjesse · 2018-03-11T22:21:51Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+/**
+ * An annotation processor that reads Kotlin data classes and generates Moshi JsonAdapters for them.
+ * This generates Kotlin code, and understands basic Kotlin language features like default values
+ * and companion objects.


swankjesse · 2018-03-11T22:26:39Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+ * If you don't want this though, you can use the runtime [MoshiSerializable] factory implementation.
+ */
+@AutoService(Processor::class)
+class MoshiKotlinCodeGenProcessor : KotlinAbstractProcessor(), KotlinMetadataUtils {


I don’t like that this implements the KotlinMetadataUtils interface. It adds that type to this type’s public API. This isn’t a dealbreaker here because this isn’t an API type, but it’s still bad to have APIs that impose themselves on API signatures.

It’s especially awkward for Kotlin since there are already good ways to expose this kind of behavior. For example, why even have the interface for extension functions? Seems like something @Takhion might be able to fix.

swankjesse · 2018-03-11T22:26:54Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+ * If you don't want this though, you can use the runtime [MoshiSerializable] factory implementation.
+ */
+@AutoService(Processor::class)
+class MoshiKotlinCodeGenProcessor : KotlinAbstractProcessor(), KotlinMetadataUtils {


(Ideally we fix in follow-up to this PR.)

swankjesse · 2018-03-11T22:35:45Z

kotlin-codegen/compiler/src/main/java/com/squareup/moshi/MoshiKotlinCodeGenProcessor.kt

+  override fun getSupportedSourceVersion(): SourceVersion = SourceVersion.latest()
+
+  override fun process(annotations: Set<TypeElement>, roundEnv: RoundEnvironment): Boolean {
+    val annotationElement = elementUtils.getTypeElement(annotationName)


I think it’s awkward how elementUtils is wired in. If we wanted a minimal way to go from MoshiSerializable::class to the corresponding element, it would be neater to have a function that does just that:

fun ProcessingEnvironment.typeElement(kclass: KClass) = ...

Can do in follow-up

swankjesse · 2018-03-12T01:01:54Z

kotlin-codegen/integration-test/src/test/kotlin/com/squareup/moshi/KotlinCodeGenTest.kt

+  inner class InnerClass(val a: Int)
+
+  @Ignore @Test fun localClassesNotSupported() {
+    class LocalClass(val a: Int)


@MoshiSerializable ?

swankjesse · 2018-03-12T01:02:06Z

kotlin-codegen/integration-test/src/test/kotlin/com/squareup/moshi/KotlinCodeGenTest.kt

+    }
+  }
+
+  @Ignore @Test fun objectDeclarationsNotSupported() {


swankjesse · 2018-03-12T01:03:12Z

...in-codegen/integration-test/src/test/kotlin/com/squareup/moshi/kotshi/ClassWithWeirdNames.kt

+import com.squareup.moshi.MoshiSerializable
+
+@MoshiSerializable
+data class ClassWithWeirdNames(


swankjesse · 2018-03-12T01:03:39Z

kotlin-codegen/integration-test/src/test/kotlin/com/squareup/moshi/kotshi/DefaultValues.kt

+data class WithCompanionProperty(val v: String?)
+
+@MoshiSerializable
+data class WithStaticProperty(val v: String?)


what’s the static property?

Might have been a vestige of the kotshi default values API

swankjesse · 2018-03-12T01:06:17Z

kotlin-codegen/integration-test/src/test/kotlin/com/squareup/moshi/kotshi/GenericClass.kt

@@ -0,0 +1,7 @@
+package com.squareup.moshi.kotshi


I’m reluctant to merge this unless it’s a contribution from the original author. CLA stuff.

ZacSweers · 2018-03-18T08:48:20Z

Think I covered the questions you had. Once #462 is merged, I can make a followup with some cleanups from above and also fro #461

rharter reviewed Feb 9, 2018

View reviewed changes

FabianTerhorst reviewed Feb 11, 2018

View reviewed changes

ZacSweers and others added 18 commits February 15, 2018 15:07

Add kotlin code gen modules

fc927f1

Update kotlin to 1.2

fabdf13

Add a serializable dummy class

c16bd74

Try using kapt configuration from kotlin-examples repo

5629bb9

Still no luck!

Use proper allocated name for assignment too

8b84f7a

Use selectName() API

6c32fb7

Clean up constructor parameter annotations & plumbing for qualifiers

221b9b3

Updates poms and kotlin code gen processor to support tests.

d9303d3

Ignore kotlin code gen tests for now

f39f3fc

None of these are data classes tests right now, which is the only thing this supports right now

Replace $ with _ in class names for consistency

f45e0ff

Shortcut Array types to arrayOf

4c2b044

Add DataClassTest

a2dd477

Try generated option first, fall back to maven after

380a3a2

More idiomatic handling

316eb77

Only use nonnullable types for adapter properties

9c98c37

Code dump of kotshi tests

35ef68c

Comment out specifics to get compiling

36689ab

Generics support!

592a08e

ZacSweers added 8 commits February 19, 2018 21:03

Use KClass where appropriate

0390cc5

End comment in period

999e3fc

Remove redundant comment

0702fc7

Throw on unrecognized type in simplified name

e090521

Use illegalargumentexception instead

1fec18a

Emit a nullcheck at the beginning of toJson instead

5ddf528

Remove extra newline

b59f36a

Simplify processing to be less abusive

a33d46c

Egorand reviewed Feb 19, 2018

View reviewed changes

ZacSweers added 2 commits February 19, 2018 22:49

Skip using asClassName() when possible

9a34bb0

Use addComment()

7525f89

ansman reviewed Feb 21, 2018

View reviewed changes

NightlyNexus reviewed Feb 23, 2018

View reviewed changes

ZacSweers added 3 commits February 24, 2018 17:59

Switch to declared constructors

5597b91

Technically more correct since we're defining these

Unmodifiable set

d877e88

return adapter(type, annotationTypes[0])

5ed2a58

Slight optimization - check if the type is parameterized first

67f4ab1

If the type is a parameterized type, then we know they'll have the two-arg constructor. This way we don't always try and fail the single arg constructor on parameterized types

NightlyNexus reviewed Feb 27, 2018

View reviewed changes

ZacSweers added 2 commits March 10, 2018 00:28

Use string templating where possible

a9ae13a

swankjesse approved these changes Mar 12, 2018

View reviewed changes

Remove all the kotshi tests

31b11f0

swankjesse merged commit 96e074d into square:master Mar 12, 2018

ZacSweers deleted the z/kotlincodegen branch March 18, 2018 08:46

yshrsmz mentioned this pull request Apr 24, 2018

2018年に使うJSONライブラリ AndroidDagashi/AndroidDagashi#168

Open

Kotlin Code Gen module #435

Kotlin Code Gen module #435

Conversation

ZacSweers commented Feb 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Feb 12, 2018

ZacSweers commented Feb 12, 2018

Takhion commented Feb 12, 2018

rharter commented Feb 12, 2018

ZacSweers commented Feb 12, 2018 • edited

Takhion commented Feb 12, 2018

ZacSweers commented Feb 12, 2018

JakeWharton commented Feb 12, 2018

ZacSweers commented Feb 12, 2018 • edited

ZacSweers commented Feb 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers Feb 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightlyNexus left a comment

Choose a reason for hiding this comment

NightlyNexus Feb 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Feb 24, 2018 • edited

NightlyNexus commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Mar 18, 2018

ZacSweers commented Feb 9, 2018 •

edited

ZacSweers commented Feb 12, 2018 •

edited

ZacSweers commented Feb 12, 2018 •

edited

ZacSweers commented Feb 13, 2018 •

edited

ZacSweers Feb 21, 2018 •

edited

NightlyNexus Feb 23, 2018 •

edited

ZacSweers commented Feb 24, 2018 •

edited