Hi Vicente,
javac changes look overall OK to me.
A few comments:
-the handling of non-sealed in JavacParser is fairly good, even though I
wonder if handling it in the tokenizer would not be better. If I read
the specification correctly, "non-sealed" is specified as a keyword, so
the tokenizer would seem like a proper place to recognize it (when
preview is enabled, etc.). So, I think this:
class NonSealed {
{
int non = 0;
int sealed = 0;
int c = non-sealed;
}
}
should not compile, but it does. In any case, not sure if there are
tests for broken/strange non-sealed, like "non -sealed",
"non/**/-sealed", "non\u002Dsealed".
Also, the checks for 'non', '-', 'sealed' tokens is duplicated twice, it
would be nicer to have this slightly complex code only once.
-parsing of permitted classes allows parameterized types, while it, I
guess, should not. I.e. it appears javac compiles this without producing
compile-time errors:
public sealed class Sealed1 permits I<String> {
}
final class I<T> extends Sealed1 {}
-in ClassReader:
new AttributeReader(names.PermittedSubclasses, V58, CLASS_ATTRIBUTE)
that should use V59?
-I appreciate the error recovery for using "sealed/non-sealed" for local
classes, but I wonder if there's something we could do when "sealed" is
used without --enable-preview (for local and non-local
classes/interfaces). Maybe if "sealed" is followed by another modifier,
'@', 'class' or 'interface', we can produce a better errors? (But we can
work on that later.)
-regarding TypeElement#getPermittedSubclasses: it currently returns
List<? extends TypeMirror>; I wonder if returning List<? extends
TypeElement> would be better?
-not sure if I prefer the "invalid permits clause" errors like:
---
DuplicateTypeInPermits.java:30: error: invalid permits clause
sealed class Sealed permits Sub, Sub {}
^
(must not contain duplicates: Sub)
---
something like:
---
DuplicateTypeInPermits.java:30: error: repeated permitted type
sealed class Sealed permits Sub, Sub {}
^
---
might work as well.
-nit: ClassTree#getPermitsClause() could use List.of(), instead of
Collections.emptyList();
Thanks,
Jan
On 19. 05. 20 0: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
