RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes

Mon May 25 10:37:52 UTC 2020

Hi,
changes look good, but the parser changes do not convince me. Now that 
the logic is more clearly defined, I see some issues e.g. there seems to 
be a tight coupling by where "sealed" or "non-sealed" is, and the fact 
that the following token must be e.g. "class". This is unlike other 
modifiers which can in fact appear in any configuation.

I believe that you can break the current logic by adding an extra 
annotation between the "sealed" token and the "class" token e.g.

sealed @foo class Bar { }

But maybe the quick fix would be to add MONKEY_AT to the set of expected 
tokens after sealed/non-sealed.

Maurizio

On 22/05/2020 19:25, Vicente Romero wrote:
> Hi,
>
> Thanks Jan and Maurizio for the reviews. I have created another 
> iteration that I hope addresses all of the comments. I recognize that 
> dealing with `sealed`, `non-sealed` is the most complicated part as 
> there is no guide right now from the spec. So I have tried to make 
> them contextual keywords, for some informal definition of the concept, 
> I think that with more success for the `sealed` case. So going in more 
> detail this iteration includes:
>
>  - ClassTree::getPermitsClause now returns `List.of()`
>  - Types::findDescriptorInternal has been modified to fail on sealed 
> interfaces
>  - several error messages have been updated as suggested, on this 
> front given that when a class list the same interface twice in the 
> implements clause, the second occurrence is the one flagged, I did the 
> same for repeated names in the permits clause
>  - modifications in ClassReader and ClassWriter as suggested
>  - JavacParser, the bulk of the changes are concentrated here, as 
> mentioned above the goal has been to try to implement the contextual 
> keyword thing and also give a nice error message in several corner 
> case situations detected in the reviews
>  - more tests were added
>
> Thanks,
> Vicente
>
> On 5/21/20 8:14 AM, Maurizio Cimadamore wrote:
>> Hi Vicente,
>> looks very good. Some comments below.
>>
>> * the parser logic is clever in its use of position to apply 
>> context-dependent keyword detection; as Jan says, perhaps just share 
>> the code so that the position checks are not repeated.
>>
>> * I found one very edge-case quirk in the context-dependent logic; 
>> not sure how we wanna address:
>>
>> class Foo {
>>     sealed m() {}
>> }
>>
>> This fails with:
>>
>> Error: invalid method declaration; return type required
>>
>> As javac parses non-sealed as a modifier, and then expects a type. I 
>> think this is probably reasonable, but it's not as context-dependent 
>> as it could be I guess.
>>
>> * This case:
>>
>> class Bar { }
>> sealed @interface Foo permits Bar
>>
>> Fails as expected, but only because Bar doesn't extends Foo. I 
>> believe we'd like to ban sealed on annotations more eagerly. Same for 
>> non-sealed. For enums and records (which are non-extensible) the 
>> compiler does the right thing and tells me that I can't just use 
>> sealed/non-sealed there.
>>
>> * The recovery logic in case preview features aren't enabled leaves 
>> something to be desired. For instance, if I compile this w/o 
>> --enable-preview:
>>
>>  record Foo() {}
>>
>> I get a very sensible error:
>>
>> records are a preview feature and are disabled by default.
>>     (use --enable-preview to enable records)
>>
>> However, if I compiler this w/o --enable-preview:
>>
>> sealed class Foo {}
>>
>> I get this:
>>
>> error: class, interface, or enum expected
>>
>> (no mention of preview features)
>>
>> It gets worse if I also specify a `permits`.
>>
>> * As Jan mentioned, type parameters on permitted types should be 
>> banned, not silently cleared in TypeEnter
>>
>> * Overall the type enter logic seems robust - I've tried several 
>> examples swapping superclass/subclass - using references to nested 
>> classes in supertype declaration, and it all works. Well done.
>>
>> * The error for lambda expressions leaves to be desired:
>>
>> sealed interface Action {
>>  void doAction();
>> }
>>
>> class Test {
>>   Action a = () -> { };
>> }
>>
>> Foo.java:6: error: class is not allowed to extend sealed class: Action
>>   Action a = () -> { };
>>              ^
>>
>> I think a dedicated error here would be useful.
>>
>>
>> * the same check is not applied to method references:
>>
>>
>> class Test {
>>
>>   Action a2 = Test::m; //no error
>>
>>   static void m() { }
>> }
>>
>> More generally, if a functional interface cannot be sealed, I think 
>> it would be better to inject the check in the functional interface 
>> check (e.g. Types::findDescriptorInternal) so that you won't need any 
>> extra code in Attr. This would also be more in spirit with the spec, 
>> where the non-sealedness check is defined in 9.8, not in section 15.
>>
>> * Pulling more on that string, the @FunctionalInterface annotation 
>> can be placed on a sealed interface and no error is issued
>>
>> * On ClassWriter - isn't calling adjustFlags() enough? That will 
>> truncate the long flags into an int - I think Flags.SEALED is bigger 
>> than that?
>>
>>
>> // error messages
>>
>> * DuplicateTypesInPermits
>>
>> I suggest:
>>
>> test/langtools/tools/javac/diags/examples/DuplicateTypeInPermits.java:30: 
>> error: invalid permits clause
>> sealed class Sealed permits Sub, Sub {}
>>                             ^
>>   (repeated type: Sub)
>>
>> [this is consistent with the error we issues in other places - e.g. 
>> when you implements same interface twice]
>>
>> * NonSealedWithNoSealedSuper
>>
>> test/langtools/tools/javac/diags/examples/NonSealedWithNoSealedSuper.java:31: 
>> error: non-sealed modifier not allowed here
>> non-sealed class Sub extends C {}
>>            ^
>>   (class must have a sealed superclasses)
>>
>> I suggest to replace the details message with something like this:
>>
>> (class C does not have any sealed supertypes)
>>
>> [since I expect this message to be applicable also for superinterfaces]
>>
>> * PermitsCantListDeclaringClass
>>
>> test/langtools/tools/javac/diags/examples/PermitsCantListDeclaringClass.java:30: 
>> error: invalid permits clause
>> sealed class C permits C {}
>>                        ^
>>   (must not include the declaring class: C)
>>
>> Here I recommend something like:
>>
>> (illegal self-reference in permits clause)
>>
>> * PermitsCantListSuperType
>>
>> test/langtools/tools/javac/diags/examples/PermitsCantListSuperType.java:32: 
>> error: invalid permits clause
>> sealed class C implements I permits I {}
>>                                     ^
>>   (must not include a supertype: I)
>>
>> I suggest:
>>
>> (illegal reference to supertype I)
>>
>> * PermitsInNoSealedClass
>>
>> test/langtools/tools/javac/diags/examples/PermitsInNoSealedClass.java:30: 
>> error: invalid permits clause
>> class C permits Sub {}
>>         ^
>>   (class must be sealed)
>>
>> This is good, but I noted that if you change the test to use an 
>> interface, the message still says "class" - the kindname should be 
>> used here.
>>
>> * SealedMustHaveSubtypes
>>
>> test/langtools/tools/javac/diags/examples/SealedMustHaveSubtypes.java:29: 
>> error: sealed class must have subclasses
>> sealed class Sealed {}
>>        ^
>>
>> I think this message reflects one of the main issues with the general 
>> type vs. class dichotomy. A subclass, in JLS lingo is e.g. `B` where 
>> `B extends A`. Interfaces do not play in the mix - they are not 
>> considered subclasses. The word subtypes could be more general - but 
>> again, it is a bit imprecise, since we're talking about declarations 
>> here, not types. I'll defer this conundrum to our spec gurus :-)
>>
>>
>> Cheers
>> Maurizio
>>
>>
>>
>> On 18/05/2020 23:42, Vicente Romero wrote:
>>> Hi,
>>>
>>> Please review this patch for the compiler, javadoc and 
>>> javax.lang.model support for the JEP 360 Sealed Classes (Preview). 
>>> The changes are provided at [1], which implements the latest JLS for 
>>> sealed types [2]. The patch also include the needed changes to 
>>> javadoc and javax.lang.model to support sealed types. The CSR witht 
>>> the changes in the javax.lang.model spec is at [3]. The sealed types 
>>> JEP is accessible at [4]. There is an ongoing review for the VM and 
>>> core-libs code of sealed types [5] and that code hasn't been 
>>> included in this webrev,
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] http://cr.openjdk.java.net/~vromero/8227046/webrev.00/
>>> [2] 
>>> http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html 
>>>
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8244367
>>> [4] https://openjdk.java.net/jeps/360
>>> [5] 
>>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066440.html
>