Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support direct annotation instantiation in code gen on Kotlin 1.6 #1390

Merged
merged 26 commits into from Oct 22, 2021

Conversation

ZacSweers
Copy link
Collaborator

Resolves #1382. This also implements it gated on the target type's language version, which allows this to seamlessly work in a backward-compatible way.

Generated adapter now looks like this

public class GeneratedAdaptersTest_PropertyWithQualifierJsonAdapter(
  moshi: Moshi
) : JsonAdapter<GeneratedAdaptersTest.PropertyWithQualifier>() {
  private val options: JsonReader.Options = JsonReader.Options.of("a", "b")

  private val stringAtUppercaseAdapter: JsonAdapter<String> = moshi.adapter(String::class.java,
      setOf(GeneratedAdaptersTest.Uppercase(inFrench = true)), "a")

  private val stringAdapter: JsonAdapter<String> = moshi.adapter(String::class.java, emptySet(),
      "b")
      
  // ...
}

@ZacSweers
Copy link
Collaborator Author

The failure is interesting. Basically, the AdapterMethodsFactory quality check here fails, but I'm not sure if this is a bug on our end or Kotlin's. Trying to reproduce it in a simple case

if (Types.equals(adapterMethod.type, type) && adapterMethod.annotations.equals(annotations)) {

@ZacSweers
Copy link
Collaborator Author

It's a bug on their side with hashCode(), filed here https://youtrack.jetbrains.com/issue/KT-48606

@ZacSweers ZacSweers marked this pull request as draft September 13, 2021 02:32
@ZacSweers ZacSweers force-pushed the z/prototypeAnnotationInstantiation branch from 886506a to a7eb500 Compare September 13, 2021 02:33
@ZacSweers
Copy link
Collaborator Author

Looks like it's not in the 1.6-M1 branch yet. Will try again when it is

image

@ZacSweers ZacSweers force-pushed the z/prototypeAnnotationInstantiation branch from a7eb500 to e1fca34 Compare September 13, 2021 02:44
@ZacSweers ZacSweers force-pushed the z/prototypeAnnotationInstantiation branch from e1fca34 to c7335ec Compare September 24, 2021 18:46
@ZacSweers
Copy link
Collaborator Author

Still broken in 1.6.0-M1, pushed to 1.6.0-RC now -_-

@ZacSweers
Copy link
Collaborator Author

Small update after talking with @swankjesse

  • We'll add an option to optionally disable this behavior, in case there's lingering bugs with the Kotlin implementation
  • It will be enabled by default for Kotlin 1.6+

Will wait for #1393 to merge in and Kotlin 1.6-RC (which should have this implemented)

@ZacSweers
Copy link
Collaborator Author

Screen Shot 2020-10-06 at 4 50 35 PM

@ZacSweers ZacSweers force-pushed the z/prototypeAnnotationInstantiation branch from b5a6ae2 to 54c0f73 Compare October 16, 2021 06:26
@ZacSweers
Copy link
Collaborator Author

Got an interesting problem here.

In short, we reuse AnnotationSpec instances for qualifiers. Since we’re directly instantiating them now, we can just reuse them and join their members as the constructor args. However, internally in kotlinpoet it uses collection literal [...] syntax for arrays, but that isn’t allowed in regular construction.

So there’s three options I can think of:
A. We change AnnotationSpec.get() to not use collection literal syntax. Use arrayOf() to maximize portability/reusability
B. We add a parameter/configuration option somewhere to control this. Would probably be hard to write in and we’d need to introduce something like a CodeWriter with configurable behavior like Moshi’s JsonWriter
C. We try to manually patch the underlying CodeBlock for array types from an AnnotationSpec. This seems really error prone

I think we should go with option A. Doesn’t read as nicely but it’s definitely in the spirit of portability like other changes have been (explicit public modifiers, imports from kotlin.*, etc). lmk what you think

@ZacSweers ZacSweers marked this pull request as ready for review October 16, 2021 16:52
@efemoney
Copy link

efemoney commented Oct 18, 2021

lmk what you think

I agree, AnnotationSpec should be updated to arrayOf

ZacSweers added a commit to square/kotlinpoet that referenced this pull request Oct 19, 2021
This allows these annotation specs' members to be more portable regardless of whether it's used as an annotation or constructor called. See square/moshi#1390 (comment) for more details
ZacSweers added a commit to square/kotlinpoet that referenced this pull request Oct 19, 2021
This allows these annotation specs' members to be more portable regardless of whether it's used as an annotation or constructor called. See square/moshi#1390 (comment) for more details
@@ -125,8 +130,8 @@ subprojects {
pluginManager.withPlugin("org.jetbrains.kotlin.jvm") {
tasks.withType<KotlinCompile>().configureEach {
kotlinOptions {
@Suppress("SuspiciousCollectionReassignment")
freeCompilerArgs += listOf("-progressive")
// @Suppress("SuspiciousCollectionReassignment")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete if not needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, will remove before merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually going to leave it but with a comment explaining it's just temporary while we support dual kotlin versions

@ZacSweers
Copy link
Collaborator Author

After talking with @Egorand, plan is option A. It's already merged in KotlinPoet and I'll update this after that release is out

@ZacSweers
Copy link
Collaborator Author

woo!

@ZacSweers ZacSweers merged commit b8fbe38 into master Oct 22, 2021
@ZacSweers ZacSweers deleted the z/prototypeAnnotationInstantiation branch October 22, 2021 17:43
Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Directly instantiate qualifier annotations in Kotlin 1.6+
4 participants