Support KSP in ContributesSubcomponentGenerator#828
Conversation
There was a problem hiding this comment.
I renamed this file without code changes first before proceeding, but github still shows them as a separate file. Sorry :/, idk what else to do about this case
There was a problem hiding this comment.
You're good, I can still see the diff by viewing the chunk of commits after the rename. Thanks for splitting it up!
There was a problem hiding this comment.
You're good, I can still see the diff by viewing the chunk of commits after the rename. Thanks for splitting it up!
| internal fun KSAnnotation.replaces(): List<KSClassDeclaration> = | ||
| (argumentAt("replaces")?.value as? List<KSType>).orEmpty().mapNotNull { | ||
| it.resolveKSClassDeclaration() | ||
| } | ||
|
|
||
| @Suppress("UNCHECKED_CAST") | ||
| internal fun KSAnnotation.exclude(): List<KSClassDeclaration> = | ||
| (argumentAt("exclude")?.value as? List<KSType>).orEmpty().mapNotNull { | ||
| it.resolveKSClassDeclaration() | ||
| } |
There was a problem hiding this comment.
Did you come across any cases here where it was expected to get a null back? I'm thinking we should throw an exception if one of the replaces/exclude types can't be resolved, otherwise if I'm reading correctly we'll silently skip one or more types.
There was a problem hiding this comment.
It's possible if it refers to a type that's not yet been generated. In theory, the correct thing to do here would be to track this original type and return it to KSP to defer them to a later round. We don't currently support this in embedded code gen though, and there's not currently a way to easily defer these. This matches the current embedded behavior, and would be worth a wider cleanup in a later PR to add support for deferring unresolved types.
There was a problem hiding this comment.
Agree on the first part around deferring eventually 👍. Can you point me to the equivalent skipping logic in the embedded impl (or relevant test if you came across one)? When I reviewed before I didn't see that, and it looked like this case would bubble up as an exception somewhere along the line while creating the reference classes for the annotation arguments. It's possible I missed it since the embedded impl has a lot more levels of abstraction before getting to specifically looking for the exclude arguments, just want to verify that the behavior is the same.
There was a problem hiding this comment.
It will accept the ClassReference list as-is here, but default to empty if that value is null.
PSI will offer an error element for this case, but ClassReference doesn't ever account for these. Instead, the pattern I see is more or less that behavior is either undefined or it gracefully avoids those situations. In this case I think it's the latter, but tbh I'm not super sure how the PSI lookups for classreferences work just glancing at the code. We can make them error if you want though 👍
There was a problem hiding this comment.
(Note that exclude() is not used in this PR, just opportunistically added it since it was missing. replaces() is what's used.)
There was a problem hiding this comment.
It will accept the ClassReference list as-is here, but default to empty if that value is null.
PSI will offer an error element for this case, but ClassReference doesn't ever account for these. Instead, the pattern I see is more or less that behavior is either undefined or it gracefully avoids those situations. In this case I think it's the latter, but tbh I'm not super sure how the PSI lookups for classreferences work just glancing at the code. We can make them error if you want though 👍
Gotcha, yea the default to empty on the argument itself makes sense since it might not have been set. My main concern is around if one of the types in a provided argument list isn't resolvable resulting in a null value.
I would say let's go ahead throw an exception in that case for the KSP impl for now (unless it reveals some tests covering this case where we don't throw an exception with the embedded impl). It's simple enough to change it back later if we find that there are cases where we want to ignore an unresolvable type. Basically just:
map {
it.resolveKSClassDeclaration() ?: throw ...
}
| clazz.getKSAnnotationsByType(ContributesSubcomponent::class) | ||
| .forEach { annotation -> | ||
| for (it in annotation.replaces()) { | ||
| val scope = annotation.scope().resolveKSClassDeclaration() ?: continue |
There was a problem hiding this comment.
Similar thought here on throwing an exception if the type isn't found
|
@JoelWilcox sorry for the delay. I've merged latest main and added the throwing behavior requested: 1717ce5 |
No worries, thanks for updating it! |
2 participants
Ref #751