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

Provide APIs to resolve positional types for parameters #717

Closed
yigit opened this issue Nov 11, 2021 · 2 comments
Closed

Provide APIs to resolve positional types for parameters #717

yigit opened this issue Nov 11, 2021 · 2 comments
Assignees
Labels
api API changes may be involved P1 major features or blocking bugs
Milestone

Comments

@yigit
Copy link
Collaborator

yigit commented Nov 11, 2021

Kotlin has some rules around how it converts variance information for Java interop.
https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html#variant-generics

Specifically, parameters of methods might include variance based on the declaration or the overridee. There are also JvmSuppressWildcards / JvmWildcards annotation to force particular behavior.

It would be great if KSP provided ability to get the type of a parameter based on its location (also accounting for annotations, overidden method signatures etc).

Looks like this is how KAPT does it:

https://github.com/JetBrains/kotlin/blob/92d200e093c693b3c06e53a39e0b0973b84c7ec5/plugins/kapt3/kapt3-compiler/src/org/jetbrains/kotlin/kapt3/stubs/ErrorTypeCorrector.kt#L145

Note that, i don't think these types should automatically resolve with the variance information as it would be unnecessary when generating Kotlin code, hence suggesting a new API JVM specific API to do it.

@yigit
Copy link
Collaborator Author

yigit commented Nov 11, 2021

related bug in X-Processing: https://issuetracker.google.com/issues/204415667

@ting-yuan ting-yuan self-assigned this Nov 11, 2021
@ting-yuan ting-yuan added this to the 1.0.2 milestone Nov 12, 2021
@ting-yuan ting-yuan added api API changes may be involved P1 major features or blocking bugs labels Nov 12, 2021
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Nov 17, 2021
This CL fixes a bug in XProcessing where the TypeName for parameters did
not include variance information.
https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html#variant-generics

For Room, we only needed this information while generating overrides
hence it was handled in MethodSpecHelper / OverrideVarianceResolver.
This created an inconsistent state when the TypeName is directly
obtained from the parameter.

We had 2 possible options:
a) Keep the location where we obtained the type along with the XType such
that we can change the TypeName generation
b) Update the XType returned from these parameters to include variance.

In most similar cases, we took route B to ease the migration to
XProcessing (e.g. suspend functions generate a synthetic parameter).
Given that the migration is complete and the future of Room is Kotlin, I
would rather keep the correct KSType information as Kotlin sees it (even
though it is inconsistent w/ KAPT).

I've tried multiple prototypes on how this information can be embedded
into KspType but unfortunately none are clean. The problem is that, we
have many places in the code where we obtain a KspType and we don't
always have the original location information. It would be possible to
pass it around but it creates a lot of noise.

Instead of doing that, I decided to add a new JvmTypeResolver property
to KspType which gives ability to hook into the TypeName generation. As
such, now each KspTypeElement class implements a resolveTypeName method
instead of overriding the property (which became final in KspType).

Unfortunately, I couldn't find a decent documentation on how this works
in all cases. So most of the logic in KspJvmTypeResolver /
KSTypeVarianceResolver is reverse engineered from the
KspTypeNamesGoldenTest. There is still cases where we miss for return
types but it also does not impact code generation.
Eventaully, we should be able to replace KSJvmTypeResolver with a KSP
API once it is available. google/ksp#717

Bug: 204415667
Test: KSTypeExtTest, KspTypeNamesGoldenTest
Change-Id: I4294dcd89e3c4d4f58b3b71f7923d3223876f9c1
@ting-yuan
Copy link
Collaborator

Implemented in #732 as a member function of Resolver:
fun getJavaWildcard(reference: KSTypeReference): KSTypeReference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API changes may be involved P1 major features or blocking bugs
Projects
None yet
Development

No branches or pull requests

2 participants