square / kotlinpoet

A Kotlin API for generating .kt source files.
https://square.github.io/kotlinpoet/
Apache License 2.0
3.93k stars 289 forks source link

add doc about `reified` keyword #939

Open z-950 opened 4 years ago

z-950 commented 4 years ago

if want to create a function like this:

inline fun <reified T> foo()

the correct but not obvious code of kotlinpoet should be (1.6.0):

FunSpec.builder("foo")
  .addModifiers(KModifier.INLINE)
  .addTypeVariable(TypeVariableName("T").copy(reified = true))
  .build()

the wrong but obvious way is :

addTypeVariable(TypeVariableName("T", KModifier.REIFIED))

and will throw:

Execution failed for task ':example:kaptKotlin'.
> A failure occurred while executing org.jetbrains.kotlin.gradle.internal.KaptExecution
   > java.lang.reflect.InvocationTargetException (no error message)

it's hard to know KModifier.REIFIED use case. or it will never be used?

ZacSweers commented 4 years ago

What's the full stacktrace? That snippet isn't enough context

Egorand commented 4 years ago

The signature of the function you're trying to use is (see TypeVariableName docs):

operator fun invoke(name: String, variance: KModifier? = null): TypeVariableName

So although it accepts a KModifier as the second argument, the subset of values that are allowed is limited to KModifier.IN and KModifier.OUT. Agree that it's unfortunate that there's no compile-time check to prevent you from misusing this API, but hopefully the parameter name should suggest that KModifier.REIFIED is not a supported argument.

z-950 commented 4 years ago

The signature of the function you're trying to use is (see TypeVariableName docs):

operator fun invoke(name: String, variance: KModifier? = null): TypeVariableName

So although it accepts a KModifier as the second argument, the subset of values that are allowed is limited to KModifier.IN and KModifier.OUT. Agree that it's unfortunate that there's no compile-time check to prevent you from misusing this API, but hopefully the parameter name should suggest that KModifier.REIFIED is not a supported argument.

reified's use place is the same as in and out. So it's strange that KModifier.REIFIED could not be used here.

z-950 commented 4 years ago

What's the full stacktrace? That snippet isn't enough context

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':example:kaptKotlin'.
> A failure occurred while executing org.jetbrains.kotlin.gradle.internal.KaptExecution
   > java.lang.reflect.InvocationTargetException (no error message)

* Try:
Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Exception is:
org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':example:kaptKotlin'.
Caused by: java.lang.reflect.InvocationTargetException
Caused by: java.lang.reflect.InvocationTargetException
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at org.jetbrains.kotlin.kapt3.base.AnnotationProcessingKt.doAnnotationProcessing(annotationProcessing.kt:76)
    at org.jetbrains.kotlin.kapt3.base.AnnotationProcessingKt.doAnnotationProcessing$default(annotationProcessing.kt:35)
    at org.jetbrains.kotlin.kapt3.base.Kapt.kapt(Kapt.kt:45)
    ... 28 more
Caused by: com.sun.tools.javac.processing.AnnotationProcessingError: java.lang.IllegalArgumentException: REIFIED is an invalid variance modifier, the only allowed values are in and out!
    at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:1035)
    at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:939)
    at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1267)
    at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1381)
    at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1263)
    at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1162)
    ... 34 more
Caused by: java.lang.IllegalArgumentException: REIFIED is an invalid variance modifier, the only allowed values are in and out!
    at com.squareup.kotlinpoet.TypeVariableName$Companion.of$kotlinpoet(TypeName.kt:772)
    at com.squareup.kotlinpoet.TypeVariableName$Companion.get(TypeName.kt:785)
    at microservice.codegen.proxy.EbProxyBuildKt.ebProxyBuild(EbProxyBuild.kt:94)
    at microservice.codegen.AnnotationProcessor.process(AnnotationProcessor.kt:60)
    at org.jetbrains.kotlin.kapt3.base.incremental.IncrementalProcessor.process(incrementalProcessors.kt)
    at org.jetbrains.kotlin.kapt3.base.ProcessorWrapper.process(annotationProcessing.kt:147)
    at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:1023)
    ... 39 more

The line:

at microservice.codegen.proxy.EbProxyBuildKt.ebProxyBuild(EbProxyBuild.kt:94)

points to: addTypeVariable(TypeVariableName("T", KModifier.REIFIED))

Egorand commented 4 years ago

Any thoughts on how we can improve this? Having a parameter like modifier: KModifier feels problematic to me since we only want to allow IN, OUT and REIFIED, but I'm struggling to come up with a good name for that parameter to represent this subset only. This is why I believe the decision was to model TypeVariableName the way it is modeled currently.

I'm also not sure what exactly you'd like to see in the docs to make the correct solution more obvious. Please feel free to send us PRs.

JakeWharton commented 4 years ago

How about TypeVariableModifier

z-950 commented 4 years ago

To make the correct solution more obvious:

  1. Add a setter to mark TypeVariableName has reified modifier.
  2. Or add the correct example to doc, in README or in other places.

In my view, KModifier.REIFIED is useless in 1.6.0 version, but i'am not sure. If true, it's better to add comments on KModifier.REIFIED about the correct way to set reified modifier.

And the best way is, adding support about KModifier.REIFIED.

@Egorand The name of the subset can come from kotlin-reference. In it, in, out and reified is told to mark type parameter. So i think TypeParameterModifier is a suitable name. What's more, there is another modifier named where mentioned in the reference, but i know little about it.

Egorand commented 4 years ago

I guess we can introduce a new factory method that takes in a TypeParameterModifier (or TypeVariableModifier as Jake suggested), which would be an enum that only includes in, out and reified. Would that make sense?

z-950 commented 4 years ago

I think it's helpful to clear the usage.