RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v3]

[Mandy Chung]

On Wed, 2 Dec 2020 14:40:15 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>> 
>> From the original PR:
>> 
>>> Please review the code for the second iteration of sealed classes. In this iteration we are:
>>> 
>>>     * Enhancing narrowing reference conversion to allow for stricter checking of cast conversions with respect to sealed type hierarchies
>>> 
>>>     * Also local classes are not considered when determining implicitly declared permitted direct subclasses of a sealed class or sealed interface
>>> 
>>>     * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, still in the same method, the return type has been changed to Class<?>[] instead of the previous ClassDesc[]
>>> 
>>>     * adding code to make sure that annotations can't be sealed
>>> 
>>>     * improving some tests
>>> 
>>> 
>>> TIA
>>> 
>>> Related specs:
>>> [Sealed Classes JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>> 
>> This PR strives to reflect the review comments from 1227:
>>  * adjustments to javadoc of j.l.Class methods
>>  * package access checks in Class.getPermittedSubclasses()
>>  * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improving Class.getPermittedSubclasses to filter out permitted classes that are not a subtype of the current class, and other adjustments per the review feedback.

I approve this version of `Class::getPermittedSubclasses` implementation for this PR.    We need to follow up the specification of `Class::getPermittedSubclasses` w.r.t. what it should return e.g. the classes whatever in `PermittedSubclasses` attribute vs the classes that are permitted subtypes at runtime and return null if this class is not sealed. 

I reviewed hotspot and java.base changes (not langtools) with a couple minor comments.

test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line 51:

> 49: public class TestSecurityManagerChecks {
> 50: 
> 51:     private static final ClassLoader OBJECT_CL = Object.class.getClassLoader();

This is `null` - the bootstrap class loader.   An alternative to the static variable we can simply use null.

test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line 66:

> 64:                                                     .getProtectionDomain()
> 65:                                                     .getCodeSource()
> 66:                                                     .getLocation();

This is essentially the classpath.   An alternative way to get the location through `System.getProperty("test.class.path")`.

test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line 86:

> 84: 
> 85:         // First get hold of the target classes before we enable security
> 86:         Class<?> sealed = testLayer.findLoader("test").loadClass("test.Base");

I would recommend to use `Class::forName` instead of `ClassLoader::loadClass`  even though this is just a test (for security reason for example avoid type confusion).   If you want to load a class from a specific module, you can use `Class.forName(String cn, Module m)`

test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line 91:

> 89:         Class<?>[] subclasses = sealed.getPermittedSubclasses();
> 90: 
> 91:         if (subclasses.length != 3) {

I suggest to check against the expected list of permitted subclasses here and also the validation in the subsequent calls to `getPermittedSubclasses` with security manager enabled.   That would help the readers easier to understand this test.

test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line 120:

> 118: 
> 119:         //should pass - does not return a class from package "test":
> 120:         sealed.getPermittedSubclasses();

Adding a check to the expected returned value would make this clear (no permitted subclasses of package `test`).

src/java.base/share/classes/java/lang/Class.java line 3035:

> 3033:      */
> 3034:     private static void checkPackageAccessForPermittedSubclasses(SecurityManager sm,
> 3035:                                     final ClassLoader ccl, boolean checkProxyInterfaces,

`checkProxyInterfaces` parameter is not needed.  It can be removed.

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

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1483