SIP public review: Open classes

[sjrd]

Hi Scala Community!

This thread is the SIP Committee’s request for comments on a proposal to introduce open classes in the language. An open class is explicitly designed to be extended by subclasses, with the ability to override non-final methods with a clear contract. Classes that are neither open, sealed nor final will by default be soft-sealed. Trying to extend a soft-sealed (normal) class from a different compilation will result in a warning, which can be silenced with a language import. You can find all the details here.

Motivation

For motivation, please read Open Classes.

Summary

When declaring a class in some file A.scala, we can mark it with the open modifier to communicate that it has been planned for extension:

open class A {
  def someMethod(): Int = 42
  def someOtherMethod(x: Int): Int = x + someMethod()
}

We can extend such a class from anywhere. An open class would typically come with a precise extension contract that describes internal calling patterns between the methods of the class. This is different from (and typically more complicated than) the external contract of a class which specifies how a class behaves when it is used. For example, the external contract of A would specify that someOtherMethod(x) returns x + 42, but the extension contract would have to specify that it returns x + someMethod(). The latter is required to make sense of what happens when someMethod() is overridden by a subclass. Once that contract is established, it is no longer valid to refactor the implementation of someOtherMethod() to be x + 42.

If we try to extend a non-open class from a different source file, a new compilation warning will be emitted:

-- Feature Warning: EncryptedWriter.scala:6:14 ----
  |class EncryptedWriter[T: Encryptable] extends Writer[T]
  |                                              ^
  |Unless class Writer is declared 'open', its extension in a separate file should be enabled
  |by adding the import clause 'import scala.language.adhocExtensions'
  |or by setting the compiler option -language:adhocExtensions.

As an escape hatch—for example if you want to patch up a class in a library by overriding one its methods—, you can silence that warning with the following language import:

import scala.language.adhocExtensions

Alternatives

During the initial discussion by the SIP committee on this feature, a few alternatives have been mentioned:

• Make non-open classes eligible for extension in the enclosing package or even project, without requiring the language import. The notion of project is not well-defined in Scala, so only talking about the enclosing package is realistic.
• Turn the problem around: do not emit a warning a extension site, but instead emit a warning at definition site if we declare a class that is neither final nor sealed nor open. This would be opt-in with a compiler option (a linter flag, really) which would typically be used by all libraries. @odersky objects that even beginners with the language should put careful consideration into declaring a class as open.

Discussion

A previous discussion on the topic can be found at

SIP: Make classes `sealed` by default

Scala extends Java’s basic class system, using the same notions for class, abstract class and final class. In hindsight, I believe the default in this system that every class is extensible is problematic. It leads to straightforward code being vulnerable to unforeseen overrides and requires final modifiers in too many places. This pre SIP is a proposal to change the default class extensibility to sealed. It can be summarized as follows: A normal class definition implies that the class is sea…

In that discussion, the main objections were:

• Users of libraries who want to “patch up” some bugs by extending library classes that were not carefully designed for their use case, and override some method. This is still possible with this proposal, simply by using the appropriate language import. Such patches are hacks that precisely counteract the fact that the library was not designed “well enough”; it makes sense for such hacks to require a language import.
• Test suites that use a lot of mocking, where the project’s classes are automatically extended by macros. This is addressed by using the command-line switch corresponding to the language import (-language:adhocExtensions). It would also be addressed by the alternative where a non-open class can be extended in the entire package, not just the same file.

Opening this Proposal for discussion by the community to get a wider perspective.

[dwijnand]

As a library author, I find the guarantees of no modifier to be too weak. How would you feel if it were an error by default instead, and the language import makes it compile?

Also, you mentioned that your use case is before every library release you feel the pressure to audit the whole codebase for any unintentionally non-final classes. Perhaps, instead of adding to this axis and making it more complex, why not setup the MissingFinal scalafix rewrite, and opt-out where you’ve purposely designed it for extension?

[sjrd]

@dwijnand

As a library author, I find the guarantees of no modifier to be too weak. How would you feel if it were an error by default instead, and the language import makes it compile?

I think it is unrealistic to make it an error tomorrow. However, it could be turned into an error in a few versions.

@dwijnand

Also, you mentioned that your use case is before every library release you feel the pressure to audit the whole codebase for any unintentionally non-final classes. Perhaps, instead of adding to this axis and making it more complex, why not setup the MissingFinal scalafix rewrite, and opt-out where you’ve purposely designed it for extension?

This is basically the second alternative I mentioned in the initial post. I guess @odersky’s objection applies to this variant as well.

[bergmark]

As an escape hatch—for example if you want to patch up a class in a library by overriding one its methods—, you can silence that warning with the following language import:

Usually I want to disable warnings only in specific places, rather than for a whole file (or the whole project). That makes migration easier when you want to gradually start enforcing things like this. It also prevents you from accidentally silencing the warning in other places. Any way to get an option to do that in this proposal, or does that belong as a more general feature request for any compiler warning? We’re currently using silencer, but it would be nice if this was supported by the compiler.

[sjrd]

