Fwd: Re: RFR: JEP 359-Records: compiler code
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Nov 1 13:18:44 UTC 2019
On 01/11/2019 13:12, Jan Lahoda wrote:
> 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:
Good point. So, I think the patch fixes the treatment of toplevel and
local records. Where it leaves something to be desired, as you point
out, is with member records (e.g. records nested inside a class
declaration). Here the grammar is inherently ambiguous with method
declarations, so not much we can do (we can probably do something if
there's a generic record though). In this cases we should just take the
record production or the method one, depending on whether preview
features are enabled or not.
Maurizio
> ---
> $ 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