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 an API to set TypeVariableName bounds lazily #842

Open
yigit opened this issue Jun 4, 2021 · 2 comments
Open

Provide an API to set TypeVariableName bounds lazily #842

yigit opened this issue Jun 4, 2021 · 2 comments

Comments

@yigit
Copy link

yigit commented Jun 4, 2021

Currently, there is no way to set bounds of a TypeVariableName after it is constructed (which makes sense as they are designed to be immutable).

Unfortunately, creating TypeName for self referencing types becomes impossible without it (class SelfReferencing<T : SelfReferencing<T>>)

JavaPoet solves this problem internally by using a private constructor:
https://github.com/square/javapoet/blame/master/src/main/java/com/squareup/javapoet/TypeVariableName.java#L108

Would it be possible to open up a builder that receives a List (and keeps it as is) so we can implement the same workaround for KSP ?

For reference, here is the WIP CL in Room where we are trying to workaround this issue. Unfortunately, the only solution we have is reflection which does not feel great.

@tbroyer
Copy link
Collaborator

tbroyer commented Jun 4, 2021

You can use a TypeVariableName.get(name) to reference the "outer" type variable, it doesn't have to represent the recursion.

$ jshell --class-path ~/.m2/repository/com/squareup/javapoet/1.13.0/javapoet-1.13.0.jar
|  Welcome to JShell -- Version 11.0.11
|  For an introduction type: /help intro

jshell> import com.squareup.javapoet.*;

jshell> JavaFile.builder("pkg",
   ...>   TypeSpec.classBuilder("Cls")
   ...>    .addTypeVariable(TypeVariableName.get("T", ParameterizedTypeName.get(ClassName.get("pkg", "Cls"), TypeVariableName.get("T"))))
   ...>    .build()).build()
$2 ==> package pkg;

class Cls<T extends Cls<T>> {
}


jshell> 

@yigit
Copy link
Author

yigit commented Jun 7, 2021

Thanks for the tip.

Unfortunately, this happens at a lower layer. We have some wrapper around KSP - KSType (kotlin symbol processing) and JavaAP-TypeMirror both of which provide an API to get a typeName. I'm trying to make sure they return the exactly same TypeName for the same type.

copybara-service bot pushed a commit to androidx/androidx that referenced this issue Jun 9, 2021
This CL fixes (a.k.a hacks) an issue in TypeName generation logic in the
KSP pipeline where we would go into an infinite loop if we are trying to
create TypeName for a self referencing type, e.g.:
class SelfReferencing<T : SelfReferencing<T>>

JavaPoet solves this using an internal constructor in TypeVariableName
and changing bounds after it is created. For this reason, we are also
finding that constructor now and using it to construct a
TypeVariableName reflectively.
square/javapoet#842

Unfortunately, this fix hits a bug in KSP.
google/ksp#476
I kept the test where it expects the Error class so we know when it is
fixed.

Also, there is still saddle differences between KAPT and KSP. More
specifically, KAPT might generate wildcard type parameters due to
variance. We do that at code generation phase and doing it in typename
creation is fairly complicated (a.k.a. i couldn't find an easy way).
This does not really matter much so I'm keeping them separate for now.

I've also changed compilation test capabilities to only consider kotlin
major-minor versions as KSP is more stable now and we should try to
avoid disabling it.

Bug: 187572913
Test: XTypeTest
Change-Id: I9d85f999cd9ec976149dad961b6e94154dde8c16
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

No branches or pull requests

2 participants