Consider using sealed hierarchies in the KSP API to allow exhaustive whens

[lukellmann]

Here is a real-world example that could benefit from sealed hierarchies in the KSP API:

/**
 * The binary name of this class-like declaration on the JVM, as specified by
 * [The Java® Language Specification](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1).
 */
val KSClassDeclaration.jvmBinaryName: String
    get() = when (val parent = parentDeclaration) {
        // this is a top-level class-like declaration -> canonical name / fully qualified name (same for top-level)
        null -> this.qualifiedName!!.asString()

        // this is a member class-like declaration -> binary name of immediately enclosing declaration + $ + simple name
        is KSClassDeclaration -> parent.jvmBinaryName + '$' + this.simpleName.asString()

        is KSFunctionDeclaration, is KSPropertyDeclaration ->
            error("jvmBinaryName isn't implemented for local class-like declarations but $this seems to be one")
        is KSTypeAlias, is KSTypeParameter -> error("$parent shouldn't be the parentDeclaration of $this")
        else -> error("$this has an unknown parentDeclaration: $parent")
    }

If KSDeclaration was a sealed interface, there would be no need to provide an else. In the hypothetical scenario that a new subtype of KSDeclaration was introduced, a compile error would arise because the when wouldn't cover all possible cases anymore (which is desirable if one wants to be exhaustive and handle all cases correctly).

jvmBinaryName could also be implemented using a visitor and can even also result in compiler errors (because of new non-overridden methods from KSVisitor) when new KSNodes are introduced, however the implementation is more complex and requires more code:

val KSClassDeclaration.jvmBinaryName get() = accept(JvmBinaryNameVisitor, data = this)

private object JvmBinaryNameVisitor : KSVisitor<KSClassDeclaration, String> {

    override fun visitClassDeclaration(classDeclaration: KSClassDeclaration, data: KSClassDeclaration): String {
        val parent = classDeclaration.parentDeclaration
        return if (parent == null) {
            classDeclaration.qualifiedName!!.asString()
        } else {
            parent.accept(this, classDeclaration) + '$' + classDeclaration.simpleName.asString()
        }
    }


    private fun localError(clazz: KSClassDeclaration): Nothing =
        error("jvmBinaryName isn't implemented for local class-like declarations but $clazz seems to be one")

    override fun visitFunctionDeclaration(function: KSFunctionDeclaration, data: KSClassDeclaration) =
        localError(clazz = data)

    override fun visitPropertyDeclaration(property: KSPropertyDeclaration, data: KSClassDeclaration) =
        localError(clazz = data)


    private fun wrongParentError(parent: KSDeclaration, child: KSClassDeclaration): Nothing =
        error("$parent shouldn't be the parentDeclaration of $child")

    override fun visitTypeAlias(typeAlias: KSTypeAlias, data: KSClassDeclaration) =
        wrongParentError(parent = typeAlias, child = data)

    override fun visitTypeParameter(typeParameter: KSTypeParameter, data: KSClassDeclaration) =
        wrongParentError(parent = typeParameter, child = data)


    private fun error(symbol: KSNode): Nothing =
        error("Unexpected symbol $symbol, JvmBinaryNameVisitor should only visit subtypes of KSDeclaration")

    // repeat for all other visit<Node> methods, if a new one is added, there will be a compile error until the method is overridden
    override fun visitNode(node: KSNode, data: KSClassDeclaration) = error(node)
    override fun visitAnnotated(annotated: KSAnnotated, data: KSClassDeclaration) = error(annotated)
    // ...
}

I think the following types would benefit from being sealed: KSNode, KSAnnotated, KSDeclarationContainer, KSReferenceElement, KSModifierListOwner, KSPropertyAccessor, KSDeclaration (the one that would benefit the example above) and also com.google.devtools.ksp.processing.PlatformInfo.

[lukellmann]

Here you can see how this could be implemented in KSP: lukellmann/ksp@97774aa...910985a (see commit messages for details)

If this change is desired, I can open a pull request.

In addition, some KSVisitor methods could be deprecated gradually in these steps: lukellmann/ksp@910985a...e8fcab3 (however, this is optional and only related to the main request of this issue)

[neetopia]

This is a good suggestion. One reason I can think of as to why this is not happening before was actually that prior to Kotlin 1.5, sealed classes has to be in the same file 🤦 .

That said, I will still need some time to think about other potential implications of making it sealed inheritance, but if nothing major comes to my mind, I am favoring this suggestion.

[lukellmann]

One obvious implication is that you will no longer be able to extend those types outside of KSP. However, I don't know why you would have to.

[lukellmann]

It would also mean that code can then use exhaustive whens on these types and when a new subtype to one of the sealed types is introduced, there would be a compiler / runtime (when the new type is matched) error.
However, as described in the issue text, I think this is a good thing, ensuring uncovered cases don't have unexpected behavior.

[lukellmann]

That said, I will still need some time to think about other potential implications of making it sealed inheritance, but if nothing major comes to my mind, I am favoring this suggestion.

Did you have time for this yet? If so, what are your thoughts? Should I open a PR?

[neetopia]

Hi I will be on vacation soon so probably not the best time for handling this. Would you mind wait for 3 more weeks?

[lukellmann]

That's fine for me.

[lukellmann]

So, what's the state of this?

[neetopia]

Hi thanks for the reminder, I've just come back.

Overall I think this looks to be a good direction to move forward. Like you mentioned there are a couple of good candidates to be adopted as sealed classes, when you said you want to open PR, which classes you have in your mind for this? I think it is better to start with smaller scopes to simplify the effort (i.e. leave out KSNode for now)

[lukellmann]

Like you mentioned there are a couple of good candidates to be adopted as sealed classes, when you said you want to open PR, which classes you have in your mind for this?

I was thinking of KSAnnotated, KSDeclarationContainer, KSReferenceElement, KSModifierListOwner, KSPropertyAccessor, KSDeclaration and PlatformInfo.

I think it is better to start with smaller scopes to simplify the effort (i.e. leave out KSNode for now)

The effort to also seal KSNode is actually quite small, see this commit.

[neetopia]

Hi, after some discussion internally, we would like to proceed with this proposal, but as this still introduces some risks of breaking change (e.g. else clause logics are effectively changed after changing to sealed interfaces), we would also like to hold on the change until a future major KSP release (maybe at end of the year)

[lukellmann]

Alright, thanks for looking into it!

[lukellmann]

[...] we would also like to hold on the change until a future major KSP release (maybe at end of the year)

Is there any update on this? Is a new major release planned? Would including this change make sense?

[neetopia]

It's currently planned for 1.9.20 release.

[lukellmann]

It's currently planned for 1.9.20 release.

Nice. Let me know if there needs to be work done on #1380 so that it can be included.

[lukellmann]

I've updated #1380 to resolve merge conflicts.

[lukellmann]

[...], but as this still introduces some risks of breaking change (e.g. else clause logics are effectively changed after changing to sealed interfaces), [...]

Could you elaborate how the logic of else clauses would change? I don't really understand what you mean here.