RFR: JEP 359-Records: compiler code

[Maurizio Cimadamore]

The compiler changes look good - I still see indentiation issues in 
TypeEnter, especially in lines 1225-1238.

Also, in JavacParser:

* in  blockStatements we do this:

  if (isRecordStart() && allowRecords) {
2629             dc = token.comment(CommentStyle.JAVADOC);
2630             return 
List.of(recordDeclaration(F.at(pos).Modifiers(0), dc));

But isRecordStart() can generate diagnostics - so should it be better to 
invert the condition, as in:

if (allowRecords && isRecordStart()) - to avoid spurious preview 
warnings on source < 14?

* how does this code avoids the problem described well by Jan in a 
previous email in this thread - reporting below:

> 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; 

Seems to me that we are using isRecordStarts in a member position - 
meaning that a member record would be parsed even if it's just a method 
returning a 'record' type (which is allowed, with warning).

Maybe gate the isRecordStart with allowRecords? Actually, maybe we 
should add  ' && allowRecords' check inside isRecordStart() all the 
time, to make sure we don't pick up more records than intended?

Maurizio


On 12/11/2019 00:31, Vicente Romero wrote:
> Hi all,
>
> I have uploaded another iteration to [3]. This iteration fixes a 
> regression introduced with one of the fixes that were addressing 
> preview review comments. In short this is the most important change:
>
> diff -r 5d6410bfafe0 -r 806e05ae98e3 
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
> --- 
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java 
> Sun Nov 10 19:41:49 2019 +0000
> +++ 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java 
> Mon Nov 11 23:42:59 2019 +0000
> @@ -1217,8 +1217,11 @@
>                  if (sym.owner.owner.kind == PCK ||
>                      (sym.owner.flags_field & STATIC) != 0)
>                      mask |= STATIC;
> -                else if ((flags & ENUM) != 0)
> +                else if ((flags & ENUM) != 0) {
>                      log.error(pos, Errors.EnumsMustBeStatic);
> +                } else if ((flags & RECORD) != 0) {
> +                    log.error(pos, 
> Errors.RecordDeclarationNotAllowedInInnerClasses);
> +                }
>                  // Nested interfaces and enums are always STATIC 
> (Spec ???)
>                  if ((flags & (INTERFACE | ENUM | RECORD)) != 0 ) 
> implicit = STATIC;
>              } else {
>
> The regression introduced was that code like:
>
> class Outer {
>     class Inner {
>         record R() {}
>     }
> }
>
> started being accepted by the compiler. The reason is because the code 
> that checks for static declarations inside inner classes in the 
> compiler, checks for declared modifiers, but records are `implicitly` 
> static as enums are. For this reason I'm now doing the check for 
> records inside inner classes in the same place where a similar check 
> for enums is done at Check::checkFlags. I also introduced a new error 
> message, please see it inserted into the webrev.
>
> Thanks,
> Vicente
>
> [3] http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.06
>
> On 11/10/19 6:48 AM, Vicente Romero wrote:
>> Hi,
>>
>> I have uploaded another iteration to [2]. This one includes some 
>> missing type and annotation javax.lang.model visitors,
>>
>> Thanks,
>> Vicente
>>
>> [2] 
>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.05
>>
>> On 11/8/19 5:02 PM, Vicente Romero wrote:
>>> Hi all,
>>>
>>> Please review another iteration of the compiler implementation for 
>>> records at [1]. New in this iteration:
>>>
>>> - I added an @implSpec comment to method: 
>>> javax.lang.model.util.Elements::recordComponentFor
>>> - moved the list of forbidden record component names to class Names
>>> - added the code proposed by Maurizio to the parser with a small 
>>> tweak for records declared as members
>>> - modified the error message to indicate that any non-canonical 
>>> constructor should invoke another constructor to: "constructor is 
>>> not canonical, so its first statement must invoke another constructor"
>>> - added method: 
>>> com.sun.tools.sjavac.comp.PubapiVisitor::visitRecordComponent
>>> - added new visitor: javax.lang.model.util.AbstractTypeVisitor14
>>> - moved annotation processing tests for records to common place for 
>>> other annotation processing tests
>>>
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] 
>>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.04
>>
>