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) #1483

Closed
wants to merge 15 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Nov 27, 2020

This pull request replaces #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
Sealed Classes JVMS
Additional: Contextual Keywords

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

/contributor add vromero
/contributor add hseigel


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8246778: Compiler implementation for Sealed Classes (Second Preview)

Reviewers

Contributors

  • Vicente Romero <vromero@openjdk.org>
  • Harold Seigel <hseigel@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1483/head:pull/1483
$ git checkout pull/1483

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 27, 2020

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Nov 27, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 27, 2020

@lahodaj
Contributor Vicente Romero <vromero@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 27, 2020

@lahodaj
Contributor Harold Seigel <hseigel@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 27, 2020

@lahodaj The following labels will be automatically applied to this pull request:

  • compiler
  • core-libs
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 27, 2020

* Then these names are converted to {@code Class} instances using
* {@linkplain #getClassLoader() the defining class loader} of the current
* {@code Class} object. If a name cannot be converted to the {@code Class}
* instance, it is silently excluded from the result.

This comment has been minimized.

@AlanBateman

AlanBateman Nov 30, 2020
Contributor

I think this paragraph will need a little bit of wordsmithing. The 3rd paragraph of getNestMembers might be useful to examine as it more clearly describes how the method attempts to "obtain" the Class object for each of the class names in the NestMembers attribute and maybe some of that wording could be used instead of using the term "convert".

Minor nit but the prevailing style for the @throws SecurityException is to align the description with the exception, probably best to keep it consistent if you can.

This comment has been minimized.

@lahodaj

lahodaj Nov 30, 2020
Author Contributor

Thanks, I've tried to improve the javadoc here:
4d48417

* loader} of the current {@code Class} object).
* The {@code Class} objects which can be obtained using this procedure
* are indicated by elements of the returned array. If a {@code Class} object
* cannot be obtained, it is silently ignored, and not included in the result

This comment has been minimized.

@AlanBateman

AlanBateman Nov 30, 2020
Contributor

Thanks for the update, this reads much better.

* {@code false}.
*
* @apiNote
* Sealed class or interface has no relationship with
* {@linkplain Package#isSealed package sealing}.

This comment has been minimized.

@mlchung

mlchung Nov 30, 2020
Member

Package sealing is legacy. Remi suggests to take out this api note which sounds good to me.

The API note in Package::isSealed has made this clear which has no relationship with sealed class or interface.

* cannot be obtained, it is silently ignored, and not included in the result
* array.
*
* @return an array of class objects of the permitted subclasses of this class or interface

This comment has been minimized.

@mlchung

mlchung Nov 30, 2020
Member

Nit: s/class objects/{@code Class} objects/

}

private native String[] getPermittedSubclasses0();
private native Class<?>[] getPermittedSubclasses0();

This comment has been minimized.

@mlchung

mlchung Nov 30, 2020
Member

Does this JVM method return the permitted subclasses or subinterfaces with the following conditions enforced by JLS:

  • If a sealed class C belongs to a named module, then every class named in the permits clause of the declaration of C must belong to the same module as C
  • If a sealed class C belongs to an unnamed module, then every class named in the permits clause of the declaration of C must belong to the same package as C

I didn't check the VM implementation.

If the return array contains only classes as specified above, checkPackageAccessForClasses can be simplified.

This comment has been minimized.

@hseigel

hseigel Nov 30, 2020
Member

The JVM method that returns the permitted subclasses (and interfaces) does not weed out permitted subclasses based on the above module requirements. It returns all the classes listed in the PermittedSubclasses attribute that it is able to load.

This comment has been minimized.

@mlchung

mlchung Dec 1, 2020
Member

So it could also return a class listed in PermittedSubclasses attribute but not a subclass of this sealed class, right?

The specification of Class::getPermittedSubclasses says:

Returns an array containing {@code Class} objects representing the direct subclasses or direct implementation classes permitted to extend or direct subinterfaces or subclasses permitted to extend or implement this class or interface if it is sealed.

I expect that Class::getPermittedSubclasses should do more work and return only the subclasses or subinterfaces permitted at runtime.

This comment has been minimized.

@lahodaj

lahodaj Dec 1, 2020
Author Contributor

I was investigating a little today. One thing to note is that there is a difference between the JLS and JVMS[1] restrictions - the JVMS restrictions only require the classes to be in the same module, but they can be in any package (even for an unnamed module).

Moreover, a lot of the constraints are checked automatically: e.g. consider class api.Api in module "a", which permits impl.Impl in module "b", subtyping Api. Loading of impl.Impl on behalf of getPermittedSubclasses() will fail (because the two classes are in different modules). So impl.Impl will not be included.

So far, it seems the only constraint that I think is not satisfied by this implicit loading is that the permitted subclass is really a direct subtype of the current class.

My proposal is to enhance the Java method with the direct subtype check (with a possible future cleanup task to move the check to native, as is done for getNestMembers()).

[1] http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html#jvms-5.3.5

This comment has been minimized.

@mlchung

mlchung Dec 1, 2020
Member

@lahodaj It is okay with me if getPermittedSubclasses returns the permitted subtypes matching the runtime view (that matches the current specification to me) and revisit this API as a follow up.

This comment has been minimized.

@AlanBateman

AlanBateman Dec 2, 2020
Contributor

Yes, would be a surprise if getPermittedSubclasses returned Class objects for classes that are not subclasses. I think it should be okay to separate that out to a separate issue so that it can be further re-examined after JEP 397 goes in.

This comment has been minimized.

@lahodaj

lahodaj Dec 2, 2020
Author Contributor

FWIW: I plan to change the method to not return such Class objects. It just will do the verification in Java, while for getNestMembers it is done in native. The follow up is for investigate the possibility of making the internal behavior consistent with how getNestMembers work.

This comment has been minimized.

@ChrisHegarty

ChrisHegarty Dec 2, 2020
Member

For the record: similar/related questions/comments have been made on a-s-e [1][2]. The questions/comments were made in order to clarify the JVMS specification, so as to better inform what responsibilities are part Core Reflection versus what are part of the VM.

[1] https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-October/002626.html
[2] https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002660.html


for (Class<?> c : classes) {
// skip the package access check on a proxy class in default proxy package
if (!Proxy.isProxyClass(c) || ReflectUtil.isNonPublicProxyClass(c)) {

This comment has been minimized.

@mlchung

mlchung Nov 30, 2020
Member

If a sealed class is in a named module, the permitted subclasses/subinterfaces are in the same module as the sealed class. If a sealed class is in an unnamed module, it will be in the same runtime package as the sealed class. A proxy class is dynamically generated and not intended for statically named in permits clause of a sealed class`. It can be in a different module or different package. So a permitted subclass or interface should never be a proxy class.

So the package access check for permitted subclasses/subinterfaces can be simplified. I would suggest this check be inlined in getPermittedSubclasses as follows:

      SecurityManager sm = System.getSecurityManager();
        if (subclasses.length > 0 && sm != null) {
            ClassLoader ccl = ClassLoader.getClassLoader(Reflection.getCallerClass());
            ClassLoader cl = getClassLoader0();
            if (ReflectUtil.needsPackageAccessCheck(ccl, cl)) {
                Set<String> packages = new HashSet<>();
                for (Class<?> c : subclasses) {
                    if (Proxy.isProxyClass(c))
                        throw new InternalError("a permitted subclass should not be a proxy class: " + c);
                    String pkg = c.getPackageName();
                    if (!pkg.isEmpty())
                        packages.add(pkg);
                }
                for (String pkg : packages) {
                    sm.checkPackageAccess(pkg);
                }
            }
        }
@@ -50,7 +50,7 @@
final class Final4 {}

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

This comment has been minimized.

@mlchung

mlchung Dec 1, 2020
Member

Why permitted is not Class<?>[]? or simply var permitted = ...

@@ -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());
}

This comment has been minimized.

@mlchung

mlchung Dec 1, 2020
Member

This cast is not needed if permitted is var or Class<?>[] type.

@@ -0,0 +1,260 @@
/*
* Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

This comment has been minimized.

@mlchung

mlchung Dec 1, 2020
Member

the copyright start year should be 2020.

@mlchung
Copy link
Member

@mlchung mlchung commented Dec 1, 2020

From David's comment:

These subtleties are what I was also getting at in the CSR discussion. If Foo lists Bar as a permitted subtype, but Bar does not extend Foo, is that an error? Should Bar not be returned by getPermittedSubclasses? The answer depends on exactly how you specify getPermittedSubclasses() and whether that aligns with reading the classfile attribute, or actually checking the runtime relationships.

As I read the current spec of Class::getPermittedSubclasses, it intends to return the direct subclasses or subinterfaces permitted to extend or implement this class or interface if it's sealed. If not, it should be clarified that it is the class file view.

NestMembers and PermittedSubclasses attributes are critical to correct interpretation of the class file by JVM. Prior to these two attributes, the attributes inspected by core reflection APIs are all non-critical. API like Class::getDeclaringClass reads InnerClasses attribute if present in order to determine its declaring class but the current spec does not specify the behavior on error cases (which I consider a spec bug - see JDK-8250226.)

IMO it is reasonable for getPermittedSubclasses (and getNestMembers and getNestHost) to return the runtime view as it returns Class objects since these attributes are critical to correct interpretation of the class file. It would be confusing if it returns Foo that is not a permitted subtype at runtime.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 2, 2020

Mailing list message from David Holmes on compiler-dev:

On 1/12/2020 10:39 am, Mandy Chung wrote:

On Mon, 30 Nov 2020 20:57:32 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

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

4478: }
4479:
4480: private native Class<?>[] getPermittedSubclasses0();

Does this JVM method return the permitted subclasses or subinterfaces with the following conditions enforced by JLS:

\- If a sealed class C belongs to a named module\, then every class named in the permits clause of the declaration of C must belong to the same module as C
\- If a sealed class C belongs to an unnamed module\, then every class named in the permits clause of the declaration of C must belong to the same package as C

I didn't check the VM implementation.

If the return array contains only classes as specified above, `checkPackageAccessForClasses` can be simplified.

The JVM method that returns the permitted subclasses (and interfaces) does not weed out permitted subclasses based on the above module requirements. It returns all the classes listed in the PermittedSubclasses attribute that it is able to load.

So it could also return a class listed in `PermittedSubclasses` attribute but not a subclass of this sealed class, right?

The specification of `Class::getPermittedSubclasses` says:

Returns an array containing {@code Class} objects representing the direct subclasses or direct implementation classes permitted to extend or direct subinterfaces or subclasses permitted to extend or implement this class or interface if it is sealed.

I expect that `Class::getPermittedSubclasses` should do more work and return only the subclasses or subinterfaces permitted at runtime.

These subtleties are what I was also getting at in the CSR discussion.
If Foo lists Bar as a permitted subtype, but Bar does not extend Foo, is
that an error? Should Bar not be returned by getPermittedSubclasses? The
answer depends on exactly how you specify getPermittedSubclasses() and
whether that aligns with reading the classfile attribute, or actually
checking the runtime relationships.

David
-----

…s that are not a subtype of the current class, and other adjustments per the review feedback.
@hseigel
Copy link
Member

@hseigel hseigel commented Dec 2, 2020

Additional changes may be needed to Class.permittedSubclasses() and/or Class.isSealed() as part of fixing bug JDK-8256867. The JVM is being changed to treat classes with empty PermittedSubclasses attributes as sealed classes that cannot be extended (or implemented).

Current thinking is that Class.permittedSubclasses() will return an empty array for both non-sealed classes and for sealed classes with empty PermittedSubclasses attributes. And, Class.isSealed() will return False in the former case and True in the latter. This will require changing the implementation of Class.isSealed() to call the JVM directly instead of calling Class.permittedSubclasses().

Does this seem like a reasonable way to handle this corner case?

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 2, 2020

Additional changes may be needed to Class.permittedSubclasses() and/or Class.isSealed() as part of fixing bug JDK-8256867. The JVM is being changed to treat classes with empty PermittedSubclasses attributes as sealed classes that cannot be extended (or implemented).

Current thinking is that Class.permittedSubclasses() will return an empty array for both non-sealed classes and for sealed classes with empty PermittedSubclasses attributes. And, Class.isSealed() will return False in the former case and True in the latter. This will require changing the implementation of Class.isSealed() to call the JVM directly instead of calling Class.permittedSubclasses().

Does this seem like a reasonable way to handle this corner case?

Uh, I just realized it may be necessary to implement Class.isSealed() differently. Consider:

sealed class Sealed permits Unknown {}

Where Unknown does not exist at runtime. So getPermittedSubclasses0() returns an empty array(?). But isSealed should return true, right? (Possibly, we could leave this from the first integration, but seems like something that should be done. Note that in the previous/JDK 15 implementation, isSealed() would return true in this case.)

@ChrisHegarty
Copy link
Member

@ChrisHegarty ChrisHegarty commented Dec 2, 2020

...
Uh, I just realized it may be necessary to implement Class.isSealed() differently. Consider:

sealed class Sealed permits Unknown {}

Where Unknown does not exist at runtime. So getPermittedSubclasses0() returns an empty array(?). But isSealed should return true, right? (Possibly, we could leave this from the first integration, but seems like something that should be done. Note that in the previous/JDK 15 implementation, isSealed() would return true in this case.)

@lahodaj I raised this issue here: https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-October/002626.html

Relevant comment inline:

> The recent change (proposed on this list) to Class::getPermittedSubclasses, means that it will no longer be possible to reflectively return permitted subclasses that are not loaded, or more specifically “loadable" - the classes must exist somewhere. Currently, in JDK 15, permittedSubclasses will return class descriptors for non-loadable classes. I think that this is ok, we just need to ensure that it fits into the other rules here.

Yep. I think this is a good change, and I think there's nothing wrong with the reflection API either ignoring some entries or reporting class loading errors. Best to do whatever the NestMembers query does.
@mlchung
Copy link
Member

@mlchung mlchung commented Dec 2, 2020

Additional changes may be needed to Class.permittedSubclasses() and/or Class.isSealed() as part of fixing bug JDK-8256867. The JVM is being changed to treat classes with empty PermittedSubclasses attributes as sealed classes that cannot be extended (or implemented).

Current thinking is that Class.permittedSubclasses() will return an empty array for both non-sealed classes and for sealed classes with empty PermittedSubclasses attributes. And, Class.isSealed() will return False in the former case and True in the latter. This will require changing the implementation of Class.isSealed() to call the JVM directly instead of calling Class.permittedSubclasses().

Does this seem like a reasonable way to handle this corner case?

I suggest Class::getPermittedSubclasses to return a non-null array if this Class is sealed, i.e. this class is derived from a class file with the presence of PermittedSubclasses attribute regardless of its content (the attribute could be empty or contains zero or more entries which is a properly loaded permitted subtype.

If this Class is not sealed, Class::getPermittedSubclasses returns null (see Class::getRecordComponents and some other reflection APIs as precedence).

@dansmithcode
Copy link

@dansmithcode dansmithcode commented Dec 2, 2020

I suggest Class::getPermittedSubclasses to return a non-null array if this Class is sealed, i.e. this class is derived from a class file with the presence of PermittedSubclasses attribute regardless of its content (the attribute could be empty or contains zero or more entries which is a properly loaded permitted subtype.

If this Class is not sealed, Class::getPermittedSubclasses returns null (see Class::getRecordComponents and some other reflection APIs as precedence).

Agree, that seems reasonable. Often, methods in Class with an array return type default to an empty array, but getRecordComponents is a good example of returning null when an empty array is meaningful.

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 3, 2020

I've changed Class.getPermittedSubclasses to return null for classes that are not sealed here:
7056143
with follow-up changes to tests here:
537e267

How does this look?

@lahodaj lahodaj force-pushed the lahodaj:JDK-8246778 branch from 537e267 to ff1abf0 Dec 3, 2020
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 3, 2020

After a discussion with Harold, I've reverted the patch where Class.getPermittedSubclasses returns null. Harold will do that separatelly under JDK-8256867, unless there are objections.

The changes that were reverted are still available here:
https://openjdk.github.io/cr/?repo=jdk&pr=1483&range=02-03

Please let me know if there are any issues.

@lfoltan
lfoltan approved these changes Dec 3, 2020
Copy link
Member

@lfoltan lfoltan left a comment

I have reviewed the hotspot changes and they look good.
Thanks,
Lois

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@lahodaj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8246778: Compiler implementation for Sealed Classes (Second Preview)

Co-authored-by: Vicente Romero <vromero@openjdk.org>
Co-authored-by: Harold Seigel <hseigel@openjdk.org>
Reviewed-by: lfoltan, mchung, alanb, mcimadamore, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 147 new commits pushed to the master branch:

  • d8ac76f: 8257651: LambdaEagerInit.java test failed in 2 different ways
  • 3eb6946: 8257732: Problem list TestJFRWithJMX for OL 8.2 until the issue is resolved
  • ac54900: 8257401: Use switch expressions in jdk.internal.net.http and java.net.http
  • 86b6575: 8257574: C2: "failed: parsing found no loops but there are some" assert failure
  • dede01e: 8257622: MemoryAccess methods are missing @forceinline annotations
  • 1d2d981: 8257423: [PPC64] Support -XX:-UseInlineCaches
  • feabdde: 8251843: jfr/tool/TestPrintJSON.java fails intermittently
  • c6f93ec: 8257707: Fix incorrect format string in Http1HeaderParser
  • fbdc187: 8257624: C2: PhaseMacroExpand::eliminate_macro_nodes() crashes on out-of-bounds access into macro node array
  • fd6756e: 8257634: C2: Introduce IncrementalInliningForceCleanup diagnostic flag
  • ... and 137 more: https://git.openjdk.java.net/jdk/compare/6e0062236c836b261cde5792963fb9de3068b8f7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Dec 3, 2020
@mlchung
Copy link
Member

@mlchung mlchung commented Dec 3, 2020

After a discussion with Harold, I've reverted the patch where Class.getPermittedSubclasses returns null. Harold will do that separatelly under JDK-8256867, unless there are objections.

No objection. Keeping Class::getPermittedSubclasses as specified in the CSR is right thing to do so that this work can be integrated and resolve JDK-8256867 separately.

@mlchung
mlchung approved these changes Dec 3, 2020
Copy link
Member

@mlchung mlchung left a comment

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.


public class TestSecurityManagerChecks {

private static final ClassLoader OBJECT_CL = Object.class.getClassLoader();

This comment has been minimized.

@mlchung

mlchung Dec 3, 2020
Member

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

URL testLocation = TestSecurityManagerChecks.class
.getProtectionDomain()
.getCodeSource()
.getLocation();

This comment has been minimized.

@mlchung

mlchung Dec 3, 2020
Member

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

OBJECT_CL);

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

This comment has been minimized.

@mlchung

mlchung Dec 3, 2020
Member

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)

//try without a SecurityManager:
Class<?>[] subclasses = sealed.getPermittedSubclasses();

if (subclasses.length != 3) {

This comment has been minimized.

@mlchung

mlchung Dec 3, 2020
Member

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.

denyPackageAccess[0] = "test";

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

This comment has been minimized.

@mlchung

mlchung Dec 3, 2020
Member

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

* NOTE: this method does not support Proxy classes
*/
private static void checkPackageAccessForPermittedSubclasses(SecurityManager sm,
final ClassLoader ccl, boolean checkProxyInterfaces,

This comment has been minimized.

@mlchung

mlchung Dec 3, 2020
Member

checkProxyInterfaces parameter is not needed. It can be removed.

Jan Lahoda

for (Class<?> c : subClasses) {
if (Proxy.isProxyClass(c))
throw new InternalError("a permitted subclass should not be a proxy class: " + c);

This comment has been minimized.

@AlanBateman

AlanBateman Dec 4, 2020
Contributor

Minor nit but I think the indentation may be messed up here.

* Returns an array containing {@code ClassDesc} objects representing all the
* direct subclasses or direct implementation classes permitted to extend or
* Returns an array containing {@code Class} objects representing the
* direct subinterfaces or subclasses 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,

This comment has been minimized.

@AlanBateman

AlanBateman Dec 4, 2020
Contributor

Did you mean {@code Void} here?

This comment has been minimized.

@lahodaj

lahodaj Dec 4, 2020
Author Contributor

I think this means void.class. void.class is a little special (as are the Class objects for primitive types, like int.class), but Void.class or Integer.class are not so much special.

I've updated the patch based on the other comments - thanks!

* object for {@code C} (using {@linkplain #getClassLoader() the defining class
* loader} of the current {@code Class} object).
* The {@code Class} objects which can be obtained using this procedure,
* and which are direct subinterfaces or subclasses of this class or interface,

This comment has been minimized.

@AlanBateman

AlanBateman Dec 4, 2020
Contributor

Minor suggestion is to drop "using this procedure" from this sentence.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Compiler changes look good to me

Jan Lahoda
@mlchung
mlchung approved these changes Dec 4, 2020
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 7, 2020

/integrate

@openjdk openjdk bot closed this Dec 7, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@lahodaj Since your change was applied there have been 167 commits pushed to the master branch:

  • 09707dd: 8252807: The jdk.jfr.Recording.getStream does not work when toDisk is disabled
  • 04ce8e3: 8257184: Upstream 8252504: Add a method to MemoryLayout which returns a offset-computing method handle
  • 5a03e47: 8255560: Class::isRecord should check that the current class is final and not abstract
  • 8e8e584: 8257588: Make os::_page_sizes a bitmask
  • 566d77a: 8254802: ThrowingPushPromisesAsStringCustom.java fails in "try throwing in GET_BODY"
  • f5a582c: 8257575: C2: "failed: only phis" assert failure in loop strip mining verification
  • d05401d: 8256679: Update serialization javadoc once JOSS changes for records are complete
  • 7620124: 8257230: assert(InitialHeapSize >= MinHeapSize) failed: Ergonomics decided on incompatible initial and minimum heap sizes
  • 05dac03: 8257803: Add -Xbatch to compiler/blackhole tests
  • 29a09c8: 8257668: SA JMap - skip non-java thread stack dump for heap dump
  • ... and 157 more: https://git.openjdk.java.net/jdk/compare/6e0062236c836b261cde5792963fb9de3068b8f7...master

Your commit was automatically rebased without conflicts.

Pushed as commit 637b0c6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment