8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D #1556

mstr2 · 2024-09-05T12:01:23Z

None of these classes can be extended by user code, and any attempt to do so will fail at runtime with an exception. For this reason, we can seal the class hierarchy and remove the run-time checks to turn this into a compile-time error instead.

In some cases, Node and Shape are extended by JavaFX classes in other modules, preventing those derived classes from being permitted subclasses. A non-exported AbstractNode and AbstractShape class is provided just for these scenarios. Note that introducing a new superclass is a source- and binary-compatible change (see JLS ch. 13).

I'm not sure if this change requires a CSR, as it doesn't change the specification in any meaningful way. There can be no valid JavaFX program that is affected by this change.

/reviewers 2

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8341659 to be approved
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D (Enhancement - P4)
JDK-8341659: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D (CSR)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to 4d3ba394)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1556/head:pull/1556
$ git checkout pull/1556

Update a local copy of the PR:
$ git checkout pull/1556
$ git pull https://git.openjdk.org/jfx.git pull/1556/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1556

View PR using the GUI difftool:
$ git pr show -t 1556

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1556.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-05T12:01:49Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-05T12:02:42Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-09-05T12:03:05Z

@mstr2
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2024-09-05T12:06:29Z

Webrevs

andy-goryachev-oracle · 2024-09-05T17:24:50Z

What is the benefit of this change?

Also, this change is breaking. One can currently create a Node:

