RFR: JDK-8225056 VM support for sealed classes

[Mandy Chung]

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.

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