JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClassState

[hjohn]

The class PseudoClassState is private API, but was exposed erroneously in the CSS API. Instead, Set<PseudoClass> should have been used. This PR corrects this.

---

Progress

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

Issues

• JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClassState
• JDK-8304994: Public API in javafx.css.Match should not return private API class PseudoClassState (CSR)

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1070

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jhendrikx! 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.

[mlbridge]

Webrevs

• 08: Full - Incremental (da8ac037)
• 07: Full - Incremental (80ff9992)
• 06: Full - Incremental (270d0cbd)
• 05: Full - Incremental (bcc9a941)
• 04: Full - Incremental (d2651894)
• 03: Full - Incremental (144e04cf)
• 02: Full - Incremental (73fa672e)
• 01: Full - Incremental (9bbe9259)
• 00: Full (dffc6f47)

[kevinrushforth]

This was clearly a mistake back when the public API was first created in JDK 9.

/reviewers 2
/csr

[openjdk]

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

[openjdk]

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

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

[hjohn]

@kevinrushforth I've created the CSR for this. Can I move it to proposed?

Also, I've been looking into https://bugs.openjdk.org/browse/JDK-8199216 which sees excessive use of PseudoClassState instances (and I see this even under more "normal" circumstances as well). I've got a fix for this issue which involves making use of an immutable variant of PseudoClassState (as almost 99% of all the instances are never mutated, but must be copied just in case they are).

This change would need to go in first though to make this possible.

[kevinrushforth]

@kevinrushforth I've created the CSR for this. Can I move it to proposed?

I took a quick look, and it is fine to move to Proposed.

[kevinrushforth]

I did leave one question that you might want to address before proposing the CSR.

[kevinrushforth]

Should this be an ObservableSet? Changing the type to the Set superclass will mean that applications would need to do an instanceof check to know whether it was observable or not?

[hjohn]

I think that Match is supposed to be immutable, given the non-public constructor. Match itself will never change the set (and nothing else will) so making it observable seems unnecessary.

However, this was not correctly spec'd, and you can change a Match now, and since it doesn't make a copy, you'd be changing the pseudo classes of whatever selector created it. Note that SimpleSelector does make a copy, and goes to great pains to not expose anything mutable, but exposes it accidentally via Match.

In other words, simpleSelector.createMatch().getPseudoClasses().clear() would break the Selectors encapsulation.

I think it's best to close that loophole. If you agree, I can document this method that it returns an immutable set, which is also what I assumed would be the case in my other PR where I made many of these immutable.

[hjohn]

The CSS API baffles me a bit. It doesn't seem consistent.

Just now I took a look at the class SimpleSelector and CompoundSelector. These are public, yet cannot be constructed by users. They're also not returned anywhere (the closest is Selector#createSelector which returns the interface).

Essentially this means that SimpleSelector and CompoundSelector should probably be package private. Yet, I guess they were made public because SelectorPartitioning is doing instanceof checks and is casting to these classes. But anybody can do that now, and that means that for example SimpleSelector#getStyleClassSet is exposed, which returns a mutable set...

Reading between the lines though it seems that SimpleSelector and CompoundSelector were intended to be fully immutable (which makes sense as they represent a style sheet). Any changes would not be picked up as nothing is observing these properties.

I think these loopholes should be closed.

There are two options IMHO:

1. Move SimpleSelector and CompoundSelector to the com hierarchy. They can't be publically constructed, and are never returned. The only way to reach them is by casting.
2. If it's too late for that, then close all loopholes and ensure that these two classes are fully immutable. From what I can see now, only getStyleClassSet and the mentioned method in Match need closing. CompoundSelector is already immutable.

[kevinrushforth]

I'll take a closer look early next week.

[kevinrushforth]

I think that Match is supposed to be immutable, given the non-public constructor. Match itself will never change the set (and nothing else will) so making it observable seems unnecessary.

Agreed.

In other words, simpleSelector.createMatch().getPseudoClasses().clear() would break the Selectors encapsulation.

