Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8246778: Compiler implementation for Sealed Classes (Second Preview) #1227

Closed
Changes from all commits
Commits
File filter
Filter file types
Beta Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -2127,16 +2127,32 @@ JVM_ENTRY(jobjectArray, JVM_GetPermittedSubclasses(JNIEnv* env, jclass current))
JvmtiVMObjectAllocEventCollector oam;
Array<u2>* subclasses = ik->permitted_subclasses();
int length = subclasses == NULL ? 0 : subclasses->length();
objArrayOop r = oopFactory::new_objArray(SystemDictionary::String_klass(),
objArrayOop r = oopFactory::new_objArray(SystemDictionary::Class_klass(),
length, CHECK_NULL);
objArrayHandle result(THREAD, r);
int count = 0;
for (int i = 0; i < length; i++) {
int cp_index = subclasses->at(i);
// This returns <package-name>/<class-name>.
Symbol* klass_name = ik->constants()->klass_name_at(cp_index);
assert(klass_name != NULL, "Unexpected null klass_name");
Handle perm_subtype_h = java_lang_String::create_from_symbol(klass_name, CHECK_NULL);
result->obj_at_put(i, perm_subtype_h());
Klass* k = ik->constants()->klass_at(cp_index, THREAD);
if (HAS_PENDING_EXCEPTION) {
if (PENDING_EXCEPTION->is_a(SystemDictionary::VirtualMachineError_klass())) {
return NULL; // propagate VMEs
}
CLEAR_PENDING_EXCEPTION;
continue;
}
if (k->is_instance_klass()) {
result->obj_at_put(count++, k->java_mirror());
}
}
if (count < length) {
objArrayOop r2 = oopFactory::new_objArray(SystemDictionary::Class_klass(),
count, CHECK_NULL);
objArrayHandle result2(THREAD, r2);
for (int i = 0; i < count; i++) {
result2->obj_at_put(i, result->obj_at(i));
}
return (jobjectArray)JNIHandles::make_local(THREAD, result2());
}
return (jobjectArray)JNIHandles::make_local(THREAD, result());
}
@@ -200,8 +200,6 @@
private static final int ENUM = 0x00004000;
private static final int SYNTHETIC = 0x00001000;

private static final ClassDesc[] EMPTY_CLASS_DESC_ARRAY = new ClassDesc[0];

private static native void registerNatives();
static {
registerNatives();
@@ -4366,49 +4364,45 @@ public String descriptorString() {
* may be removed in a future release, or upgraded to permanent
* features of the Java language.}
*
* Returns an array containing {@code ClassDesc} objects representing all the
* Returns an array containing {@code Class} objects representing all the
* direct subclasses or direct implementation classes permitted to extend or
* implement this class or interface if it is sealed. The order of such elements
* is unspecified. If this {@code Class} object represents a primitive type,
* {@code void}, an array type, or a class or interface that is not sealed,
* an empty array is returned.
*
* @return an array of class descriptors of all the permitted subclasses of this class or interface
* @return an array of class objects of all the permitted subclasses of this class or interface
*
* @jls 8.1 Class Declarations
* @jls 9.1 Interface Declarations
* @since 15
*/
@jdk.internal.PreviewFeature(feature=jdk.internal.PreviewFeature.Feature.SEALED_CLASSES, essentialAPI=false)
public ClassDesc[] permittedSubclasses() {
String[] subclassNames;
if (isArray() || isPrimitive() || (subclassNames = getPermittedSubclasses0()).length == 0) {
return EMPTY_CLASS_DESC_ARRAY;
}
ClassDesc[] constants = new ClassDesc[subclassNames.length];
int i = 0;
for (String subclassName : subclassNames) {
try {
constants[i++] = ClassDesc.of(subclassName.replace('/', '.'));
} catch (IllegalArgumentException iae) {
throw new InternalError("Invalid type in permitted subclasses information: " + subclassName, iae);
}
public Class<?>[] getPermittedSubclasses() {

This comment has been minimized.

@mlchung

mlchung Nov 17, 2020
Member

What happens if a permitted subclass is not found? I see that getPermittedSubclasses0 ignores the entry if the class is not found. Should that be specified?

Have you considered whether security package access is needed (now that this method returns Class objects the caller may not have access to)? This needs to be discussed with the security team. If someone gets a hold of a sealed class (e.g. obj.getClass()), this method could leak other Class objects.

Class<?>[] subClasses;
if (isArray() || isPrimitive() || (subClasses = getPermittedSubclasses0()).length == 0) {
return EMPTY_CLASS_ARRAY;
}
return constants;
return subClasses;
}

/**
* * {@preview Associated with sealed classes, a preview feature of the Java language.
* {@preview Associated with sealed classes, a preview feature of the Java language.
*
* This method is associated with <i>sealed classes</i>, a preview
* feature of the Java language. Preview features
* may be removed in a future release, or upgraded to permanent
* features of the Java language.}
*
* Returns {@code true} if and only if this {@code Class} object represents a sealed class or interface.
* If this {@code Class} object represents a primitive type, {@code void}, or an array type, this method returns
* Returns {@code true} if and only if this {@code Class} object represents
* a sealed class or interface. If this {@code Class} object represents a
* primitive type, {@code void}, or an array type, this method returns
* {@code false}.
*
* @apiNote
* This method reports on a distinct concept of sealing from
* {@link Package#isSealed() Package::isSealed}.
*
* @return {@code true} if and only if this {@code Class} object represents a sealed class or interface.
*
* @jls 8.1 Class Declarations
@@ -4421,8 +4415,8 @@ public boolean isSealed() {
if (isArray() || isPrimitive()) {
return false;
}
return permittedSubclasses().length != 0;
return getPermittedSubclasses().length != 0;
}

private native String[] getPermittedSubclasses0();
private native Class<?>[] getPermittedSubclasses0();
}
@@ -221,6 +221,10 @@ public String getImplementationVendor() {
/**
* Returns true if this package is sealed.
*
* @apiNote
* This method reports on a distinct concept of sealing from
* {@link Class#isSealed() Class::isSealed}.
*

This comment has been minimized.

@AlanBateman

AlanBateman Nov 16, 2020
Contributor

This API note will be very confusing to readers. I think the javadoc will need to be fleshed out and probably will need to link to a section the Package class description that defines the legacy concept of sealing.

This comment has been minimized.

@mlchung

mlchung Nov 17, 2020
Member

I agree. This @APinote needs more clarification to help the readers to understand the context here. One thing we could do in the Package class description to add a "Package Sealing" section that can also explain that it has no relationship to "sealed classes".

This comment has been minimized.

@mlchung

mlchung Nov 24, 2020
Member

I added an API note in Package::isSealed [1] to clarify sealed package vs sealed class or interface.

The API note you added in Class::isSealed can be clarified in a similar fashion like: "Sealed class or interface has no relationship with {@linkplain Package#isSealed package sealing}".

[1] 3c230b8

This comment has been minimized.

@lahodaj

lahodaj Nov 25, 2020
Contributor

Thanks for that update, Mandy! I've tweaked the API note as per your recommendation. I'll publish a fixed PR later, reflecting the other review comments as well.

* @return true if the package is sealed, false otherwise
*/
public boolean isSealed() {
@@ -81,7 +81,7 @@ static JNINativeMethod methods[] = {
{"getNestMembers0", "()[" CLS, (void *)&JVM_GetNestMembers},
{"getRecordComponents0", "()[" RC, (void *)&JVM_GetRecordComponents},
{"isRecord0", "()Z", (void *)&JVM_IsRecord},
{"getPermittedSubclasses0", "()[" STR, (void *)&JVM_GetPermittedSubclasses},
{"getPermittedSubclasses0", "()[" CLS, (void *)&JVM_GetPermittedSubclasses},
};

#undef OBJ
@@ -1629,32 +1629,69 @@ public boolean isCastable(Type t, Type s) {
}

/**
* Is t is castable to s?<br>
* Is t castable to s?<br>
* s is assumed to be an erased type.<br>
* (not defined for Method and ForAll types).
*/
public boolean isCastable(Type t, Type s, Warner warn) {
// if same type
if (t == s)
return true;
// if one of the types is primitive
if (t.isPrimitive() != s.isPrimitive()) {
t = skipTypeVars(t, false);
return (isConvertible(t, s, warn)
|| (s.isPrimitive() &&
isSubtype(boxedClass(s).type, t)));
}
boolean result;
if (warn != warnStack.head) {
try {
warnStack = warnStack.prepend(warn);
checkUnsafeVarargsConversion(t, s, warn);
return isCastable.visit(t,s);
result = isCastable.visit(t,s);
} finally {
warnStack = warnStack.tail;
}
} else {
return isCastable.visit(t,s);
result = isCastable.visit(t,s);
}
if ((t.tsym.isSealed() || s.tsym.isSealed())) {

This comment has been minimized.

@mcimadamore

mcimadamore Nov 16, 2020
Contributor

It would probably be better to only run this extra check if result == true, to minimize compatibility impact.

return (t.isCompound() || s.isCompound()) ?
false :
!areDisjoint((ClassSymbol)t.tsym, (ClassSymbol)s.tsym);
}
return result;
}
// where
private boolean areDisjoint(ClassSymbol ts, ClassSymbol ss) {
if (isSubtype(ts.type, ss.type)) {
return false;
}
// if both are classes or both are interfaces, shortcut
if (ts.isInterface() == ss.isInterface()) {

This comment has been minimized.

@mcimadamore

mcimadamore Nov 16, 2020
Contributor

What happens with code like this?

interface A permits B { }
non-sealed interface B extends A { }
interface C { }

class D implements C, B { } // this is a valid witness for both A and C, but A and C are unrelated with subtyping

class Test {
  void m(A a, C c) {
     a = (A)c;
  }
}```
Note that, w/o sealed types, the above snippet compiles ok - e.g. casting C to A is not going to give problems (as there could be a common subtype D <: A, C).
return !(isSubtype(ss.type, ts.type));
}
if (ts.isInterface() && !ss.isInterface()) {
/* so ts is interface but ss is a class
* an interface is disjoint from a class if the class is disjoint form the interface
*/
return areDisjoint(ss, ts);
}
// a final class that is not subtype of ss is disjoint
if (!ts.isInterface() && ts.isFinal()) {
return true;
}
// if at least one is sealed
if (ts.isSealed() || ss.isSealed()) {
// permitted subtypes have to be disjoint with the other symbol
ClassSymbol sealedOne = ts.isSealed() ? ts : ss;
ClassSymbol other = sealedOne == ts ? ss : ts;
return sealedOne.permitted.stream().allMatch(sym -> areDisjoint((ClassSymbol)sym, other));
}
return false;
}

private TypeRelation isCastable = new TypeRelation() {

public Boolean visitType(Type t, Type s) {
@@ -1306,7 +1306,10 @@ else if ((sym.kind == TYP ||
SEALED | NON_SEALED)
&& checkDisjoint(pos, flags,
SEALED,
FINAL | NON_SEALED)) {
FINAL | NON_SEALED)
&& checkDisjoint(pos, flags,
SEALED,
ANNOTATION)) {
// skip
}
return flags & (mask | ~ExtendedStandardFlags) | implicit;
@@ -4264,11 +4264,19 @@ protected boolean isSealedClassStart(boolean local) {
private boolean allowedAfterSealedOrNonSealed(Token next, boolean local, boolean currentIsNonSealed) {
return local ?
switch (next.kind) {
case MONKEYS_AT, ABSTRACT, FINAL, STRICTFP, CLASS, INTERFACE, ENUM -> true;
case MONKEYS_AT -> {
Token afterNext = S.token(2);
yield afterNext.kind != INTERFACE || currentIsNonSealed;
}
case ABSTRACT, FINAL, STRICTFP, CLASS, INTERFACE, ENUM -> true;
default -> false;
} :
switch (next.kind) {
case MONKEYS_AT, PUBLIC, PROTECTED, PRIVATE, ABSTRACT, STATIC, FINAL, STRICTFP, CLASS, INTERFACE, ENUM -> true;
case MONKEYS_AT -> {
Token afterNext = S.token(2);
yield afterNext.kind != INTERFACE || currentIsNonSealed;
}
case PUBLIC, PROTECTED, PRIVATE, ABSTRACT, STATIC, FINAL, STRICTFP, CLASS, INTERFACE, ENUM -> true;
case IDENTIFIER -> isNonSealedIdentifier(next, currentIsNonSealed ? 3 : 1) || next.name() == names.sealed;
default -> false;
};
@@ -21,11 +21,10 @@
* questions.
*/

// This class has entries in its PermittedSubclasses attribute that do not exist.
// Test that this does not prevent JVM_GetPermittedSubclasses() from returning
// their names.
// This class has an entry in its PermittedSubclasses attribute that does not exist.
// Test that JVM_GetPermittedSubclasses() only returns the existing class.
//
// sealed class NoLoadSubclasses permits iDontExist, I/Dont/Exist/Either { }
// sealed class NoLoadSubclasses permits OldClassFile, I/Dont/Exist/Either { }
//
class NoLoadSubclasses {
0xCAFEBABE;
@@ -47,7 +46,7 @@ class NoLoadSubclasses {
Utf8 "NoLoadSubclasses.java"; // #12 at 0x73
Utf8 "PermittedSubclasses"; // #13 at 0x89
class #15; // #14 at 0x9D
Utf8 "iDontExist"; // #15 at 0xA0
Utf8 "OldClassFile"; // #15 at 0xA0
class #17; // #16 at 0xAA
Utf8 "I/Dont/Exist/Either"; // #17 at 0xAD
} // Constant Pool
@@ -50,7 +50,7 @@
final class Final4 {}

public static void testSealedInfo(Class<?> c, String[] expected) {
Object[] permitted = c.permittedSubclasses();
Object[] permitted = c.getPermittedSubclasses();

if (permitted.length != expected.length) {
throw new RuntimeException(
@@ -65,7 +65,7 @@ public static void testSealedInfo(Class<?> c, String[] expected) {
// Create ArrayList of permitted subclasses class names.
ArrayList<String> permittedNames = new ArrayList<String>();
for (int i = 0; i < permitted.length; i++) {
permittedNames.add(((ClassDesc)permitted[i]).descriptorString());
permittedNames.add(((Class)permitted[i]).getName());
}

if (permittedNames.size() != expected.length) {
@@ -102,10 +102,11 @@ public static void testBadSealedClass(String className, String expectedCFEMessag
}

public static void main(String... args) throws Throwable {
testSealedInfo(SealedI1.class, new String[] {"LGetPermittedSubclassesTest$NotSealed;",
"LGetPermittedSubclassesTest$Sub1;",
"LGetPermittedSubclassesTest$Extender;"});
testSealedInfo(Sealed1.class, new String[] {"LGetPermittedSubclassesTest$Sub1;"});
testSealedInfo(SealedI1.class, new String[] {"GetPermittedSubclassesTest$NotSealed",
"GetPermittedSubclassesTest$Sub1",
"GetPermittedSubclassesTest$Extender"});

testSealedInfo(Sealed1.class, new String[] {"GetPermittedSubclassesTest$Sub1"});
testSealedInfo(Final4.class, new String[] { });
testSealedInfo(NotSealed.class, new String[] { });

@@ -115,8 +116,8 @@ public static void main(String... args) throws Throwable {
// Test class with an empty PermittedSubclasses attribute.
testBadSealedClass("NoSubclasses", "PermittedSubclasses attribute is empty");

// Test returning names of non-existing classes.
testSealedInfo(NoLoadSubclasses.class, new String[]{"LiDontExist;", "LI/Dont/Exist/Either;"});
// Test returning only names of existing classes.
testSealedInfo(NoLoadSubclasses.class, new String[]{"OldClassFile" });

// Test that loading a class with a corrupted PermittedSubclasses attribute
// causes a ClassFormatError.
@@ -86,8 +86,8 @@
public void testSealedClasses(Class<?> cls) {
assertTrue(cls.isSealed());
assertTrue(!Modifier.isFinal(cls.getModifiers()));
assertTrue(cls.permittedSubclasses() != null);
assertTrue(cls.permittedSubclasses().length > 0);
assertTrue(cls.getPermittedSubclasses() != null);
assertTrue(cls.getPermittedSubclasses().length > 0);
}

@DataProvider(name = "notSealedClasses")
@@ -110,8 +110,8 @@ public void testSealedClasses(Class<?> cls) {
@Test(dataProvider = "notSealedClasses")
public void testNotSealedClasses(Class<?> cls) {
assertTrue(!cls.isSealed());
assertTrue(cls.permittedSubclasses() != null);
assertTrue(cls.permittedSubclasses().length == 0);
assertTrue(cls.getPermittedSubclasses() != null);
assertTrue(cls.getPermittedSubclasses().length == 0);
}

@DataProvider(name = "non_sealedClasses")
@@ -128,8 +128,8 @@ public void testnon_sealedClasses(Class<?> cls) {
assertTrue(!cls.isSealed());
assertTrue(!Modifier.isFinal(cls.getModifiers()));
assertTrue((cls.getSuperclass() != null && cls.getSuperclass().isSealed()) || Arrays.stream(cls.getInterfaces()).anyMatch(Class::isSealed));
assertTrue(cls.permittedSubclasses() != null);
assertTrue(cls.permittedSubclasses().length == 0);
assertTrue(cls.getPermittedSubclasses() != null);
assertTrue(cls.getPermittedSubclasses().length == 0);
}

@DataProvider(name = "reflectionData")
@@ -215,10 +215,10 @@ public void testSealedReflection(Class<?> sealedClass,
throws ReflectiveOperationException
{
assertTrue(sealedClass.isSealed());
assertTrue(sealedClass.permittedSubclasses().length == numberOfSubclasses);
assertTrue(sealedClass.getPermittedSubclasses().length == numberOfSubclasses);
int i = 0;
for (ClassDesc cd : sealedClass.permittedSubclasses()) {
assertTrue(cd.displayName().equals(subclassDescriptors[i]), "expected: " + subclassDescriptors[i] + " found: " + cd.displayName());
for (Class<?> cd : sealedClass.getPermittedSubclasses()) {
assertTrue(cd.getName().equals(subclassDescriptors[i]), "expected: " + subclassDescriptors[i] + " found: " + cd.getName());
i++;
}
i = 0;
ProTip! Use n and p to navigate between commits in a pull request.