On Wed, 2 Dec 2020 14:40:15 GMT, Jan Lahoda <jlah...@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