Of course. The language import doesn’t have to be at the top of the file. As with any other import, you can scope it to precisely the location where you need it

[bergmark]

Maybe I don’t know the full scala syntax then! How would you scope this import to a top level class declaration?

[sjrd]

Oh yes for a top-level class declaration you’re a bit out of luck. Depending on the API guarantees that you then offer to your users, the following workaround may help:

package your.pack

import someone.elses.NonOpenClass

object NonOpenClassExtensions {
  import scala.language.adhocExtensions

  class OpenTheNonOpenClass extends NonOpenClass
}

class MyExtension extends NonOpenClassExtensions.OpenTheNonOpenClass

[dwijnand]

@sjrd

I think it is unrealistic to make it an error tomorrow. However, it could be turned into an error in a few versions.

Can it? Can dotc --from-tasty foo.tasty v3.4 fail to compile where with v3.3 it compiled?

[bergmark]

I very much like the feature by the way!

[sjrd]

No recompiling from tasty cannot turn a non-error into an error. But recompiling from source can.

[lrytz]

It feels to me that this is a departure from Scala as I know it. Until now, by default, things are public and open, and you have to think about restricting your API. Now you have to think about opening as well.

The fact that the default class C easily leads to warnings seems quite annoying, and gives people the feeling that they’re doing something wrong. In the REPL a special case would be needed, otherwise writing class C and then class D extends C on the next line would warn.

One aspect that bothers me is that the proposal changes the semantics of existing code. When upgrading, a library author needs to go through all the code and make sure he doesn’t miss marking the right (or all non-abstract/sealed/final) classes open, otherwise clients break.

The feature is also problematic for cross-building with 2.13 – should it be disabled in Scala2-mode?

Since we are in the context of restricting subclassing: was it considered to make sealed transitive (propsed by @lihaoyi on the old thread)?

[lihaoyi]

One consideration is that we already have a concept of “classes to be extended”: these are classes with abstract methods, and implementing abstract methods thus doesn’t require any annotation at all.

Furthermore, we already have a concept of “things that may not be intended for extension, but you can override methods with an annotation”: that is what the override keyword is for!

This proposal would be adding a third level of open-ness, moving the current "override keyword" to "override keyword with language import", and adding a “Needs override but doesn’t need language import” level of open-ness that you can mark a class with via open.

I also think that adding another language-import/warning will not have any significant impact on how people write Scala. I don’t think import scala.language.experimental.macros has in any way dissuaded people from writing macros, nor has import scala.language.higherKinds dissuaded people from using higher kinded types. My fear is that open will likely turn into just another nuisance warning that people ignore in the compile logs, and another nuisance Cmd-Enter auto-fix ritual for your IDE to insert a language import, when everyone (you, your compiler, your editor) all already know what you want.

From that perspective, three levels of open-ness rather than two is a bit more granularity and a bit more control, but I don’t think it crosses the bar as something useful enough that it needs to go into the language.

[sjrd]

@lrytz

It feels to me that this is a departure from Scala as I know it. Until now, by default, things are public and open, and you have to think about restricting your API. Now you have to think about opening as well.

This difference is justified by the fact that an extension contract is much more difficult to think about, and is often not thought about at all, than an external contract. Especially in Scala where most things are immutable, external contracts are easy to specify.

And more importantly, if you screw up the external contract of a method, you can deprecate it, leave it there for compatibility and then not think about it anymore. You cannot do that with the extension contract of a class. You cannot deprecate the problematic pieces and then not think about them. You have to keep them in mind forever, and always call them with the right “timing”, in order not to break compatibility.

This is why an external contract can be public by default, whereas an extension contract should only be opened if it has specifically been designed and thought through as such.

@lrytz

The feature is also problematic for cross-building with 2.13 – should it be disabled in Scala2-mode?

We can probably disable it in Scala 2 mode. We could also introduce a no-op open modifier in 2.13.x, to ease cross-compilation.

@lrytz

Since we are in the context of restricting subclassing: was it considered to make sealed transitive

The problem with that is that there are legitimate use cases for sealing only one level, although I will admit they are pretty rare. We would need an escape hatch to mark a subclass of a sealed class as non-sealed… Oh wait, that’s exactly what open means! I like this

[soronpo]

Maybe a middle ground approach. Disable this feature by default and allow the library author to opt-in via -YclassesClosedByDefault (so no need for special Scala 2 options here). So if I’m a library author who doesn’t want to forget final by mistake I can just turn on this flag.

Regarding case classes though I think they should be final by default without any exception.

[RichType]

For me the primary purpose of sealed is for folds. As I’ve suggested else where I’d prefer a self type that guarantees that the final class / object inherits from a set of traits/ classes. So I feel sealed is already unnecessarily restrictive.

I don’t like the current proposal at all. Hasn’t Scala got enough of these irritating imports. I think they’re irritating for many experienced Scala programmers, but can be downright confusing for less experienced Scala programmers. I know I was thrown by them in the past. They seem to me to go against the whole principle of scalable language, where you can gradually increase the sophistication of your code. At the very least I’d like to hear from some developers who actually found the warnings useful.

