RFR: JEP 359-Records: compiler code

Tue Nov 12 09:00:02 UTC 2019

I also took another look at tests - it seems to me that we're a bit thin 
in this department, I'll try to explain why.

* We need at least a combo test for the parser which tries records with 
different source levels and in different source positions (top-level, 
member, local), since we found several complex issues in this part of 
the parser

* I noted that, while we have many diagnostics tests, we have basically 
zero negative tests to check that the compiler is generating the desired 
error message _without any additional_ spurious errors. In principle, 
there should be at least one negative test for each record-related error 
message. This leads to replication with the diagnostics tests, but we've 
always done it this way. The diagnostic framework is for diagnostics - 
per se, it is insufficient to verify good quality of compiler output.

* I suggest maybe to group tests in different folders depending on the 
'category' they fall in. E.g. I see you have a reflection test and a 
serialization test, then some annotation processing tests - it would be 
nice to label these accordingly by putting them in separate folders.

* there's no test for checking what happens when javac is fed a 'bad 
record classfile attribute' (again a combo here is probably the right 
choice, given the number of ways the attribute can be wrong)

* I see no tests for constructor delegation to canonical constructor - 
again, here we should try different things, with different level of 
delegation (e.g. C1 -> CANONICAL, C1 -> C2 -> CANONICAL and so forth) to 
make sure javac is not tripped up by the indirections

* Probably DA/DU rules for compact constructor are, again, complex 
enough that when you start reasoning about 2-3 different record 
components, the number of different states go up pretty rapidly, and 
again a combo could be useful (completes normally, completes abruptly, 
explicitly assigns to x, implicitly assigns to x, ...).

* OriginTest seems a bit weak in that it tests _all implicit_ and _all 
explicit_ and nothing in between; maybe that's part of the test design, 
but looks odd

Maurizio

On 12/11/2019 08:42, Maurizio Cimadamore wrote:
> 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
>>>
>>