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