We seem to have swung from one extreme to another. For years change in Scala has been frustratingly slow. For example trait parameters, an actually useful feature, is listed as accepted in June 2015, but still hasn’t been implemented. Now it seems like everything must be debated agreed and changed in a matter of weeks. Why can’t we make case classes final by default. Introduce the open keyword, but make the warnings for extending non explicit-open classes opt in rather than opt out?

[sjrd]

@lrytz

One aspect that bothers me is that the proposal changes the semantics of existing code. When upgrading, a library author needs to go through all the code and make sure he doesn’t miss marking the right (or all non-abstract/sealed/final) classes open , otherwise clients break.

If a class was intended to be open before the upgrade, the library would have tests that test the extension and override scenarios. Those tests would break during the upgrade (since the tests are not in the same file) so the maintainer would know to add the open modifier too their class before releasing anything.

If the library didn’t have tests that extend the class, then it’s clear it wasn’t intended to be extended, and it is the whole point of this proposal to cause warnings when trying to extend such a class!

[sjrd]

@lihaoyi

One consideration is that we already have a concept of “classes to be extended”: these are classes with abstract methods, and implementing abstract methods thus doesn’t require any annotation at all.

Furthermore, we already have a concept of “things that may not be intended for extension, but you can override methods with an annotation”: that is what the override keyword is for!

I disagree with this interpretation. There are a number cases of classes that have 0 abstract method but that are still intended for extension and where the overriding contracts are well-defined. In fact most of my truly open classes (the ones I would add open to) are concrete.

If override were the legitimate way of saying “I’m using an escape hatch now to patch that library which did not cater to my use cases”, then we should be asking for a language import every time someone uses the override keyword!

The override keyword itself is way too natural to communicate the dangerousness of what one is doing. So it clearly does not solve the issues presented in the motivation.

@lihaoyi

My fear is that open will likely turn into just another nuisance warning that people ignore in the compile logs, and another nuisance Cmd-Enter auto-fix ritual for your IDE to insert a language import, when everyone (you, your compiler, your editor) all already know what you want.

But most of us don’t know what we want! We don’t understand that when we extend a class that was not meant too be extended, and designed and tested as such, we are entering dangerous waters and expose ourselves to breakages that the library authors might not even think about. That’s the whole point of this proposal!

Hopefully we can convince the IntelliJ team not to implement such an “auto-fix”, because adding this language import is supposed to be a hack, and you should carefully think about it, instead of using it as a rule.

[sjrd]

@soronpo This is once again the second Alternative. (Because you’d still need an open modifier for the rare intentionally open classes within the library codebase.)

[lihaoyi]

@sjrd

There are a number cases of classes that have 0 abstract method but that are still intended for extension and where the overriding contracts are well-defined. In fact most of my truly open classes (the ones I would add open to) are concrete.

I’m curious what the number of classes that fall into this category is; I have hardly written any in my open source libraries, except for things like Mill build configurations, even though I purposely leave stuff non-final or non-private to override/use “at your own risk”. For the few non-Mill-build related concrete overrides, those are always one or two fields with default values you can set, rather than an entire class of members meant to be over-ridden ad-hoc.

Perhaps open should apply to individual members, instead of classes?

That would be an interesting way of indicating “these are the things to override”, instead of doing the reverse and marking all the things you can’t override final and having the user scan for non-final things

@sjrd

We don’t understand that when we extend a class that was not meant too be extended, and designed and tested as such, we are entering dangerous waters and expose ourselves to breakages that the library authors might not even think about.

Is this really true though? Among most of the people I’ve worked with, both commercially and open-source, they already know that extending some random library class and overriding arbitrary methods is a pretty sketchy and dangerous way of doing things. If these folks can get by without extending library classes, they already would, but sometimes they can’t.

Even beyond the Scala community, “composition instead of inheritance” is a mantra, and “fragile base class” is an antipattern, so I find it hard to believe that there’s a whole lot of people in the Scala community wildly extending/overriding concrete class members with reckless abandon.

@sjrd

Hopefully we can convince the IntelliJ team not to implement such an “auto-fix”, because adding this language import is supposed to be a hack, and you should carefully think about it, instead of using it as a rule.

That’s a reasonable thing to hope for, but empirically the track record of such efforts isn’t great

And I don’t blame the intellij folks at all. Among everyone people i’ve ever worked with, I haven’t met anyone who writes a macro unless they really need to, used a higher-kinded type unless they really need to, or extends/override class members unless they really need to.

For all these folks, language imports are just a weird dance to do that provides no value at all. IntelliJ providing auto-fixes isn’t the problem, but a symptom of the low value these language imports provide at guiding user behavior. A sort of Desire Path, if you will, and such paths should be really properly paved rather than fenced off.

[lrytz]

I also think this is better suited as an opt-in lint warning for authors, with @open as an annotation instead of a keyword.

Scala (the ScalableLanguage) also wants to be a lightweight language that allows you to just “code away” with minimal barriers. This feature immediatly pops up when prototyping something, or when getting started as a beginner, and draws your attention.