I think it's best to close that loophole. If you agree, I can document this method that it returns an immutable set, which is also what I assumed would be the case in my other PR where I made many of these immutable.

Yes, this seems like the best solution to me.

Essentially this means that SimpleSelector and CompoundSelector should probably be package private.

I agree that these two classes should not have been made public when the javafx.css package was created as public API back in JDK 9.

The usual process for removing API is to deprecate it for removal in one release and then remove it in a future release, but in this case, since those classes cannot be constructed, and are never returned by any public API, any use of them would be relying on an implementation detail (via an instanceof and a cast, to no good purpose).

Since there is no useful way an application could be using these classes, I recommend option 1, as long as those two classes can be cleanly moved to com.sun.javafx.css. Otherwise, we could go with option 2 along with deprecating those two classes for removal.

The Specification section of the CSR would simply propose to remove those two classes. You could describe what is happening (moving them to a non-exported package) in the Solution section.

[hjohn]

@kevinrushforth It looks like it will have to be option 2; the reason is that both Selector and Match are abstract classes (with a package private constructor). The CompoundSelector and SimpleSelector extend this abstract class (as there is some small overlap). If Selector and Match had been interfaces, it would have allowed this.

I couldn't find anything about binary compatibility when changing an abstract class with a package private constructor to an interface, so I tested it and I got an IncompatibleClassChangeError when calling Selector Selector.createSelector(...) method. Apparently, the compiled method call does encode the difference between an interface and a class:

invokestatic  #7                  // Method Selector.createSelector:(I)LSelector;

vs:

invokestatic  #7                  // InterfaceMethod Selector.createSelector:(I)LSelector;

At the most, I could make CompoundSelector and SimpleSelector package private, or nested classes in Selector; they would at least be hidden then. Can't do that either, they're referred in private API.

[kevinrushforth]

OK. In that case, option 2 it is. And since there doesn't seem to be a good way to remove these classes from the public API, it makes no sense to deprecate them for removal.

We could deprecate them with a note that they are used by the CSS implementation, and not intended to be used by applications, but since applications aren't usefully using them today, we could defer that to a later time (or not do it at all).

[hjohn]

I think this is the best that can be done for now. There are still some mutable aspects to Selector (you can change the Rule and the ordinal of one). I'm really not quite sure why it is done that way, it seems there was may have been some vision that these classes could be used to "edit" a stylesheet -- something that's probably best done with a different set of classes to avoid burdening regular read-only CSS work with editable variants, requiring defensive copying everywhere.

Ideally, Selector and Match would have been interfaces, and the private code should work with the CSS classes only through those. Changing them to interfaces now seems impossible, it's not binary compatible, and I don't see a deprecation route to get there.

However, it is still possible to improve the situation a bit (in another PR perhaps), as the private code only requires a few additional methods to be made public API on Selector to remove its direct dependencies on SimpleSelector and CompoundSelector:

• getName() which returns the java class name to which the selector is applied
• getId() returns the CSS id (if any) of a Selector
• getStyleClassSet() returns the (immutable) style classes of a Selector

These three methods are implemented by SimpleSelector, but not by CompoundSelector. Instead, its last selector is extracted (which is always a SimpleSelector) and the methods are called on those. These methods could be made part of Selector, then CompoundSelector implements them by directing them to its last SimpleSelector, and the private code would not need to care about the selector type anymore. These selector classes could then be hidden completely.

[hjohn]

I had to pull in some of the fixes I did in BitSet in #1076

Just wrapping a Set<PseudoClass> in an unmodifiable set is enough to break lots of tests, because BitSet has many methods that will simply do nothing if the "other" Set is not a BitSet.

[hjohn]

@kevinrushforth about the CSR -- as I couldn't move the classes to non-public hierarchy, I think the CSR is currently correct. I also removed a public method in SimpleSelector as it wasn't used (and as discussed before, nobody can reach it without casting).

There were a few javadoc only changes that could be seen as clarification (whether something can be null or whether it is immutable). I could include those in the CSR.

