Bug ID: JDK-8254183 Compiler implementation for Sealed Classes (Second Preview)

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

Type: CSR
Component: tools
Sub-Component: javac

Priority: P3
Status: Closed
Resolution: Approved
Fix Versions: 16

Submitted: 2020-10-07
Updated: 2020-12-12
Resolved: 2020-11-10

Related Reports

CSR :	JDK-8246778 - Compiler implementation for Sealed Classes (Second Preview)
Relates :	JDK-8246776 - JLS changes for Sealed Classes (Second Preview)
Relates :	JDK-8246278 - Refine API for sealing in java.lang.Class
Relates :	JDK-8246777 - JVMS changes for Sealed Classes (Second Preview)

Description

Summary
-------

Since the release of the "Sealed Classes" feature 
[JEP 360](https://openjdk.java.net/jeps/360) in Java SE 15, a small number of
clarifications, enhancements and fixes have been identified. The feature will
preview for a second time in JDK 16.

Problem
-------

Following testing, as well as feedback and discussions, the following issues
with the "Sealed Classes" preview feature have been identified:

 - The JLS is not clear about the use of context when applying the lexical
 grammar, particularly in the identification of what is described as contextual
 keywords. This affects, in particular, the new keywords introduced by this JEP:
 `sealed`, `non-sealed`, and `permits`. 
 
 - A local class is not be permitted to extend a
 `sealed` class or implement a `sealed` interface. Accordingly a local class
 should not be considered when determining the implicitly permitted subclasses
 of a `sealed` class or interface.
 
 - The current specification for narrowing reference conversions is too
 permissive, in that knowledge of sealed hierarchies could be used to identify
 erroneous casts and `instanceof` expressions at compile-time.
 
 - It has been suggested that the return type of method
 `java.lang.Class::permittedSubclasses` should not be
 `java.lang.constant.ClassDesc[]`, both for consistency issues and because it
 would force users to use the `java.lang.constant` API which is mostly intended
 for advanced users. It has also been suggested that the name of the method should be `getPermittedSubclasses` also on consistency basis.

Solution
--------

 - Clarify the use of context when applying the lexical grammar, in particular
 by introducing a first class notion of _contextual keywords_ (formerly
 described as "restricted identifiers" and "restricted keywords"). Then the
 character sequences `sealed`, `non-sealed`, and `permits` can be defined as
 contextual keywords.
 
 - Do not consider local classes when calculating the implicitly declared
 permitted direct subclasses of a `sealed` class or interface.
 
 - Extend the notion of a narrowing reference conversion to explicitly navigate
 sealed hierarchies to determine if a conversion can be rejected at
 compile-time. 
 
 - Change the return type of method `java.lang.Class::permittedSubclasses` to
 `java.lang.Class<?>[]` and rename it as `getPermittedSubclasses`
 

Specification
-------------

 - A new companion specification document that clarifies the treatment of contextual
 keywords is attached.
 
 - `Section 8.1.6 Permitted Direct Subclasses`, has been modified to state that
 if a `sealed` class C lacks a `permits` clause, local classes are not considered
 as permitted direct subclasses of C.
 
 - `Section 5.1.6.1 Allowed Narrowing Reference Conversion` has been modified in
 order to clarify the cases for which a narrowing reference conversion exist if
 `sealed` classes or interfaces are involved in the conversion.
 
 - The specification for method `java.lang.Class::permittedSubclasses` has been
 updated as follows:


```      
    -     * 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 Class<?>[] getPermittedSubclasses() {}
```

Comments

[~darcy] Some small changes were made to the spec following reviews. I attach the updated specs along with diff files showing precisely what changed between the specs.
07-12-2020
[~dholmes], technical review can still continue after a CSR is approved or a push is done.
24-11-2020
I think there is a somewhat self-fulfilling circular reasoning being applied here. We introduce new attributes and would like to expose a mechanism to inspect those attributes. We don't have such a mechanism so we fall back to adding methods to Class; we then justify implementing those methods in a way that doesn't actually inspect the attribute by appealing to what the existing (completely unrelated) methods of Class do. I think the comparison with other core reflection methods is an apples and oranges comparison. With regard to documenting failure modes etc, there has been a concerted effort by core-libs folk (i.e. Mandy) to address significant short-comings with APIs when it comes to the detail of loading versus linking versus initialization. New methods should be doing the best they can to accurately and completely specify their behaviour, not doing the least they think they can get away with based on pre-existing poorly specified methods. As it seems I am a lone voice and as this CSR request has been approved despite this discussion, further discussion here or elsewhere seems pointless.
24-11-2020
On `getPermittedSubclasses`: I'd suggest broadening the set of precedents we're thinking about to _almost every method_ in `java.lang.Class` and `java.lang.reflect.*`. Loading classes as needed to model the information contained in class files is simply how this API works. >The real question is: what is the purpose of this API? I think the answer to that should be "to inspect the permittedSubclases attribute of the class", not "to gain access to all the permitted subclasses of the class". This is the core of the differing perspectives, I think. And I'd argue that the nature of this API is the latter��it has decidedly _not_ tried to support inspection of class file attributes, separate from any attempt to interpret the contents. Compare `getGenericParameterTypes`, for just one example. We don't have an API for low-level inspection of loaded classes or class files. Perhaps we should, but `java.lang.Class` isn't it. I do think it's reasonable to adopt security protocols similar to those of other methods in the API, like `getDeclaredClasses`. The list of private classes that extend a sealed class is potentially sensitive information. (I'd suggest exploring this in more depth in the next release.) > That debate aside, the method not only needs to document the failure modes it needs to clearly state exactly how the subclasses will be attempted to be loaded. It should also clarify (which we didn't do with nestmates) that the class is only loaded and not necessarily linked and certainly not initialized. More documentation is (almost) always good, but keep in mind how poorly the rest of the API documents its failure modes. Classes are loaded on-demand everywhere, and very rarely is there any documentation about what happens if the attempt fails. (Again, `getDeclaredClasses` provides a good comparison. It has nothing to say about malformed or inconsistent InnerClasses attributes.) (Last note: I agree with Brian, if this deserves more discussion, probably best to pursue it on a mailing list.)
23-11-2020
[~darcy] Yes, I believe they are congruent. Currently when considering a narrowing reference conversion the compiler says yes UNLESS it can see that the two types are classes that are unrelated and the source is `final`. In this case there is no possible extension of the source type so we can conclude at runtime they are distinct. The proposed new relation keeps this behaviour, and for non-sealed classes, the behaviour is the same. The only change is if one of the classes - let's say the source - is sealed. If it is we use the fact that the subclasses are enumerated to consider them one by one (if it was non-sealed and non-`final` then we'd say "not distinct"). We recursively test the subclasses. But then we are then essentially back to the problem we had before - if the subclasses are `final` then they are distinct (old behaviour), if any are `non-sealed` then we say they are not-distinct (old behaviour), and if it is `sealed`, then we recursively descend. I think this is a conservative extension of what we have already; which is a very flexible relation that allows the test to happen at runtime unless it sees two types are unrelated and one of them is final.
23-11-2020
[~gbierman], per instanceof and compilation issues. At times, code may knowingly contain an instanceof check that would always pass/fail at compile time, but may behave differently at runtime because of a different context, separate compilation, type evolution, etc. Are the design choices deeming some instanceof checks to be compilation failures congruent with previous separate compilation and binary compatibility decisions?
21-11-2020
[~briangoetz] Just because a permitted subclass is permitted to subclass it doesn't mean it is trusted to have full access to all internals of the superclass - unlike nestmates. And the superclass certainly has no special access to the subclasses - again unlike nestmates. Yes it is quantitative but I think it is an order-of-magnitude difference. I just don't think we should have any API that can force all subclasses of a type to be loaded as this API permits. The API isn't loading them because there is a need to do so, it is only a side-effect of deciding that the way to describe the set of permitted subclasses is via an array of Class objects. > But given that this precedent has been set, setting yet another precedent for something so similar, in this mess of an API, seems questionable. Hmmm that sounds like a "for consistency" argument ;-) But seriously if we have a new way to do this better then I think we should use that rather than maintain consistency for consistency sake. The real question is: what is the purpose of this API? I think the answer to that should be "to inspect the permittedSubclases attribute of the class", not "to gain access to all the permitted subclasses of the class". That debate aside, the method not only needs to document the failure modes it needs to clearly state exactly how the subclasses will be attempted to be loaded. It should also clarify (which we didn't do with nestmates) that the class is only loaded and not necessarily linked and certainly not initialized.
20-11-2020
[~dholmes] While there is a difference between nestmates and permitted subtypes, I think perhaps "huge" is a "large" overstatement. They _are_ restricted to be in the same module (and, if in the unnamed module, in the same package.) So the assumption of co-maintenance is still present. So the difference seems to be more of a quantitative than qualitative one. I agree that it would have been better to use ClassDesc, if it were present, for nestmates. But given that this precedent has been set, setting yet another precedent for something so similar, in this mess of an API, seems questionable. I agree that documenting the failure modes is a requirement. I guess my question here is: what failure mode or problem are you imagining that is not also present in nest members? (If there's more substantial discussion required, it should probably come back to amber-dev or amber-spec-experts.)
19-11-2020
[~gbierman] thanks for the link to the discussion, though it did not seem to reach any conclusion IMO. [~briangoetz] There is a huge difference with nestmates, as was pointed out in that discussion. Permitted subclasses can be in different packages and while they have a special relationship with the super class, in terms of being allowed to inherit from it, they are not considered to be in the same trust domain the way that nestmates are! While we may have specific views about how we intend to use "permits" within the JDK, it is a broader facility than that, and may be used in ways we did not expect. This API is supposed to reflect on the static attribute of the super class - what it permits as subclasses - not the dynamic runtime presence of any particular subclass. I also agree that getNestMembers() would itself have been better expressed as returning descriptors rather than Class objects - had the option existed then. Trying to load listed subclasses can be the wrong thing to do and I think this has a much higher security exposure than exists for nestmates, or for the original API for which the security review was done. At a minimum the security aspect of this needs to be re-evaluated. Even if it is decided that trying to load and return Class instances is what is desired, this API specification fails to clearly state that such loading will happen or what occurs if loading fails (as pointed out by [~mchung] in the current code review). Using the text of getNestMembers() would be a start, though the failure mode is subtly hidden in that text: >The classes and interfaces which are recorded by H as being members of its nest, and for which H can be determined as their nest host, are indicated by subsequent elements of the returned array. If the class can't be found then it can't be determined that H is the nest host and thus the class does not appear in the returned array. Also note that nestmates requires agreement from the nesthost and the nest member that they are in the same nest - hence you can't just look at the NestMembers attribute to discern that information, but have to examine the nest member itself. Similarly a permitted subclass may not actually extend the base class it is permitted to extend, so further validation of the relationship would be needed. There is a tension between reflecting on the raw attribute stored in the classfile, and reflecting on valid data contained in the attribute.
19-11-2020
[~darcy]Joe: I assume you mean the changes to the narrowing reference conversion? I don't think we have any difference to the current status quo. Currently Java is very flexible with casts - any cast is valid unless we can be very sure that it is pointless. This "pointless" case looks like this: final class A {} class B {} A a; B b = (B)a; // Error All this new relation is doing is saying here is that if A is sealed, then we can look at the permitted subclasses rather than just saying yes and doing the type conversion test at runtime. sealed class A permits X {} final class X extends A {} class B {} A a; B b = (B)a; // Error whereas: sealed class A permits X {} non-sealed class X extends A {} class B {} A a; B b = (B)a; // Ok, let's see... [So all we are doing is spanning the permits list and seeing if it is the "pointless" case, or an accepted one (above), or recurse.] This latter example is the interesting one for binary compatibility. So, what can we change? Whatever we do, the semantic remains the same which is a checkcast from A to B. [This result is dependent on the classes, but that's no different to now.] Perhaps I am misunderstanding your question?
18-11-2020
[~dholmes] David: The discussion regarding the name of this method is in the thread beginning with: http://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-May/002196.html tl;dr: If we are returning an array of `Class` then it should be named using the standard `get` convention as per other related methods. Re: Whether it should return `Class` or `ClassDesc` - That's beyond my expertise. The discussion above converged on this decision, but happy to reopen this.
18-11-2020
[~dholmes] David, originally we did use ClassDesc for the reasons you suggest, though it was not a very deeply informed choice. But it was then argued that all the same concerns are present for `getNestMembers()`, and that there is nothing here that would be any different, and so a consistent treatment with nestmates is both reasonable and desirable. Is there any reason why this would be OK for nest members, but not permitted subtypes? Seems like all the same failure modes are present in each.
18-11-2020
Also note that with regard to renaming permittedSubclasses, we originally started with getPermittedSubclasses and changed it to permittedSubclasses: https://bugs.openjdk.java.net/browse/JDK-8244556?focusedCommentId=14340076&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14340076 Not clear on what feedback that decision is now reversed.
18-11-2020
I have serious concerns about the change to getPermittedSubclasses. As per my response on the review thread: My recollection from earlier discussions here was that the use of ClassDesc was very deliberate as the permitted subclasses may not actually exist and there may be security concerns with returning them!
18-11-2020
Approved revised CSR. [~gbierman], a general question: is there a separate compilation wrinkle in the changed rules around instanceof? In particular, can the check can have different values at compile-time and runtime?
10-11-2020
[~darcy] I thought I had changed the name of java.lang.Class::permittedSubclasses to java.lang.Class::getPermittedSubclasses, but yes I guess we can finalize it already if you are OK with that
05-11-2020
So there were no changes to the proposed updates since the CSR was Provisional? [~vromero] did you mean to Finalize the request rather than moving it back to Proposed?
05-11-2020
@darcy this CSR is intended to cover all the the aspects of the JEP, this iteration won't have any new changes to the JVMS. Also I have updated the name of the method java.lang.Class::permittedSubclasses to java.lang.Class::getPermittedSubclasses
26-10-2020
Moving to Provisional, not Approved. Before the request is Finalized, please clarify which aspect of the JEP this CSR is meant to cover. The Interface Kind only lists Java API, but API and language changes are implied. I assume the permittedSubclasses update will be changed to reflect any changes from the on-going discussion.
25-10-2020