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

[Kevin Rushforth]

On Thu, 30 Mar 2023 23:59:50 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

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

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