RFR: JDK-8225056 VM support for sealed classes

[Harold Seigel]

Thanks for the comments.

Here's version 3 of the JDK and VM changes for sealed classes.

full webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/

The new webrev contains just the following three changes:

 1. The sealed classes API's in Class.java (permittedSubclasses() and
    isSealed()) were revised and, in particular, API
    permittedSubclasses() no longer uses reflection.
 2. An unneeded 'if' statement was removed from
    JVM_GetPermittedSubclasses() (pointed out by David.)
 3. VM runtime test files SealedUnnamedModuleIntfTest.java and
    Permitted.java were changed to add a test case for a non-public
    permitted subclass and its sealed superclass being in the same
    module and package.

Additionally, two follow on RFE's will be filed.  One to add additional 
VM sealed classes tests and one to improve the implementations of the 
sealed classes API's in Class.java.

Thanks, Harold

On 5/28/2020 8:30 PM, David Holmes wrote:
>
> Hi Harold,
>
> Sorry Mandy's comment raised a couple of issues ...
>
> On 29/05/2020 7:12 am, Mandy Chung wrote:
>> Hi Harold,
>>
>> On 5/27/20 1:35 PM, Harold Seigel wrote:
>>>
>>> Incremental webrev: 
>>> http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
>>>
>>> full webrev: 
>>> http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
>>>
>> Class.java
>>
>> 4406 ReflectionData<T> rd = reflectionData();
>> 4407 ClassDesc[] tmp = rd.permittedSubclasses;
>> 4408 if (tmp != null) {
>> 4409 return tmp;
>> 4410 }
>> 4411
>> 4412 if (isArray() || isPrimitive()) {
>> 4413 rd.permittedSubclasses = new ClassDesc[0];
>> 4414 return rd.permittedSubclasses;
>> 4415 }
>>
>> This causes an array class or primitive type to create a 
>> ReflectionData.   It should first check if this is non-sealed class 
>> and returns a constant empty array.
>
> It can't check if this is a non-sealed class as the isSealed() check 
> calls the above code! But for arrays and primitives which can't be 
> sealed we should just do:
>
> 4412 if (isArray() || isPrimitive()) {
> 4413 return new ClassDesc[0];
> 4414 }
>
> But this then made me realize that we need to be creating defensive 
> copies of the returned arrays, as happens with other APIs that use 
> ReflectionData.
>
> Backing up a bit I complained that:
>
> public boolean isSealed() {
> return permittedSubclasses().length != 0;
> }
>
> is a very inefficient way to answer the question as to whether a class 
> is sealed, so I suggested that the result of permittedSubclasses() be 
> cached. Caching is not without its own issues as we are discovering, 
> and when you add in defensive copies this seems to be trading one 
> inefficiency for another. For nestmates we don't cache 
> getNestMembers() because we don;t think it will be called often - it 
> is there to complete the introspection API of Class rather than being 
> anticipated as used in a regular programmatic sense. I expect the same 
> is true for permittedSubclasses(). Do we expect isSealed() to be used 
> often or is it too just there for completeness? If just for 
> completeness then perhaps a VM query would be a better compromise on 
> the efficiency front? Otherwise I can accept the current 
> implementation of isSealed(), and a non-caching permittedClasses() for 
> this initial implementation of sealed classes. If efficiency turns out 
> to be a problem for isSealed() then we can revisit it then.
>
> Thanks,
> David
>
>
>> In fact, ReflectionData caches the derived names and reflected 
>> members for performance and also they may be invalidated when the 
>> class is redefined.   It might be okay to add 
>> ReflectionData::permittedSubclasses while `PermittedSubclasses` 
>> attribute can't be redefined and getting this attribute is not 
>> performance sensitive.   For example, the result of `getNestMembers` 
>> is not cached in ReflectionData.  It may be better not to add it in 
>> ReflectionData for modifiable and performance-sensitive data.
>>
>>
>> 4421 tmp = new ClassDesc[subclassNames.length];
>> 4422 int i = 0;
>> 4423 for (String subclassName : subclassNames) {
>> 4424 try {
>> 4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
>> 4426 } catch (IllegalArgumentException iae) {
>> 4427 throw new InternalError("Invalid type in permitted subclasses 
>> information: " + subclassName, iae);
>> 4428 }
>> 4429 }
>> Nit: rename tmp to some other name e.g. descs
>>
>> I read the JVMS but it isn't clear to me that the VM will validate 
>> the names in `PermittedSubclasses`attribute are valid class 
>> descriptors.   I see ConstantPool::is_klass_or_reference check but 
>> can't find where it validates the name is a valid class descriptor - 
>> can you point me there?   (otherwise, maybe define it to be unspecified?)
>>
>>
>> W.r.t. the APIs. I don't want to delay this review.  I see that you 
>> renamed the method to new API style as I brought up.  OTOH,  I expect 
>> more discussion is needed.  Below is a recent comment from John on 
>> this topic [1]
>>
>>> One comment, really for the future, regarding the shape of the Java 
>>> API here: It uses Optional and omits the "get" prefix on accessors. 
>>> This is the New Style, as opposed to the Classic Style using null 
>>> (for "empty" results) and a "get" prefix ("getComponentType") to get 
>>> a related type. We may choose to to use the New Style for new 
>>> reflection API points, and if so let's not choose N different New 
>>> Styles, but one New Style. Personally I like removing "get"; I 
>>> accept Optional instead of null; and I also suggest that arrays (if 
>>> any) be replaced by (immutable) Lists in the New Style
>>
>> There are a few existing Class APIs that use the new API style:
>> Class<?> arrayClass();
>> Optional<ClassDesc> describeConstable();
>> String descriptorString();
>>
>> This will set up a precedence of the new API style in this class.  
>> Should this new permittedSubclasses method return a List instead of 
>> an array?  It's okay with me if you prefer to revert back to the old 
>> API style and follow this up after integration.
>>
>> 4442 * Returns true if this {@linkplain Class} is sealed.
>> 4443 *
>> 4444 * @return returns true if this class is sealed
>>
>> NIt: {@code true} instead of true -  consistent with the style this 
>> class uses (in most methods)
>>
>> test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java
>>
>> Nit: s/sealed_classes/sealedClasses/
>> - the test directory/file naming convention use camel case or java 
>> variable name convention.
>>
>> Thanks
>> [1] https://github.com/openjdk/valhalla/pull/53#issuecomment-633116043