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

[Maurizio Cimadamore]

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