Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sealed interfaces and sealed classes freedom #226

Open
elizarov opened this issue Nov 10, 2020 · 14 comments
Open

Sealed interfaces and sealed classes freedom #226

elizarov opened this issue Nov 10, 2020 · 14 comments

Comments

@elizarov
Copy link
Contributor

elizarov commented Nov 10, 2020

The goals of this proposal are:

  • Introduce the concept of a sealed interface.
  • Allow more freedom to sealed class, unify both sealed interface and sealed class as the same concept.
  • Seamlessly interoperate with JDK 17+ sealed classes and interfaces.

Proposed text: https://github.com/Kotlin/KEEP/blob/master/proposals/sealed-interface-freedom.md

@JakeWharton
Copy link

For a sealed interface this proposal is not to attempt any protection scheme, but just compile an interface as if it was not sealed. We will introduce an IDE checker for the Java code that is attempting to extend or implement a sealed Kotlin interface.

I do not like this behavior being allowed by default when jvmTarget is lower than 15. It is very dangerous for published libraries where the consumer may be in Java and not using a version of IntelliJ with the checker or using another IDE altogether. Moreover, even in Kotlin it's dangerous as users of j.l.r.Proxy and libraries like Mockito can create subtypes in ways that cannot be checked by any compiler.

I would like to advocate for an opt-in flag which allows the use of sealed interface when the jvmTarget is lower than 15. Something like -Xunchecked-sealed-interface.

If someone lowers the jvmTarget from 15 to 14, or simply creates a new project targeting Java 11 and tries to use sealed interface I would like the build to fail by default.

@elizarov
Copy link
Contributor Author

We plan to introduce sealed interface as a feature of the Kotlin language that is unconditional on the backend or compilation target that you are using. It is indeed, unfortunate, that there is no easy way to enforce sealed interface restrictions on JDK<15.

We've discussed what kind of harm it could cause for JDK<15 users. Consider an analogy. With Java code (especially with reflection) you can sneak a null value into a non-null Kotlin field with relative ease. Kotlin checks for it in various places, but it is not very fool-proof, so a null value can travel for a while around Kotlin, breaking something somewhere. We never strive to provide a total guarantee. Being good enough and adding value is the key differentiator in the pragmatic design approach of Kotlin.

With a sealed interface some Java code can create an unexpected instance of a sealed interface. Same story. Every time Kotlin compiler compiles an exhaustive when(x) statement it inserts an else branch just in case an unexpected value appears, so an unexpected instance of a sealed interface can break something but will get caught somewhere, too.

Indeed, -Xunchecked-sealed-interface would be a useful command-line option for maintainers of stable libraries, but the needs of library writers are secondary in Kotlin design -- the Kotlin is primarily designed to be convenient for writers of applications. We assume that users that (for whatever business reasons) need strong enforcement of sealed interface guarantees will either target JDK>=15 or avoid using sealed interfaces. However, we don't expect that the users needing strong enforcement will constitute any considerable fraction of all Kotlin users.

@JakeWharton
Copy link

Reflection is a big hammer that almost guarantees violation of invariants. I'm not sure it's the best comparison.

Polymorphism (i.e., implementing an interface) is an exceedingly normal thing. I think it's more apt to compare it to the handling of null for parameters by Kotlin. After all, both are refinements of the type system in a way that the VM cannot represent or enforce (prior to 15).

Why then does Kotlin emit intrinsic checks for null references for a function's parameters? Couldn't it omit those and use the IDE to check Java callers in the same way this proposal means to?

Or, put a different way, a potential solution for jvmTarget<15 is to emit intrinsic checks which validate the actual type of a sealed interface is one of the permitted subtypes. Was that considered?

@elizarov
Copy link
Contributor Author

The difference between null checks and sealed interfaces is just in the perceived scale of the problem and is a matter of a pragmatic compromise. It is well-known that NPE is a big problem in the real-life Java code, so it is worth taking the extra mile to ensure that sneaky nulls are caught as soon as possible. However, it does not mean we go all over our heads to totally get rid of nulls. For example, we do add null checks for the parameters of public methods, since we know that it is relatively cheap on JVM and is worth added bytecode due to the large scale of null problem in JVM.

The story with sealed interfaces is totally different. Java<15 does not have any support for sealed interface enforcement, yet people write interface-based APIs all the time where they just trust their users not create instances of unexpected types. They just assume it or write it in docs. Unlike the story with nulls, you will not find dozens of blogs complaining on that problem and, in general, you will not find users suffering from it in any noteworthy way. We know from our own API design experience the actual cases where we might use sealed interfaces in our design and we don't believe that ability for a user to sneak an authorized implementation of these interfaces would be anyhow ruining UX of the corresponding libraries.

Every day we rely on documentation in all our APIs. We enforce things through the type system whenever possible but, at the end, there are always tons of complex contracts that are explained only in the documentation and that you must abide, or else something breaks.

However, sealed interfaces on JDK<15 are going to behave much better than just spelling the corresponding constraint in the documentation. They are a useful addition to a library-writer toolkit, even if we put aside how useful they are to an application developer. You will get exhaustiveness help from the Kotlin compiler, you will get meaningful errors on an attempt to extend a sealed interface from a different package/module in Kotlin, you will get errors from IDEA when you try to implement it in Java. Moreover, if somebody finds it to be a serious problem that compiling the code via javac that tries to extend a Kotlin sealed interface does not produce any error, then we can provide a javac processor for this check in the future.

