Make hierarchy in KSP API sealed (attempt 2) #1592
Conversation
lukellmann
force-pushed
the
sealed-hierarchies
branch
from
431bc8f
to
503a03f
Compare
|
|
||
| override val origin by lazy { | ||
| open /*override*/ val origin by lazy { |
There was a problem hiding this comment.
I'm a bit concerned with this. Having to cast the implementations is not only inconvenient, but also error prone and hard to maintain. Is there a better way to do it?
There was a problem hiding this comment.
this approach was the one with minimal changes. another one i can think of is to have these abstract classes but override and upcall in all implementation classes. this would be a bigger change but also less error prone.
There was a problem hiding this comment.
what about this: cda8056 (replacing casts with abstract functions with the desired return type that are overridden to simply return this)
Interfaces that were sealed: KSAnnotated, KSDeclarationContainer, KSReferenceElement, KSModifierListOwner, KSPropertyAccessor, KSDeclaration and PlatformInfo
Abstract implementation classes that inherited from now sealed types have those supertypes removed and replace overridden with open members. Additional casts had to be introduced in a few places. A few when statements had to be made exhaustive (by specifying all cases) because they now operate on subjects with a sealed type.
KSNodeDescriptorImpl, KSNodeJavaImpl and KSNodeKtImpl were removed because they were unused but inherited from the now sealed KSNode.
lukellmann
force-pushed
the
sealed-hierarchies
branch
from
503a03f
to
cda8056
Compare
|
After thinking this again, I feel it might not be right to make the API sealed. The class hierarchy may be expanded in the future to reflect language changes. Making them sealed will make it very tricky. Could you justify why sealing it is the way to go? |
|
Like described in #1351, the motivation was to be able to have the compiler enforce code changes when the hierchy is changed. So changes in the hierachy leading to compiler errors in sealed |
To be clear, you are talking about the API side facing the KSP user here, right? Or do you also mean something else? |
|
Making the API sealed only helps discover source level incompatibility. From library users' point of view, sealed interface / class isn't truly sealed. A unhandled case can still arise in an exhaustive-when in the user code. If It'd also be tricky when API users try to be future compatible; If their |
And that's fine. The goal is to make writing the code easier. An unhandled case at runtime will throw and clearly tell what went wrong - the processor isn't built for handling this new feature.
How would it be silent? Non-exhaustive
Tools working on source code (like KSP processors) will always break on new language features, in one way or another. The whole point is to get help by the compiler when things change. Visitors can still be used when preferred.
I'd argue if a user really wants to be future compatible to this extent, a suppression would be ok. |
| return (container.declarations as? Sequence<AbstractKSDeclarationImpl>) | ||
| ?.sortedWith(declarationOrdering.comparator) ?: container.declarations | ||
| return container.declarations | ||
| .sortedWith(compareBy(declarationOrdering.comparator) { it as AbstractKSDeclarationImpl }) |
There was a problem hiding this comment.
explanation:
This was an unchecked cast (generics involved) and would always succeed - so replacing as? with as shouldn't change any behavior.
# Conflicts: # common-util/src/main/kotlin/com/google/devtools/ksp/common/IncrementalContextBase.kt # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/AbstractKSDeclarationImpl.kt # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/KSTypeAliasImpl.kt
# Conflicts: # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/AbstractKSDeclarationImpl.kt
Another try of #1380 with additional compilation fixes.