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

[Kevin Rushforth]

On Fri, 31 Mar 2023 13:24:27 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> 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.
>
> I'll take a closer look early next week.

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

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1160981604