Fwd: Re: RFR: JEP 359-Records: compiler code
Jan Lahoda
jan.lahoda at oracle.com
Fri Nov 1 13:12:13 UTC 2019
On 01. 11. 19 13:56, Maurizio Cimadamore wrote:
> Also, while experimenting, I noted that compiling code like:
>
> record Foo(String s) { }
>
> Without the --enable-preview flag, causes a spurious parser error, with
> no clue that I'm using a preview feature:
>
> Test.java:1: error: class, interface, or enum expected
> record Foo(String s) {
> ^
> 1 error
>
> Is that expected? I believe the issue is with 'isRecordToken' which
> always returns 'false' if preview features are disabled. I think
> something should call checkSourceLevel(Feature.RECORDS), so that preview
> errors for records will be triggered.
I think I was touching that in one of my comments. I am afraid it is
probably not so simple - we could do the checks easily for top-level
records, but not so easily for nested records. I.e.:
---
class X {
record R(int i) {
return null;
}
}
class record {}
---
is valid currently ("R" is a method which returns "record"), but not
with the patch:
---
$ javac /tmp/R.java
/tmp/R.java:2: error: records are a preview feature and are disabled by
default.
record R(int i) {
^
(use --enable-preview to enable records)
/tmp/R.java:3: error: illegal start of type
return null;
^
/tmp/R.java:6: warning: 'record' may become a restricted type name in a
future release and may be unusable for type declarations or as the
element type of an array
class record {}
^
2 errors
1 warning
---
I suspect it may be necessary to only add a warning/note for the
suspected records, with a possible special-case for top-level records
failing with a more appropriate error.
Jan
>
> And, looking more, I see some issues with parsing inside blocks - for
> instance, this program no longer compiles (w/o enable-preview):
>
> class Foo {
> void test() {
> final record r = null;
> }
> }
>
> class record {}
>
> That's because when we're inside a method body, we always interpret
> 'final record' as a record declaration.
>
> A better solution would be to always check (with lookahead) for "record
> IDENT '('". That should be more robust.
>
>
> In other words, I think that the parser code should be consolidated so
> that you have a single routine that checks for the start of a record
> declaration:
>
> record IDENT '('
>
> If the above production is matched (with lookahead), then you call
> checkSourceLevel(Feature.RECORDS), so that the correct errors are
> generated.
>
> The routine could be something like this:
>
> boolean isRecordStart() {
> if (token.kind == IDENTIFIER && token.name() == names.record &&
> (peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
> peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
> checkSourceLevel(Feature.RECORDS);
> return true;
> } else {
> return false;
> }
> }
>
> Then you remove isRecordToken, and start to use the above everywhere. I
> tried with a crude patch (see below), and things seem to work much
> better. Examples:
>
> record foo(int x) {}
>
> gives:
>
> Test.java:1: error: records are a preview feature and are disabled by
> default.
> record foo(int x) {}
> ^
> (use --enable-preview to enable records)
> 1 error
>
> And, this:
>
> class Foo {
> void test() {
> final record r = null;
> }
> }
>
> class record {}
>
> Gives the following:
>
> Test.java:3: warning: 'record' may become a restricted type name in a
> future release and may be unusable for type declarations or as the
> element type of an array
> final record r = null;
> ^
> Test.java:7: warning: 'record' may become a restricted type name in a
> future release and may be unusable for type declarations or as the
> element type of an array
> class record {}
> ^
> 2 warnings
>
>
> Which I think it's closer to what we want.
>
> Patch below:
>
> diff -r 1da4a8addd21
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java
> ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.javaThu
> Oct 31 23:59:56 2019 -0400
> +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.javaFri
> Nov 01 12:53:31 2019 +0000
> @@ -2558,7 +2558,7 @@
> if (token.kind == INTERFACE ||
> token.kind == CLASS ||
> token.kind == ENUM ||
> - (token.kind == IDENTIFIER && token.name() ==
> names.record) ) {
> + isRecordStart()) {
> return
> List.of(classOrRecordOrInterfaceOrEnumDeclaration(mods, dc));
> } else {
> JCExpression t = parseType(true);
> @@ -2625,9 +2625,7 @@
> //else intentional fall-through
> }
> }
> - if (isRecordToken() &&
> - (peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
> - peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
> + if (isRecordStart()) {
> dc = token.comment(CommentStyle.JAVADOC);
> return List.of(recordDeclaration(F.at(pos).Modifiers(0),
> dc));
> } else {
> @@ -3661,7 +3659,7 @@
> protected JCStatement
> classOrRecordOrInterfaceOrEnumDeclaration(JCModifiers mods, Comment dc) {
> if (token.kind == CLASS) {
> return classDeclaration(mods, dc);
> - } if (isRecordToken()) {
> + } if (isRecordStart()) {
> return recordDeclaration(mods, dc);
> } else if (token.kind == INTERFACE) {
> return interfaceDeclaration(mods, dc);
> @@ -4022,7 +4020,7 @@
> int pos = token.pos;
> JCModifiers mods = modifiersOpt();
> if (token.kind == CLASS ||
> - isRecordToken() ||
> + isRecordStart() ||
> token.kind == INTERFACE ||
> token.kind == ENUM) {
> return
> List.of(classOrRecordOrInterfaceOrEnumDeclaration(mods, dc));
> @@ -4117,9 +4115,16 @@
> }
> }
>
> - boolean isRecordToken() {
> - return allowRecords && token.kind == IDENTIFIER && token.name()
> == names.record;
> - }
> + boolean isRecordStart() {
> + if (token.kind == IDENTIFIER && token.name() == names.record &&
> + (peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
> + peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
> + checkSourceLevel(Feature.RECORDS);
> + return true;
> + } else {
> + return false;
> + }
> +}
>
> /** MethodDeclaratorRest =
> * FormalParameters BracketsOpt [THROWS TypeList] (
> MethodBody | [DEFAULT AnnotationValue] ";")
>
>
> Cheers
> Maurizio
>
More information about the amber-dev
mailing list