Make hierarchy in KSP API sealed (attempt 2)#1592
Make hierarchy in KSP API sealed (attempt 2)#1592lukellmann wants to merge 16 commits intogoogle:mainfrom
Conversation
lukellmann
force-pushed
the
sealed-hierarchies
branch
from
431bc8f to
503a03f
Compare
...in/src/main/kotlin/com/google/devtools/ksp/symbol/impl/binary/KSDeclarationDescriptorImpl.kt
Outdated
Show resolved
Hide resolved
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
# Conflicts: # 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/KSFunctionDeclarationImpl.kt # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/KSPropertyAccessorImpl.kt # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/KSPropertyDeclarationImpl.kt
# Conflicts: # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/AbstractKSDeclarationImpl.kt
|
What's the state of this PR, @ting-yuan? Does the proposed change have a chance to be merged? |
# Conflicts: # compiler-plugin/src/main/kotlin/com/google/devtools/ksp/symbol/impl/kotlin/KSPropertyAccessorImpl.kt # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/ResolverAAImpl.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/KSFunctionDeclarationImpl.kt
|
also: should i merge or rebase + force-push to resolve conflicts? |
# Conflicts: # 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 # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/KSPropertyAccessorImpl.kt
# Conflicts: # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/KSPropertyAccessorImpl.kt
# Conflicts: # kotlin-analysis-api/src/main/kotlin/com/google/devtools/ksp/impl/symbol/kotlin/KSPropertyAccessorImpl.kt
2 participants
Another try of #1380 with additional compilation fixes.