https://kotlinlang.org logo
Join Slack
Powered by
XProcessing top level constructs thread. cc: <@U4F...
# ksp
y
XProcessing top level constructs thread. cc: @gabrielfv @elihart
@gabrielfv, @elihart needs this feature as well (to model top level functions/properties etc).
so i'm creating this thread to sync not to have two parallel implementations
e
yes, happy to help if I can, and also curious when you might have this work done @gabrielfv I haven’t looked to carefully at it yet, but one issue seems to be that in kapt a top level construct will have an enclosing class and in ksp it will not. I’m curious what you all think about how to approach that, and what the philosophy is on how much the abstraction needs to maintain similar behavior between environments vs diverging in some (well documented) cases
g
@yigit suggested that we could put divergences like that behind a 
targetLanguage
configuration. The way KSP works with its new api is by returning us elements like 
KSClassDeclaration
, but they all have enclosing elements, the top-level one being 
KSFile
y
yea there are mutliple cases for this targetLanguage thing, e.g. how suspend methods are handled. Btw, for this case, one tricky part is the 
enclosingTypeElement
in 
XExecutableElement
. we need to make it nullable (due to target lang Kotlin) but also need to impelemnt a synthetic class when it is java. this is already something i would need to do regardless of this change (though room does not have that case, which is why it is not implemented 🤷 )
g
Could we make 
enclosingTypeElement
more generic to 
enclosingElement
or would that break many things? Perhaps a 
KSFile
abstraction could be useful for 
targetLanguage=kotlin
, however this kind of falls under the same problem of how we would deal with it on kapt
y
we can consider making it 
enclosingElement
but that does not change the fact that we need to synthesize a class from KSFile when processed for java. Otherwise, an annotation processor with javac in mind will have mine-fields when moving from kapt to ksp. Also, maybe I need to make that change first because this will require some refactoring ont he room codebase.
g
In this case I could follow along making it nullable and you could refactor it when it seems fit
Well actually a refactor on room would probably be needed either way
y
yea so thats why i was thinking of doing it first (make it nullable even though it is never really null )
g
Well yeah, I will keep on wiring the target language configuration and merge the changes when they´re ready
y
oh nooo.... i love replace with for these refactors but apperantly cannot use it here.
i'm curious, for methods in KSFile, would we put enclosingElement as something like 
XFile
(similar to 
KSFile
) or detect the package from the file and sysntesize an 
XPackageElement
for them. I'm asking because making 
enclosingElement
nullable for 
XExecutableElement
feels a bit weird. can it really be null ?
e.g. if you need to generate code, you really need to know the package of the file to import it in the first place
e
good point, I think it’s quite important to be able to get the package name of the executable element. Could enclosingElement return a sealed class, with values either being a file or a type?
y
not until we get sealed interfaces i believe. but any XElement can have package so it would be fine i believe. btw first step: https://android-review.googlesource.com/c/platform/frameworks/support/+/1679736
👍 1
https://android-review.googlesource.com/c/platform/frameworks/support/+/1679737 and for fields. looks like we were not calling them much anyways
g
So moving forward we´d need to define how we´re going to model the divergence between 
KSFile
and 
PackageElement
. The interesting part I see here is navigating through an element´s siblings, but this is still different for either entities. Personally I don´t think we should try to emulate packages as elements, but what you guys think?
Added the configuration so that we could start looking into what to do about the top level solution. https://github.com/androidx/androidx/pull/165
y
We never needed them so i don't have a strong opinion. If we don't implement package, what would be the containing element type for top level functions when target is Kotlin?
(for Java target, it needs to be a synthetic type element from ksfile, the same way you would see it though kapt)
g
Probably what you mentioned, we would expect it to be a synthetic KSFile, but one thing I´ve not tested with Kapt is if it´s able to address top level methods/variables, at least through that API. If it´s not able to, I think we could assume we would never need to synthesize a 
XFile
.
I´m taking a look at that rn
y
with KAPT, you'll see them as methods/fields inside the generated class from file
g
I think that could work, or we could still synthesize it to have some symmetry. What do you think?
y
from KSP, we need to synthesize it
when target lang is java (so that is more like a missing feature of XProcessing right now)
g
But would KSP run with target java?
y
yes
that is the case we have in room
technically, what it does right now in xprocessing
g
That´s curious, I would assume KSP isn´t capable of understanding java code
y
KSP understands the java code
in fact, last time i checked, we could run our java integration app w/ ksp. and almost all of our tests are already feeding it java code (because those tests existed before ksp)
g
I see
I will need to modify the configuration then because currently KSP is set to Kotlin only
y
oh that is my bad, I should've caught it in the review 🤦‍♂️
to avoid any future confusing. right now, both KSP / JavaAP paths act like lang = 
java
.
that includes taking into account annotations like @JvmName
ugh this is actually tricky... XPrrocessing will need to look at ALL KSFiles, see any of them has top level declaration 😮
to at least support 
requireTypeElement
and even handle merging because you can have multiple files that merge into 1 class in jvm
ugh, this is going to be really hard 😕 (creating a synthetic class from a set of KSFiles)
😞 1
so this might be feasible to make work. https://android-review.googlesource.com/c/platform/frameworks/support/+/1682305 I'm creating a synthetic KSClassDeclaration, KSType etc from a KSFile. Unfortunately, it is limited to KSFile's in source (so the ones in dependencies are lost) and also handling multiple KSFIles that have the same JVMName won't be trivial but should be doable.
it 
might
be OK to only support this for files in source though because for anything else, you really have no way of accessing them as far as I can see but I'll try to play with is. Especially, if a lib dependency has java + kotlin code where Java class has a reference to the generated class file from kotlin file, there should be a way to get that type & declaration
so yea it is there via KSClassDeclarationDescriptorImpl of kotlin. But it is a bit interesting... If you every access that synthetic java class via some other means, then KSP can return it in 
getClassDeclarationByName
. but if you never accessed it, it cannot. i'll file a bug to track, not sure how the right solution is. technically that class does not exist in KSP's realm but in practice it does exist and might be returned in different places.
oh actually it was working due to the cache in XProcessing, not KSP
g
Maintaining consistency across APIs will be really tricky, I was considering this. Requiring certain behaviour only from certain places is not good design.
I believe synthesizing the types instead of the files would be the easiest way, or following the nullable path
If KSP helps us with the former then there's our solution
y
yea i'm happy to hack this in XProcessing though we need the bug in 396 resolved at least when it happens in source
t
I'm still studying this. My impression so far is that FileKt hasn't been synthesized yet when processing happens. 
getClassDeclarationByName
may need some tweaks, too. I need to check how it is generated in compiler and see how it can be delivered in KSP.
OHTH, would the recently introduced 
fun getDeclarationsFromPackage(packageName: String): Sequence<KSDeclaration>
serve as an alternative for the need to access FileKt directly? We are also working on 
getFunctionDeclarationByName
, 
getPropertyDeclarationByName
All of those enables accesses to top level declarations. Sorry I'm still trying to catch up this thread and may ask stupid questions.
y
no worries. there are two issues here. one is migration from KAPT where these classes existed and does not exist anymore in KSP. I think for it, KSP can just say no we won't syntehsize those. The other more important issue is that if there is Java class in sources that accesses the generated class, then KSP fails to resolve it. That part seems like a bug that should be fixed. But then if we fix that, we might as well synthesize the class? Btw, synthesizing is not super trivial because kotlin allows multiple KSFile's to be merged into the same class via JvmName annotation
➕ 2
t
idea has a implementation for exactly the same purpose. Most of the logic resides in a common part of compiler and is available to KSP, including those that handle 
@JvmName
, etc. I guess it's fairly feasible to synthesize file classes in KSP. The other consideration is schedule; We may need to reprioritize existing tasks a little bit. Would this be a blocker for you guys?
y
this is not a requirement for Room except for the bug in source files case. cc: @gabrielfv @elihart for their requirements
e
Yeah, we have several libraries with top level functions/properties where this would be a blocker. I think that our use cases are pretty simple, and we just need to be able to 1) get an annotated top level construct 2) differentiate top level ones vs ones enclosed in classes 3) Access package information and be able to reference it So I think a basic workaround in XProcessing that can expose that information would work for now, but I know it would be ideal to have a solid long term solution in place. I haven’t had the time to fully understand the problem space to be able to make a recommendation on a solution though 😞
y
We can also change XProcessing executable to not require a containing typeElement if that is enough for your use case
keep in mind though, your processor will see different structures based on KSP vs KAPT and we probably cannot detect generated classes in KAPT case to hide it
e
I think it would be find to handle different structures in ksp vs kapt to some degree for now. having a nullable containing typeElement seems fine, and I can check for null as an indicator of a top level construct.
g
I believe it would be fine to set that as a longer term target and follow an easier short term solution, at least in my use case.
💯 1
y
so i played with a bunch of prototypes and every time i try to fake a type element for KSFIle, it explodes somewhere... Also, making it null is kind of OK but not really because you still need the 
className
to generate java code. So, I have a new prototype where I created a new type: XMemberContainer (name tbd). This interface provides two properties:
Copy code
className : ClassName
type: XType?
type is nullable to indicate that this is not a real type in kotlin but className is mandatory to aide with java codegen. @elihart / @gabrielfv If you can review this to see if it will address your use cases, that would be a huge help: https://android-review.googlesource.com/c/platform/frameworks/support/+/1684749 It is still a prototype but i think it will pass all tests (i didn't add many tests yet but updated @elihart’s tests to not throw for top level elements)
g
It certainly fits mine well enough, and looks like a good solution at least for now
👍 1
In fact it might be good enough for most scenarios I can think of
e
Just took a look through the code changes. This seems like a good approach. For our use case with top level members we only generate kotlin code, but supporting java would be good to and I think this abstraction seems simpler and less hacky. I don’t see a way in place yet to check whether a member is top level or not, is that simple to add? It seems difficult to do from the user side right now due to differences between ksp/kapt. I’m also wondering how easy it will be for a consumer to use this new interface to generate code that references the member, given the differences between kapt/ksp.
I also noticed that this error check wasn’t yet removed (and a similar one for properties)
Copy code
checkNotNull(enclosingContainer) {
                "XProcessing does not currently support annotations on top level " +
                    "functions with KSP. Cannot process $declaration."
            }
Thanks for making progress on this, I’ll try to finally address your feedback on my PR’s from last week
y
thanks for checking it. I'll continue on that CL then and will ping you when it is ready for review.
🙏 1
btw the check for enclosing container is still there but i don't think it will ever happen because KSP seems to be using the class if the top level function/member is coming from a dependency
@elihart that might impact your use case. though maybe i'll find a way to revert them as well. will add some tests to see if we can figure that out (though i doubt)
oh actually, when it is in lib, parent is null 😕
and containing file is also null 😮
(which is actually kind of good)
but we do have a qualified name that would be enough to generate code for it for kotlin, not from java though 😕
which means i think this member container will need to not require a 
className
😕
@Ting-Yuan Huang wdyt about adding another JVM API to get the JVM class for a top level member ?
t
You mean the file class? The idea sounds good to me, but why would we need a new API if we could use 
getClassDeclarationByName
? I mean, I'm thinking about synthesizing it (like what you suggested a few days ago) and not sure if there are other approaches?
One of the drawbacks of synthesized file classes is probably that it is JVM only. Making a specific function for it can prevent users from expecting it on other platforms.
Or do you mean that, e.g., getting the parent class (in any forms, TBD) of 
KSFunctionDeclaration
?
y
i actually tried synthesizing on the Room side and didn't like it much tbh. it gets complicated rather easily when the KSType does not exist (unless it exists in KSP internal, idk)
so my new solution does not synthesize it, instead, it has a 
MemberContainer
interface that has 
className
(that is class name in JavaPoet) which is enough to generate the java code
i can construct the className from 
KSFile
BUT i don't have KSFile when the top level function is in a dependency
so it becomes impossible to generate java code that will call that top level function (even though it works in KAPT)
so rather than bringing JVM structure into KSP by synthesizing a class, my recommendation is don't do it but instead provide:
Copy code
Resolver.getSyntheticJvmClass(member : (KSFunctionDeclaration/KSPropertyDeclaration): String (qualified name)
btw right now we have 
KSFunctionDeclaration.qualifiedName
which is enough to generate kotlin code to call it but not enough to generate Java code
t
You are right, synthesized class declarations don't have corresponding types and they will get complicated quickly. Getting the qualified name only is much cleaner.
y
i'll try to prortoype something, maybe i can catch the 1.5.0 related release (i assume there will be a ksp release soon to match kotlin 1.5)
👍 2
🙏 2
so i have this almost working. just 1 question remaining. What do we return 😄 The TypeMapper gives us an ASM type which has either binary name (where nested classes are separated by $) OR internal name (where they are separated by 
/
and 
$
for nested). I've not tried Arrays yet but from what i read in the source code, it will also add 
[]
for them
t
To be clear, resolution doesn't sound too bad to me. Let's keep the implementation simple.
Merged. Thanks!
y
oh that was quick 🙂
for XProcessing, we won't really call this API unless we need to
@elihart given that this is merged, is it fine if i wait for 1.5.0 update before updating xprocessing to support top level members ?
e
yes that’s fine, I can wait. Thanks both of you for getting this in so quickly!
g
Now I´m even more excited for 1.5.0
3 Views
Open in Slack
PreviousNext
Powered by