-
Notifications
You must be signed in to change notification settings - Fork 289
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
TypeMirror.asTypeName() returns java.lang.String when receiver type is kotlin.String #236
Comments
It's not really a bug. Kotlin uses a Java string when run on the JVM so that's what you see when using introspection APIs. Your workaround will incorrectly change non-Kotlin string types into Kotlin ones and it doesn't cover the hundreds of other JVM types that Kotlin ones map directly to. |
Thx! I understood the rationale and let me elaborate the situation I encountered. @MarkerAnnotation
fun hoge(arg: String) {
// do something
} Then we expect such code is generated with KotlinPoet: fun hogeWithKapt(arg: String) {
// arg type is expected to be kotlin.String
} But now we've got the code like: fun hogeWithKapt(arg: java.lang.String) {
// IDE flags warning about argument to use kotlin.String
} Sorry I forgot to mention but our workaround is just for our library, so we don't have to consider any side effect about it(I didn't intend to add it to KotlinPoet for sure). Anyway I realized it's kind of counter-intuitive because it means KotlinPoet user always take care about replacing java String to kotlin String. I don't come up with good way to deal with but I just reported as feedback 🙇 Please close this issue if it's not fit for your plan. |
We might be able to pull the Kotlin type information from the metadata annotations automatically. |
Note that the two types aren’t interchangeable. This compiles:
This doesn’t.
|
The same thing happens with @JakeWharton getting the type information from metadata is indeed possible. See which correctly returns |
I also have this problem, here is my solution.
you need to import kotlin-reflect |
@richardwrq it doesn't seem to be working for me with generic classes like |
@JakeWharton Any news on this one? |
I'm interested in this as well :) |
@richardwrq @tschuchortdev This code is ok also for generic classes like List: fun Element.javaToKotlinType(): TypeName =
asType().asTypeName().javaToKotlinType()
fun TypeName.javaToKotlinType(): TypeName {
return if (this is ParameterizedTypeName) {
ParameterizedTypeName.get(
rawType.javaToKotlinType() as ClassName,
*typeArguments.map { it.javaToKotlinType() }.toTypedArray()
)
} else {
val className =
JavaToKotlinClassMap.INSTANCE.mapJavaToKotlin(FqName(toString()))
?.asSingleFqName()?.asString()
return if (className == null) {
this
} else {
ClassName.bestGuess(className)
}
}
}
`` |
@zigzago Works like a charm! Thank you |
@zigzago I debugged a little bit and seems that the second Additionally the solution uses
which I don't want to rely on... |
@StefMa this is strange, because Anyway my solution is a workaround. As mentioned above, kotlin metadata should be used to really fix the issue. |
@zigzago
Anyway, thanks for this solution! @StefMa |
after 1.0.0-RC1 the above answer will need some changes in order to work, these lines to be specific.
====>
this was suggested on #424 . |
In version |
Api has changed a bit (use parameterizedBy instead of ParameterizedTypeName.get): private fun TypeName.javaToKotlinType(): TypeName = if (this is ParameterizedTypeName) {
(rawType.javaToKotlinType() as ClassName).parameterizedBy(
*typeArguments.map { it.javaToKotlinType() }.toTypedArray()
)
} else {
val className = JavaToKotlinClassMap.INSTANCE
.mapJavaToKotlin(FqName(toString()))?.asSingleFqName()?.asString()
if (className == null) this
else ClassName.bestGuess(className)
} |
For me it's just |
I'd like to note that in light of this "bug", God bless the opensource nature of this library and kotlin's functions extensions: I was able to copy paste |
Is the JavaToKotlinClassMap class still accessible? i can't seem to find it. |
A map is not the correct nor accurate solution to this problem since it's not a 1:1 mapping. Multiple distinct Kotlin types are represented as the same JVM type and it's only exacerbated by inline classes. Parsing the metadata is the only way forward. |
I see, any plans to implement this in the near future or is it staying in the IceBox? |
Hasn't the metadata stuff already been merged? |
I'm using v1.4.1 and i still have this problem happening, so unless if i'm missing something, i don't think so. |
The metadata artifacts are separate and were released in 1.4.0. There are full READMEs in their artifacts on the repo. It’s important to understand that metadata annotations are only present on classes. Not individual functions, types, properties, parameters, or anywhere else. In short: cases like You can, however, derive the metadata context from navigable elements like methods, fields, etc. You simply traverse up its hierarchy to the first enclosing class, which will have the needed metadata annotation on it. Then you can connect that parsed information back to your source element and connect the dots to deduce the correct type. This is exactly how the snippet I provided above for There is possibly an argument to be made that either kotlinpoet-metadata artifacts should live directly in the main KotlinPoet artifact, or the existing problematic APIs should be deprecated and moved into the kotlinpoet-metadata artifact where they can ask for the needed classpath information. At this point I think that’s a separate issue. |
@StefMa @zigzago Interestingly, if we have a As a result, we can improve
And now it should work while I didn't test it thoroughly. |
`
` |
Is there a "correct" way to handle this yet? |
Yes, use the metadata APIs. |
@ZacSweers are there any examples on how to use that? |
Did you look at the readmes on both metadata artifacts? |
Is there any resource/example one could follow to use the metadata APIs? I can't rely on the documentation of the metadata artefact as it's not clear at all. |
@andromedcodes there are READMEs in their modules. If those aren't sufficient, PR suggested improvements. |
@ZacSweers Thanks. I have a question if I may. I have this annotation
as you can see args is an annotation but is used as an array of values in |
Please ask usage questions on stackoverflow. Given the general availability of metadata support, I'm going to close this issue out. |
For any future reader here is a very interesting talk about metadata @JohnOberhauser |
Hey @ZacSweers I'm trying to use the overriding implementation provided above, but the "jvmMethodSignature" method in: val jvmSignature = element.jvmMethodSignature(types) can't be found anywhere. Has this been deleted? If or otherwise, is it possible to get an updated version of the code above? (also the test in the snippet uses the old signature, it needs newly added third parameter.) |
Are there any documentation for kotlinpet metadata anywhere? |
Did you look at their docs on the project site? https://square.github.io/kotlinpoet/kotlinpoet_metadata/ https://square.github.io/kotlinpoet/kotlinpoet_metadata_specs/ |
FYI both doc links are broken |
您的来信已收到。谢谢
|
@CasualGitEnjoyer the artifacts have been renamed, which invalidated the old URLs. Here's an up-to-date link for interop-kotlinx-metadata docs: https://square.github.io/kotlinpoet/interop-kotlinx-metadata/. |
Overview
Thx for amazing library!
I encountered a problem that
TypeMirror.asTypeName()
returnsjava.lang.String
when receiver type iskotlin.String
. It seems this is a bug so currently we inserted a workaround below to avoid that.The text was updated successfully, but these errors were encountered: