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

Tue May 26 04:14:33 UTC 2020

Hi Maurizio,

Right good catch I forgot to add the MONKEY_AT to the list of expected 
tokens. I have fixed that. I have published another iteration at [1]. 
Apart from the MONKEY_AT issue addressed at the parser this iteration also:

  - adds another error key to compiler.properties, this new key is 
oriented to show a more explicit error message when an interface with no 
`non-sealed` or `sealed` modifier extends a sealed interface. A class in 
this position can also be `final` but this is not possible for interfaces.
  - changes to PrintingProcessor and to 
test/langtools/tools/javac/processing/model/element/TestSealed.java 
suggested by Joe in this thread
  - adds a new subtest at SealedCompilationTests testing the sealed and 
non-sealed modifiers being followed by different modifiers or annotations
  - modified a subtest also at SealedCompilationTests, testPrinting, 
that was failing on Windows

Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8227046/webrev.03.delta/

On 5/25/20 6:37 AM, Maurizio Cimadamore wrote:
> 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
>>