-
Notifications
You must be signed in to change notification settings - Fork 157
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
=str Make SubFlow and SubSource a final class. #619
base: main
Are you sure you want to change the base?
Conversation
What's the reasoning behind them needing to be |
Forbidden user extension and final class sometimes faster too. |
I'm -1 on this
|
MiMA check can be fixed. Extends this class will not work as no extension point, they are not marked because of a out of sight, not because of it was right. +1 for this;
|
+1 If we don't know if it can be inherited, we should close it. Although it is possible to lose flexibility, but this is potential flexibility, but also we avoid potential dangers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to block this PR for now because making public non final classes final is technically a breaking change since if any user happened to extend SubFlow
/SubSource
their code will no longer work if they were to bring in a version of Pekko with this change. Its only in Pekko 2.0.x that such a change can be merged, and we are not even at that stage yet. You can't just retroactively break code for users because you think that they shouldn't extend these classes, breaking users is breaking users and in doing so we are ignoring semver.
Also the merits of this PR is somewhat dubious wrt performance. The JVM performs CHA (class hierarchy analysis) at runtime which means that all classes are effectively final if the JVM detects that there are no subclasses of that class when the program is being run. In other words if this is a hotspot, the JVM will inline this to static methods at runtime if no one extends SubFlow
/SubSource
.
To me, legitimate reasons to use final
is if by design contract you don't want users to extend the class (because they can break the design in some fundamental way). Scala's case class
is a good example of this, i.e. they should always be final
because otherwise people can override methods such as equals
which would create lots of nasty surprises. The Scala 2 inliner is another reason why you may want to use final
(depending on the context, the Scala 2 inliner may only work with final
).
Now it could be that legitimately speaking a user should never extend SubFlow
/SubSource
however if this is the case as said before it would have needed to be done before Pekko's first 1.0.0
release (too late for that now) or in 2.0.x.
Unlike Akka we follow semver which means we can only change this in 2.0.x. Also I doubt this has any actual impact as JVM will make it effectively final as long as no one subclasses it. |
Ok, let's schedule it to 2.0.x, at least I think no one will be affected. |
Motivation:
Those two class should be final.
Update:
I still think those classes should be marked final and was marked as final in Akka since 2.7.0 and no one complain. Who will extends those two classes outside of pekko ?