If we want to move forward later to perhaps try and remove SimpleSelector and CompoundSelector (perhaps by adding those few public API methods to Selector), I could already mark these deprecated (or I could do so to indicate they shouldn't be used at all, whether we want to remove them or not). What do you think?

[bridgekeeper]

@hjohn 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!

[hjohn]

@kevinrushforth do you think we can move forward with this?

[andy-goryachev-oracle]

I'll take a look at this next week.

[kevinrushforth]

The changes look good with a few inline comments.

[kevinrushforth]

OK. In that case, option 2 it is. And since there doesn't seem to be a good way to remove these classes from the public API, it makes no sense to deprecate them for removal.

We could deprecate them with a note that they are used by the CSS implementation, and not intended to be used by applications, but since applications aren't usefully using them today, we could defer that to a later time (or not do it at all).

[andy-goryachev-oracle]

two sticky points:

• verify somehow that nulls cannot be passed to *All() methods
• consider adding a component type to BitSet

[andy-goryachev-oracle]

I think we may need to add a 'component type' to BitSet, to make sure than BitSet never intersects with BitSet.

(Didn't we do it already, may be in some other PR?)

[hjohn]

We did, in #1076. However, I would like to not pull in all the BitSet fixes into this PR as this has nothing to do with BitSet and its many problems.

[andy-goryachev-oracle]

Since it's an internal class, and both PseudoClassState and StyleClassSet are final, checking for class == class is probably sufficient.
Do you think we should add a warning to BitSet, it is not a problem since no-one will ever extend it?

[hjohn]

This is also solved in #1076:

 if (obj instanceof BitSet<?> bitSet && getElementType().equals(bitSet.getElementType()))

But since that requires pulling in even more changes (getElementType) I've left it there.

Relying on subclasses being final though seems like a bad idea for an abstract class, unless we seal it and use permits (or, just remove BitSet completely... it's purpose will be minimal if #1076 is accepted).

[andy-goryachev-oracle]

that's my point. I suppose we could use sealed class here, since javafx requires minimum java17?

[hjohn]

Do you think this needs to be changed as part of this PR?

Note that the == solution requires:

• sealing BitSet permitting only PseudoClassState and StyleClassSet
• adding a warning that if you ever unseal it, that you must re-evaluate the equals implementation
• adding a warning to PseudoClassState / StyleClassSet to re-evaluate BitSet#equals if you remove final

The instanceof solution allows arbitary subclasses, like any other Set implementation does (AbstractSet#equals for example use instanceof Set) and requires no further changes.

What I'm missing is the reasoning why you would want to change this. I realize that the other solution can also work, but that's not enough IMHO. For me the two options are practically equivalent, neither being truly better than the other, so then it is a matter of preference, and I slightly favor using instanceof here to avoid potential problems down the road.

[kevinrushforth]

Do you think this needs to be changed as part of this PR?

I'll answer this part of your question: No.

It seems out of scope for this particular bug fix, which is aimed at fixing an API and impl problem with Match, and should limit the changes to other classes to those needed to accomplish this.

It's great to point out existing weaknesses in, say, BitSet, but they should be addressed separately, after evaluating the need.

[andy-goryachev-oracle]

I see that a null check was added.
Are we sure we added it to every possible place?

(it's not trivial to verify that with Eclipse without massive changes)

[hjohn]

The underlying collection BitSet is only used by classes dealing with styles and pseudo class states. I checked the classes involved, and only this one needed changing. The check was added because BitSet erroneously allowed adding null before (which resulted in no change), but that breaks the Set contract.

In any case, adding a null collection is an error, we should never be lenient when it comes to nulls.

[andy-goryachev-oracle]

Consider this example:

    @Test
    public void testEquals() {
        PseudoClassState a = new PseudoClassState();
        StyleClassSet b = new StyleClassSet();
        assertFalse(a.equals(b));
    }

[hjohn]

Consider this example:

    @Test
    public void testEquals() {
        PseudoClassState a = new PseudoClassState();
        StyleClassSet b = new StyleClassSet();
        assertFalse(a.equals(b));
    }

I'm aware that this is broken now, and still is broken after this PR. It's fixed in #1076 and has this test case even.

It's worse than that tough, even BitSets of the same type can fail equals (from #1076):

@Test
void shouldBeEqual() {
    StyleClassSet set1 = new StyleClassSet();
    StyleClassSet set2 = new StyleClassSet();

    set1.add(StyleClassSet.getStyleClass("abc"));
    set2.add(StyleClassSet.getStyleClass("abc"));

    assertEquals(set1, set2);

    for (int i = 0; i < 1000; i++) {
        // grow internal bit set array:
        set1.add(StyleClassSet.getStyleClass("" + i));

        assertNotEquals(set1, set2);
    }

    for (int i = 0; i < 1000; i++) {
        set1.remove(StyleClassSet.getStyleClass("" + i));
    }

    // still equal despite internal array sizes being different size:
    assertEquals(set1, set2);
}

Do you want me to pull in more changes from #1076 unrelated to fixing the public API problem in Match?

[andy-goryachev-oracle]

My concern is that replacing == with .equals() in BitSet.equals() may lead to issues in internal CSS code. It is highly unlikely that application would extend BitSet, or put StyleClassSet in the same hash table due to both being in com.sun; but the internal code might (and I don't want to comb the CSS code looking for possible occurrences).

If we replace line 532 with if(getClass == obj.getClass()) { however, we would not have to.

[kevinrushforth]

It is highly unlikely that application would extend BitSet, or put StyleClassSet in the same hash table due to both being in com.sun;

Not merely unlikely, it is not possible, so this need not be considered.

but the internal code might (and I don't want to comb the CSS code looking for possible occurrences).

If we replace line 532 with if(getClass == obj.getClass()) { however, we would not have to.

That's a good suggestion. Given that we want the minimal changes to BitSet for this particular PR, this seems a reasonable, and safe choice. BitSet is an internal-only abstract class, and the only two concrete subclasses are both final with different type parameters.

[hjohn]

My concern is that replacing == with .equals() in BitSet.equals() may lead to issues in internal CSS code. It is highly unlikely that application would extend BitSet, or put StyleClassSet in the same hash table due to both being in com.sun; but the internal code might (and I don't want to comb the CSS code looking for possible occurrences).

If we replace line 532 with if(getClass == obj.getClass()) { however, we would not have to.

Alright, I see your point. The current version will always return false when comparing StyleClassSet with a PseudoClassState. So does the (full) solution in #1076 -- but this half copied solution doesn't, which is a slight regression (probably harmless, but even so).

I'll fix it.

[hjohn]

I will still fix it so it isn't worse than it was before, but I just found another reason why you shouldn't use getClass() == other.getClass() when dealing with sets:

@Test
void twoEmptyBitSetsShouldBeEqual() {

    /*
     * Per Set contract, the empty set is equal to any other empty set.
     */

    assertEquals(new StyleClassSet(), new PseudoClassState());
    assertEquals(new PseudoClassState(), new StyleClassSet());
    assertEquals(Set.of(), new PseudoClassState());
    assertEquals(new PseudoClassState(), Set.of());
    assertEquals(Set.of(), new StyleClassSet());
    assertEquals(new StyleClassSet(), Set.of());
}

I will leave that for #1076 (where this test is coming from). I'm only adding a test to verify the fix I'm adding here, and was seeing what I can use before writing new ones. The above test passes though because it will fall back to super.equals(obj) in that case.

[openjdk]

@hjohn Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[hjohn]

Sorry for the force push, that wasn't what I intended; wanted to undo changes GitHub did on the remote by merging in master as I also merged it in locally (but I added a commit just before). Unfortunately, the PR already picked up the master merge from GitHub... I'll know that for next time.

[andy-goryachev-oracle]

tested with the MonkeyTester on macOS Ventura 13.3.1(a), including dynamic re-loading of the stylesheets. see no ill effects.

[kevinrushforth]

I took a closer look at the compatibility issues Joe raised in the CSR, and I can confirm that yes, this change will break binary compatibility for any application that calls the existing method. It would also can break source compatibility for the subset of those applications that use the return value as an ObservableSet (which is the lowest public supertype of PseudoClassState), although this latter concern seems very unlikely, and not all that impactful.

So it's the binary compatibility we need to consider. The question is: is the Match::getPseudoClasses method being called in a useful manner by applications today?

If not, then the change seems OK.

If it is, then we might need to consider adding a replacement method and deprecating the existing method for removal. The implementation of the replacement method would be exactly what your PR currently has for the existing method, and the existing method, would create a PseudoClassState object to hold a copy of the objects in the set.

[hjohn]

So it's the binary compatibility we need to consider. The question is: is the Match::getPseudoClasses method being called in a useful manner by applications today?

If not, then the change seems OK.

If it is, then we might need to consider adding a replacement method and deprecating the existing method for removal. The implementation of the replacement method would be exactly what your PR currently has for the existing method, and the existing method, would create a PseudoClassState object to hold a copy of the objects in the set.

I've done some Googling with "getPseudoClasses" import javafx, and also looked at several code search sites, and if it's used, then I can't find any examples at all. I also searched for createMatch, with similar results, no uses outside the JavaFX code base.

To call this method, one would to do something like this at a minimum:

 StyleSheet styleSheet = StyleSheet.loadBinary( <input stream> );

 for (Rule rule : styleSheet.getRules()) {
      for (Selector selector : rule.getSelectors()) {
           Match match = selector.createMatch();

           match.getPseudoClassStates();  // here...
      }
 }

I did find a few references to loadBinary, but nothing for getRules which would be needed next.

There's not a lot of reason to call getPseudoClassStates -- the method is mainly public so internal code outside its the public package can reach these to do the actual matching logic. This is why I initially tried to make Selector's subclasses private, but couldn't.

[kevinrushforth]

I've done some Googling with "getPseudoClasses" import javafx, and also looked at several code search sites, and if it's used, then I can't find any examples at all. I also searched for createMatch, with similar results, no uses outside the JavaFX code base.
...
There's not a lot of reason to call getPseudoClassStates -- the method is mainly public so internal code outside its the public package can reach these to do the actual matching logic.

This is what I would have expected as well. I'll do some additional searching as well, but, as of now, I think we can proceed down the current path and live with the incompatibility.

[kevinrushforth]

I've completed my review, and everything looks good. I found one missing doc change in the CSR (the change in SimpleSelector) that should be added, and then I'll formally review the CSR.

[openjdk]

@hjohn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304959: Public API in javafx.css.Match should not return private API class PseudoClassState

Reviewed-by: kcr, angorya

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

• 4b24c86: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet
• 2f5dcfd: 8306329: Update ICU4C to 73.1

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[kevinrushforth]

@hjohn In case you missed seeing it, the CSR was approved, so Skara marked this PR as ready.

[hjohn]

/integrate

[openjdk]

Going to push as commit 3fa02ee.
Since your change was applied there have been 9 commits pushed to the master branch:

• 2a6e48f: 8308017: [Mac] Update deprecated constants in GlassWindow code
• f8c8a8a: 8308191: [macOS] VoiceOver decorations are shifted on second monitor
• 6334032: 8223373: Remove IntelliJ IDEA specific files from the source code repository
• 5bbb95b: 8308307: Update to gcc 12.2.0 on Linux
• 2833a53: 8308306: Update to Xcode 14.3 on macOS
• afa71d4: 8233955: VM crashes if more than one file are added to ClipboardContent via drag and drop
• 56fb71a: 8091153: Customize the Table Button Menu
• 4b24c86: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet
• 2f5dcfd: 8306329: Update ICU4C to 73.1

Your commit was automatically rebased without conflicts.

[openjdk]

@hjohn Pushed as commit 3fa02ee.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.