SIP public review: Open classes

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

1 Like

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

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

I very much like the feature by the way!

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

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)?

7 Likes

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.

4 Likes

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.

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.

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 :slight_smile:

1 Like

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.

8 Likes

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?

3 Likes

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!

1 Like

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.

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.

1 Like

@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.)

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

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.

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

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.

8 Likes

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.

5 Likes

I’m warming up to the idea of an @open annotation on methods intended for extension, or alternatively on whole classes if all their non-final methods are. The rest of the proposal would still hold as is.

2 Likes

This has already been discussed at length over two discussions in this forum; there were no clear conclusions other than the realization that the topic is heavily controversial, and so do this proposal and its motivation. The language should not include such a core feature – tied with its own keyword – if its so controversial. On the other hand, an @open annotation with an opt-in compiler warning? Sure, why not.

Furthermore, I find major flaws in the logic and wording of the motivation:

When writing a class, there are three possible expectations of extensibility:

  1. The class is intended to allow extensions.
  2. Extensions of the class are forbidden.
  3. There is no firm decision either way.

The three cases are clearly distinguished by using open for (1), final for (2) and no modifier for (3).

(1) is clearly distinguished from (3) by using either trait or abstract.

It is good practice to avoid ad-hoc extensions in a code base, since they tend to lead to fragile systems that are hard to evolve.

This is clearly an odd statement given that implicit is such a major feature in the language, used heavily to provide just that – ad-hoc extensions via type classes and extension methods.

I’m not really sold on open, so I don’t really have any skin in this, but this doesn’t really make sense to me as a counter argument.

Extension methods and typeclasses can only combine the various methods of a class’s external API and can’t be used to override methods, so I don’t think it’s valid to compare them with extending a class or overriding it’s methods.

2 Likes

I’m not sure whether it’s a counter-argument, a comment about the particular wording, or both.

“ad-hoc extensions” is not a term I’m familiar with, but I am familiar with ad-hoc polymorphism, which type classes are a form of.

While I agree that this mechanism can be useful in some scenarios, I’m concerned that it might:

  • subvert expectations
  • add unnecessary inconvenience

I believe that the following idiom is very common in many programming languages:

class A
class B extends A

and that the common expectation is that this has the same meaning independent of whether A and B are defined in the same file.

Introducing the open mechanism would break this expectation. In other words, a feature that might be useful in certain advanced usages of class hierarchies, would introduce a non-optional restriction that might confuse user coming from other languages, and inconvience users who have no need for the feature. This is in contrast to other modifiers such as protected and final which adds a restriction when present, not when absent.

Additionally, one should remember that programming is not an homogenous art - there is a wide variety of programming domains, and different programming techniques will be appropriate for different domains. As such I object to the following generalization (from the “Motivation” section):

The class is intended to allow extensions. This means one should expect a carefully worked out and documented extension contract for the class.

In a low-level library consumed by hundreds of users, yes, perhaps. But in simple scenarios like e.g. modules for code re-use and organizational purposes

class CommonInvoiceOps {
  // methods
}

object AbcInvoiceMgr extends CommonInvoiceOps {
  // more methods
}

object XyzInvoiceMgr extends CommonInvoiceOps {
  // more methods
}

I would certainly not expect a “carefully worked out and documented extension contract for the class”.

As each of the classes above grows larger, it might make sense, simply for maintability purposes, to move them to seperate files. Having to add open on the base class just because it is in a different file would imply a significance that is simply not there.

So, in my opinion:

At the least, the mechanism should work on the package level and not the file level.

(On a related note, I would really like sealed to be package-level as well. Actually, I think the concept of “file” should not have any semantic meaning in a program at all.)