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

Fri May 22 18:25:12 UTC 2020

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