public class CanCreateNode extends Application {
    @Override
    public void init() {
        Node n1 = new Node() {
        };
        class Yo extends Node {
        }
        Yo n2 = new Yo();
    }

mstr2 · 2024-09-05T17:45:52Z

What is the benefit of this change?

It enforces a system invariant at compile-time instead of run-time.

Also, this change is breaking. One can currently create a Node:

public class CanCreateNode extends Application {
    @Override
    public void init() {
        Node n1 = new Node() {
        };
        class Yo extends Node {
        }
        Yo n2 = new Yo();
    }

You can't do anything meaningful with this node. As soon as you add it to a scene graph, you'll get a runtime exception. The fact that you can't extend Node is a system invariant, finding pathological situations in which you won't get an exception is just a loophole.

kevinrushforth · 2024-09-06T15:13:01Z

If this PR goes forward, it will need a CSR, since it does impact source code compatibility. As you point out, you can't usefully extend Node today, since you will get a runtime exception as soon as you try to create an instance of a subclassed Node, so what this does is move a runtime error to compile time. That's generally a good thing, but..is it worth doing? We can continue to discuss.

/csr

openjdk · 2024-09-06T15:13:35Z

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8339603 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mstr2 · 2024-09-06T15:36:10Z

If this PR goes forward, it will need a CSR, since it does impact source code compatibility. As you point out, you can't usefully extend Node today, since you will get a runtime exception as soon as you try to create an instance of a subclassed Node, so what this does is move a runtime error to compile time. That's generally a good thing, but..is it worth doing? We can continue to discuss.

/csr

In addition to moving a runtime error to compile time, it also documents intent for future JavaFX maintainers. Given that the change is not invasive and has pretty much no downsides, I think it is worth it.

andy-goryachev-oracle · 2024-09-06T15:41:45Z

The fact that you can't extend Node is a system invariant

But I can, and the code provided above runs with no exception - in either init() or start(), making this change a breaking one.

Whether such code is meaningful is another story.

I would vote against this change.

mstr2 · 2024-09-06T15:46:37Z

The fact that you can't extend Node is a system invariant

But I can, and the code provided above runs with no exception - in either init() or start(), making this change a breaking one.

Whether such code is meaningful is another story.

I would vote against this change.

But you just proved that you can violate a system invariant. The existing code tries to detect some violations and alerts you that extending Node is not supported by JavaFX.

andy-goryachev-oracle · 2024-09-06T15:52:17Z

can you point me to where it says one can't extend Node or that it is verboten?

nlisker · 2024-09-06T15:53:22Z

I thought about doing this some years ago for Shape3D, Material and LightBase when I was adding light types. Shape3D states:

An application should not extend the Shape3D class directly. Doing so may lead to an UnsupportedOperationException being thrown.

However, I didn't see enough value in this. There is some small benefit for LightBase because it allows an exhaustive switch on its subtypes in NGShape3D (IIRC). Also, for Material and LightBase, even before compile time - at code writing time - the user will find it impossible to subtype these. Shape3D can be made to allow inheritance as its subtypes don't require native-level handling. I assume that this is why it has a comment about subtyping, and there is more benefit in sealing it.

The downside is minor too: custom shaders will allow user implementations, but that's not coming any time soon.

Since removing sealed/final from a class can be done in the future without much hassle, I'm not against sealing the above classes (and removing the comments about not extending them). It just doesn't overcome the initial inertia for me. I will review the changes for these classes if this goes forward.

As for Node and Shape, I didn't look at subclassing them and never thought about it, so I have no opinion there.

mstr2 · 2024-09-06T15:53:48Z

can you point me to where it says one can't extend Node or that it is verboten?

jfx/modules/javafx.graphics/src/main/java/com/sun/javafx/scene/NodeHelper.java

Line 81 in 4647367

throw new UnsupportedOperationException(

nlisker · 2024-09-06T15:59:09Z

The documentation for Node has the line:

An application should not extend the Node class directly. Doing so may lead to an UnsupportedOperationException being thrown.

Just above StringID.

andy-goryachev-oracle · 2024-09-06T16:00:08Z

a comment in some internal helper class is not, in my opinion, a normative reference.

I am not against this change in principle - it might be a welcome enhancement, if it were solving a real world problem. I am against this change because it has a risk of existing application failing (compatibility risk).

App developers are a creative bunch, they often do things that the platform developers did not expect.

andy-goryachev-oracle · 2024-09-06T16:01:55Z

"should" and "may" are not the same as "must" and "will".

but anyway, my objection is due to possible compatibility risk.

edit: +1 for rtfm :-)

nlisker · 2024-09-06T16:07:39Z

I forgot to mention Camera in my reply. Same as the 3D classes: the user can't do it if they try, so it will fail before compilation. I don't mind sealing it and will review changes for this as well. I also wonder if there are even other camera types in the literature and if the class can be built for subtyping at all.

andy-goryachev-oracle · 2024-09-06T16:16:02Z

I wonder if it is possible to have some kind of interim solution that will warn the app developers that they are doing something they are not supposed to?

Printing a warning won't work: it will be ignored.
Shutting down the app (and allowing it to run with a system property) is probably even worse

What do you think?

mstr2 · 2024-09-06T16:23:22Z

"should" and "may" are not the same as "must" and "will".

I think this is just suboptimal language, and not a material distinction. You will get a runtime error if you use it in any meaningful way. Anyway, if a developer really wants to create an instance of Node, they can use the non-exported AbstractNode at their own peril.

kevinrushforth · 2024-09-06T16:27:12Z

"should" and "may" are not the same as "must" and "will".

I think this is just suboptimal language, and not a material distinction. You will get a runtime error if you use it in any meaningful way. Anyway, if a developer really wants to create an instance of Node, they can use the non-exported AbstractNode at their own peril.

Indeed it is a case of suboptimal language. The intention was to make it clear that this is not supported. Period. And it isn't in any meaningful way. If there are corner cases where we miss throwing an exception, that is a bug.

There might be other reasons to not make this change -- I haven't formed an opinion yet -- but concern over compatibility isn't one of them.

andy-goryachev-oracle · 2024-09-06T16:56:43Z

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

-public abstract class Node implements EventTarget, Styleable {
+public abstract sealed class Node
+        implements EventTarget, Styleable
+        permits AbstractNode, Camera, LightBase, Parent, SubScene, Canvas, ImageView, Shape, Shape3D {


We may need to use stronger wording in javadoc in regards to extending Nodes. should -> must, etc. (or, rather, 'cannot' and possibly explain why?)

I've removed the language that you can't extend those classes, as it is now redundant because it is enforced by the compiler. We shouldn't document obvious facts without providing any added value (for example, adding context and reasoning why it's the way it is).

bridgekeeper · 2024-10-06T22:41:18Z

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

kevinrushforth · 2024-10-07T14:58:03Z

@mstr2 I think there is value in proceeding with this. Can you merge the latest master into your branch? (That will also keep the Skara bot from auto-closing it).

andy-goryachev-oracle · 2024-10-07T21:26:31Z

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/CameraHelper.java

@@ -57,7 +57,7 @@ public static void initHelper(Camera camera) {

    @Override
    protected NGNode createPeerImpl(Node node) {
-        throw new UnsupportedOperationException("Applications should not extend the Camera class directly.");
+        throw new AssertionError();


is this the right error type?
why change UnsupportedOperationException?

When Camera is sealed, this method cannot be called by user code. UnsupportedOperationException would be okay if it could reasonably be called, but this is now an invariant that would signal a bug in JavaFX.

maybe an explanatory comment should be added?

We typically use InternalError for the case where there should be no possible reason for a particular case, but also use AssertionError in some cases. Either is OK with me. I agree that a comment to the affect of "Should not ever get here" wouldn't be a bad idea.

I've added a comment to CameraHelper and LightBaseHelper.

nlisker · 2024-10-07T22:52:44Z

If this goes forward, please seal Material as well.

mstr2 · 2024-10-08T04:25:53Z

If this goes forward, please seal Material as well.

If we seal Material, we should probably also seal Paint. What do you think?

nlisker · 2024-10-08T04:36:16Z

If this goes forward, please seal Material as well.

If we seal Material, we should probably also seal Paint. What do you think?

I don't see why they are related, but if Paint can't be subclassed then I don't see a reason not to seal it. I can envision possibilities to create new paints, like other types of gradients. There's nothing inherently stopping that except for access to methods like platform paint.

mstr2 · 2024-10-08T04:48:20Z

I don't see why they are related, but if Paint can't be subclassed then I don't see a reason not to seal it. I can envision possibilities to create new paints, like other types of gradients. There's nothing inherently stopping that except for access to methods like platform paint.

They are in the same package, and they are related insofar as an argument for sealing Material would also apply for Paint. I'll do that in a separate PR.

andy-goryachev-oracle

thank you for adding the comment!

bridgekeeper · 2024-11-05T19:47:05Z

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Seal Node, Camera, LightBase, Shape, Shape3D

c24aed5

openjdk bot added the rfr Ready for review label Sep 5, 2024

openjdk bot added the csr Need approved CSR to integrate pull request label Sep 6, 2024

andy-goryachev-oracle reviewed Sep 6, 2024

View reviewed changes

kevinrushforth self-requested a review September 7, 2024 13:22

remove documentation

38b7a22

Merge branch 'master' into feature/sealed-classes

ba6c542

andy-goryachev-oracle reviewed Oct 7, 2024

View reviewed changes

add comment

8332d7e

andy-goryachev-oracle approved these changes Oct 8, 2024

View reviewed changes

Merge branch 'master' into feature/sealed-classes

4d3ba39

mstr2 added 3 commits November 6, 2024 20:10

Merge branch 'master' into feature/sealed-classes

d5cd8cb

Merge branch 'master' into feature/sealed-classes

c7f8b72

Merge branch 'master' into feature/sealed-classes

1921be2

8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D #1556

Are you sure you want to change the base?

8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D #1556

Conversation

mstr2 commented Sep 5, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

mlbridge bot commented Sep 5, 2024 • edited Loading

Webrevs

andy-goryachev-oracle commented Sep 5, 2024

mstr2 commented Sep 5, 2024

kevinrushforth commented Sep 6, 2024

openjdk bot commented Sep 6, 2024

mstr2 commented Sep 6, 2024

andy-goryachev-oracle commented Sep 6, 2024

mstr2 commented Sep 6, 2024 • edited Loading

andy-goryachev-oracle commented Sep 6, 2024

nlisker commented Sep 6, 2024 • edited Loading

mstr2 commented Sep 6, 2024

nlisker commented Sep 6, 2024

andy-goryachev-oracle commented Sep 6, 2024

andy-goryachev-oracle commented Sep 6, 2024 • edited Loading

nlisker commented Sep 6, 2024

andy-goryachev-oracle commented Sep 6, 2024

mstr2 commented Sep 6, 2024

kevinrushforth commented Sep 6, 2024

andy-goryachev-oracle Sep 6, 2024

Choose a reason for hiding this comment

mstr2 Sep 8, 2024

Choose a reason for hiding this comment

bridgekeeper bot commented Oct 6, 2024

kevinrushforth commented Oct 7, 2024

andy-goryachev-oracle Oct 7, 2024

Choose a reason for hiding this comment

mstr2 Oct 8, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle Oct 8, 2024

Choose a reason for hiding this comment

kevinrushforth Oct 8, 2024

Choose a reason for hiding this comment

mstr2 Oct 8, 2024

Choose a reason for hiding this comment

nlisker commented Oct 7, 2024

mstr2 commented Oct 8, 2024

nlisker commented Oct 8, 2024

mstr2 commented Oct 8, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

bridgekeeper bot commented Nov 5, 2024

mstr2 commented Sep 5, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 5, 2024 •

edited

Loading

mstr2 commented Sep 6, 2024 •

edited

Loading

nlisker commented Sep 6, 2024 •

edited

Loading

andy-goryachev-oracle commented Sep 6, 2024 •

edited

Loading