Of course, if we were talking about the problem on the scale of NPE then all of the above would not have been enough, but we don't see any evidence to substantiate the claim that "unchecked extension of a sealed interface" is going to be any sort of a major problem to be worth added inconvenience to an average Kotlin user who has zero code in their project that would ever try to extend any of their Kotlin interfaces from Java. We also assume that those (advanced) users who develop Kotlin APIs for consumption from Java would understand the tradeoffs and will be able to make their own decision on their use of Kotlin sealed interfaces.

@BenWoodworth
Copy link

Would adding a synthetic method to sealed Kotlin interfaces with special characters work for preventing implementation from Java? If we had a sealed class called Example, we could compile it to something like:

interface Example {
    //@JvmSynthetic // Java classes doesn't have to implement synthetic methods, so this doesn't help
    fun `sealed-interface-marker`()
}

Then it can't be implemented by a Java class, since sealed-interface-marker can't be overridden.

@elizarov
Copy link
Contributor Author

@BenWoodworth Yes. Just adding a non-Java name is enough to ensure that it cannot be implemented from Java.

@elizarov
Copy link
Contributor Author

UPDATE: JDK 16 is not going to have a stable sealed interface/classes and is running a second preview (see JEP 397), so we've decided that the implementation for emitting of PermittedSubclasses attribute and the recognition of the corresponding JVM attribute shall be postponed until JDK 17 or later when it becomes stable. The whole bunch of Kotlin-language improvements (more freedom for classes and sealed interfaces) are on track to be available in Kotlin 1.5 and will be available for early preview starting from Kotlin 1.4.30-RC.

@spand
Copy link

spand commented Feb 9, 2021

I have prototyped replacing our use of classpath scanning with sealed interfaces instead (traversing .sealedSubclasses). It works great but when these hierarchies reach > 100 classes the "same package" restriction is getting annoying. It would be much nicer to subdivide files into folders for clarity and organization. Why is this rule in place ? (even when considering that folder/package does not need to match in Kotlin). In my use case I guess "same or sub package" would be flexible enough but I lack a rationale for the rule in the first place.

@elizarov
Copy link
Contributor Author

elizarov commented Feb 9, 2021

@spand I have prototyped replacing our use of classpath scanning with sealed interfaces instead (traversing .sealedSubclasses). It works great but when these hierarchies reach > 100 classes the "same package" restriction is getting annoying. It would be much nicer to subdivide files into folders for clarity and organization. Why is this rule in place ? (even when considering that folder/package does not need to match in Kotlin). In my use case I guess "same or sub package" would be flexible enough but I lack a rationale for the rule in the first place.

The rationale is multiple-fold. There is this basic rule: don't add a feature unless we have an explicit use-case at hand. But in this particular case there are some specific constraints:

  • It makes it easier/faster to implement in IDE (we don't need to scan the whole project -- just a package).
  • It makes it easier/safer to interop with future sealed classes in Java, which has some funky rules w.r.t same/different package. In Java, non-public class cannot extend a public sealed class from another package (see the quote from the specs below).

Otherwise, if the class named as the direct superclass of C has a PermittedSubclasses attribute (4.7.30) and any of the following are true, derivation fails with a IncompatibleClassChangeError:

  • The superclass belongs to a different run-time module than C.
  • C does not have its ACC_PUBLIC flag set (4.1) and the superclass belongs to a different run-time package than C.
  • No entry in the classes table of the superclass's PermittedSubclasses attribute references a class or interface with name N.

Since Java design is still in a flux, it is simply too riskly to commit to some specific rules right now, but further relaxation might be possible in the future (esp. if we figure out a faster way to implement it in IDE).

I suggest to open a separate issus here and explain your use-cases of having too many classes for a single package in more detail.

@spand
Copy link

spand commented Feb 12, 2021

Thank you. I have written up a bit about our use case in this issue: https://youtrack.jetbrains.com/issue/KT-44895

@andreasnomikos
Copy link

... then we can provide a javac processor for this check in the future.

I 'd like to second this and upvote it on youtrack if you have an issue for it already. We had multiple cases at uber where we wanted to utilize sealed hierarchies and had to resort to custom ErrorProne checkers.

@elizarov
Copy link
Contributor Author

@andreasnomikos I 'd like to second this and upvote it on youtrack if you have an issue for it already. We had multiple cases at uber where we wanted to utilize sealed hierarchies and had to resort to custom ErrorProne checkers.

Can you, please, elaborate on what exactly has stopped you from utilizing Kotlin sealed class hierarchies?

@andreasnomikos
Copy link

@elizarov Basically we wanted to utilize sealed class hierarchies to define events across module boundaries (parent-child relationships) to replace our listeners, but because the codebase is still a mixture of java and kotlin modules we didn't have a way to enforce sealed class semantics in java consumers. We were considering a custom ErrorProne plugin looking at the class metadata. Enforcement was mostly to ensure that switch statements in the parent module are exhaustive over the different types of events to achieve the same compile-time checks as listeners. (if the child module adds a new event in the api then all parents MUST handle the new event)

@mgroth0
Copy link

mgroth0 commented May 29, 2023

@elizarov I second @spand 's desire to spread a sealed hierarchy throughout multiple packages in a module. My use case is that I want the code defining the sealed hierarchy to be ... hierarchal. It will make the code much more organized. Example:

src/main/kotlin/animals/animal.kt

sealed class Animal

src/main/kotlin/animals/mammals/mammal.kt

sealed class Mammal: Animal

src/main/kotlin/animals/mammals/cats/cat.kt

sealed class Cat: Mammal

src/main/kotlin/animals/mammals/cats/tiger.kt

sealed class Tiger: Cat

src/main/kotlin/animals/mammals/cats/lion.kt

sealed class Lion: Cat

For context, the main two reasons I use sealed classes are:

  • polymorphic serialization
  • typesafe when statements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants