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

add doc about reified keyword #939

Open
z-950 opened this issue Jul 9, 2020 · 9 comments
Open

add doc about reified keyword #939

z-950 opened this issue Jul 9, 2020 · 9 comments
Milestone

Comments

@z-950
Copy link

z-950 commented Jul 9, 2020

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
Copy link
Collaborator

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

@Egorand
Copy link
Collaborator

Egorand commented Jul 9, 2020

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
Copy link
Author

z-950 commented Jul 10, 2020

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
Copy link
Author

z-950 commented Jul 10, 2020

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
Copy link
Collaborator

Egorand commented Jul 10, 2020

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
Copy link
Member

How about TypeVariableModifier

@z-950
Copy link
Author

z-950 commented Jul 10, 2020

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
Copy link
Collaborator

Egorand commented Jul 10, 2020

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
Copy link
Author

z-950 commented Jul 10, 2020

I think it's helpful to clear the usage.

This was referenced Jul 30, 2020
@Egorand Egorand added this to the Backlog milestone Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants