Make concrete classes final by default

#53

I pretty much agree with what @Krever has just said.

I agree it would be short-sighted if these accidental situations happened rarely, but I don’t subscribe to that view. These accidental design decisions that allow downstream users to fix/work around behavior they don’t like occur quite often, in my experience. I don’t have data to back this up, but I imagine that some static analysis could yield some interesting answers to the question of how often do libraries extend public classes of third-party software. Remember, too, that users only patch behavior on classes that are user-facing and that usually are instantiated by them and then passed around the rest of the APIs.

We have discussed whether making class hierarchies final by default is worth it or not from different perspectives. The strongest argument against it is that it would break an immense amount of code and it’s too late for it. That alone is a good argument to not consider this change for future versions of Scala, as we care about binary compatibility and not breaking people’s mental model about the language.

However, I haven’t seen many people challenging @joshlemer’s initial claim about why making class hierarchies final is the best default:

And I think that it’s worth not taking the “pretty rare” and “code smells” claims for granted. We don’t actually have data to back up how rare it is, and the usefulness of extending open class hierarchies is not about good API design, but about working around behaviors you don’t like (or that may be too specific for your use case) in user land. If everyone used Bazel or Pants in our community, this wouldn’t be an issue, since we would have a frictionless way of forking and then changing somebody else’s code.

#54

It has happened to me a few times that I wanted to do something with a library and was eventually told by developers that the only practical way of doing what I wanted was using some undocumented method. The lesson is that library developers usually don’t anticipate all the use cases.

Making concrete classes final by default would make sense under one scenario: if there was a separation of type and implementation, by which I mean, if every concrete class (i.e the implementation) extended a type that exposes exactly the same members. I would actually love to have a language that enforces that. If we had separation of type and implementation, you could always create your own implementation based on an existing one via delegation.

#55

To add on to this, it is also accidental that they didn’t make the class final. It is quite possible that they meant to and forgot.

#56

The problem is, non-final classes present very real problems, ranging from incorrectness to security vulnerabilities.

Extending a class which should have been final may allow the extending class to violate class invariants which are not exposed as part of a public API, but are inadvertently broken due to the class being non-final.

Worse still, a security-critical method may take an instance of some class (that does not normally allow exploitation), but an extending class is able to leak data (e.g. a private key) out of the API, because the author(s) forgot to make the class final.

#57

My 2 cents …

Well, no, that’s not the middle-ground.

Instead of extending classes in order to tie yourself to private and brittle APIs in order to fix the library, far better ideas would be to:

  1. contact the authors of the library, like via a GitHub issue
  2. fork the library, fix it, then send a PR
  3. publish and use your own fork in case the above doesn’t work in a reasonable time

Point nr 3 sounds like a lot of work, but with the tooling we have publishing your own stuff in your own private Maven repository is not that complicated. All companies end up with their own Maven repo of stuff.

I personally make all of my classes final by default, I also recommend this to other people, I’ve seen others do the same, so this argument is not valid for an increasingly number of Scala libraries.

Also I agree with @jvican in the sense that library authors do not know how their library will get used and in my own work I had to patch Java libraries, so I know this has been happening and for Java open classes has been a good thing.

However from the library’s point of view it’s best if you limit the exposed API, because if you’re exposing internals that may change in the future, you then can’t change those internals without breaking the API obviously. The less you expose as a library author, the more future proof the API is. So for a library author the safe choice is to make those classes final and only open them when users show a genuine need for it.

I actually wish Scala adopted C#'s convention in this regard … all classes should be final by default and mark the open ones via a keyword like virtual. That’s the sanest approach and even though C# libraries aren’t friendly to brittle patches like in Java, C# developers have been surviving this limitation just fine.

#58

Btw, I’m basically echoing the thoughts of Anders Hejlsberg, the lead architect of C#, read this interview:

https://www.artima.com/intv/nonvirtual.html

#59

I strongly agree with the 3 paths you mentioned, but first 2 are rarely acceptable as the brings a development delay. The third one is fully acceptable but only worth doing in case of big libraries.

Also you mentioned case classes and I can’t recall a single case when I needed to patch those. Having case classes final is not a problem at all, its normal classes that needs patches sooner or later.

I also don’t get the ‘API breakage’ argument. If I decide to rely on internals, I’m not really in position to make any complaints, am I?

#60

As a library author, if I left my class open in one version, it’s not internal, it’s become part of the public API. I am not in a position to break some of my users’ code by releasing a new version where it is final, or even where I’ve broken the undocumented “outgoing contact”, as Anders Hejlsberg calls it (brilliant interview btw!).

#61

I would wish all concrete classes would be final by default in Scala 3.

Making a class “open” should require the new modifier keyword overridable (a nice symmetry with override) on the class or one of its members.

I think such a change could be handled by ScalaFix and would not need any manual interaction to preserve the programm’s semantics.

Concerning “repair / modification from the distance” of library classes / objects (traits?): It would be nice if Scala would get some AOP (aspect-oriented programming) support for that.

Only an opinion from some random guy on the internet…

#62

Is there more of an argument for making final the default than for making private the default?

If the writer of a library should have to be explicit about what parts of the library can be used in what way, doesn’t that extend to method access? Shouldn’t public APIs have to be marked as such explicitly?

#63

Actually there is. See the interview posted by @alexandru. It is much more difficult to maintain and reason about the outgoing contract (which is only a concern for non-final) than the incoming contract (which applies to everything non-private).

#64

To answer all the question at once (out of my perspective): No, no, and no. :smile:

The messages you can send to an object are its public interface, aren’t they?

Methods correspond to messages so it’s quite natural that they are public by default. A non public method on the other hand is something special. It does not correspondent to a message. It’s an implementation detail! So it should be marked explicitly as such.

If classes wouldn’t be overridable by default there is even no strong need for hiding the methods anyway.

#65

I strongly agree with those words. I feel it is far more practical to loosely declare which APIs are intended to be used and which are internals that should be used with care.

To continue this line of thought, I have to say that I also very much dislike when library authors restrict the scope of many of their classes to only be used by their library (private[mylibrary]). It has been too often that I just had to give up a desired feature or a bug fix in a big library (akka, scalikejdbc, etc) just because of restricted scopes. I’d rather have the authors mark those parts as internals (perhaps via annotation which would yield a compiler warning) than restrict them entirely.

#66

I agree with a previous comment that it would be nice to have tooling support to crack open the implementation details of a class in order to apply a patch or test non-public implementation details.

These implementation details might be restricted members, or they might be local functions.

There are endless debates about whether it is wise to test non-public implementation details, where you would sprinkle asserts for invariants, and I don’t mean to reopen that side of the debate. But I would like to add here that local functions are not different from private methods with regard to testing and patching hacks.

#67

It seems like there are two views on this – perhaps it’s largely people who value freedom to modify other people’s libraries vs. library authors who value freedom to modify their own libraries without breaking other people’s code…

(Personally I’m more in the former camp.)

In any case, it seems like a viable solution would have to satisfy both camps, and of course not break existing code.

Maybe we need three levels - (a) final for “if you extended this things would likely break, so just NO,” but also (b) “this is meant to be extended at will” (e.g., JComponent) and © “if you extend this, you’re taking your life into your own hands.” It seems reasonable to make the first two explicit, since they are the most opinionated. I think © pretty much describes how things work very often already. So we could say © is the default. On the other hand, adding a new keyword for (b) (like open or virtual) seems too big of a change, even if it weren’t a breaking change.

Two approaches:

  1. Add an annotation @open (or whatever bikeshed color) to document a class as an open hierarchy. A class with that annotation is in category ©, otherwise (without final) it’s in (b).

  2. In Dotty, traits can have parameters, and support for trait parameters may be coming even in Scala 2.x, which begs the question why have classes if traits can do everything classes do and aren’t restricted to single inheritance. So we say traits are (of course) in (b), and all (non-final) classes would be considered ©, and discouraged to extend.

In any case, we don’t want to be too harsh on people in the © camp. So extending something in © shouldn’t be an error, or even flood the build log with warnings by default. Instead, either collapse the warnings by default like is currently done with deprecation, feature, and unchecked warnings, or else make the warnings opt -in with -Xlint. If someone wants to kickstart it they can do it as a library / compiler plugin, or add support to wartremover or the like, and promote it wherever possible.

#68

As you probably know, in Java, the default visibility is package privage which I personnaly think is the correct default if you think of your system as being assembled from components. public and private require explicit annotations which supposedly make them that slight bit harder to use therefore encouraging good design. Sadly the thoughtfulness put in choosing this default was totally defeated by IDEs making everything public by default in their templates and upon usage.

the interesting point about package private is that it is not impossible for someone with sufficient motivation to overcome the encapsulation from outside the library code base. One can create a class in the library’s package to access whatever one needs.

Could the same thing be applied to final ?

  • final : no one can touch this
  • package final: no one can extend/override this from outside the package (would be the default)
  • override: you are encouraged to extend/override this.

(override may not be the best way to express this but it has the strong advantage of not requiring the creation of a new keyword. adding keywords to a language is always quite painful for end users).

I don’t know the JVM/compiler well enough to know if this is achievable or not

#69

This proposal (@nafg’s) is actually very sensible. I would endorse it.

I would just keep @open in dotty. There are still use cases for open concrete classes even when traits can get parameters. The most important ones are Java interop and their properties wrt binary compatibility.

#71

Excuse my ignorance but what would be gained by this proposal? I don’t get it.

I read this as: Everything stays the same except there would be a new annotation that suppresses some warnings.

Did I misunderstood something? What problem is solved this way? Could you explain please?

The original proposal solves a real problem, and would make a best practice the default (also helping newcomers to the language doing “the right thing” without thinking about it). The coast of that change is also low. It would be a fully automatic rewrite. In some well written code-base it would also reduce the visual line noise by removing the “final” keyword that is used in almost every declaration. Such change would also make “open” (or “overridable” which I prefer due to symmetry but that’s a bikeshed color anyway) visually stand out, marking clearly the places some external implementation can hook in.

Removing classes from the language does seam problematic also. How would an code upgrade to scala 3 look like? Can really every class be simply rewritten to a trait? I’m not sure about that.

And like I said: Extending random alien classes not owned by yourself isn’t the clean approach to “bugfixing” / “experimenting with” library code in my opinion. AOP would be a much cleaner pattern to achieve the same goal.

#72

No matter the exact keyword used for this, if this feature does get implemented with an open keyword or similar, it doesn’t have to be backwards incompatible wrt the keywordization, right? Couldn’t open (or overridable) be somewhat like Java 9’s new restricted keywords, i.e. normal identifier unless it appears in a context where an identifier couldn’t have been before in the first place (as we have @annotations unlike Ceylon).

Or is the implementation of that/technical debt too high for consideration? (As I have not much insight into the compiler’s design)

#73

No, it adds warnings. The annoyance is that the “best practice” would require writing the annotation by default. I’d actually prefer having the annotation for (b).

We’ve chatted about it on the Dotty side, it looks plausible but we haven’t tried out how invasive it’d be, and it’s not been a priority yet.

AOP isn’t coming to Scala. AOP research has been mostly abandoned, because AOP is too powerful and fragile, especially if the advised code evolves without accounting for the aspects. Even its flagship conference (http://modularity.info/) died out / was renamed into something else.
We understand the tradeoffs of the OOP approach much better, even tho it’s not perfect, and we have some sensible guidelines (see Anders Hejlsberg’s interview); AOP has all the same problems (and more), but magnified.
I admit “final by default” would be cleaner, but even with ScalaFix the transition cost is too big, as ScalaFix doesn’t rewrite existing knowledge in either heads or docs.

I think that’s only sort-of supported, code using that will sometimes break, especially in libraries and breaks at least with OSGi; I don’t use it myself, but some libraries need to.