RFR: JDK-8242478: compiler implementation for records (Second Preview)

[Vicente Romero]

Hi Maurizio,

Thanks for the review. I have published 3 webrevs [1] is the same as the 
previous review. I just added it again to avoid confusion on what the 
other delta reviews were based on. [2] is implementing a recent change 
in the records spec which forbids assignments to record fields inside of 
the compact constructor. [3] is the patch addressing your review 
comments. The application order is [1, 2, 3]. The latest spec is at [4]. 
There are also some comments inlined below,

On 5/1/20 8:23 AM, Maurizio Cimadamore wrote:
> Hi Vicente,
> good work - some comments below:
>
> * I think Tagir is right - consider code like this:
>
> void m() {
>    enum Foo { A, B };
> }
>
> surely we want this to fail if you compile it w/o --enable-preview ?

right, I just read Tagir's mail too fast :|, good catch @Tagir!

>
> * Check.java
>
> "if (implicitlyStatic || (flags & STATIC) != 0) {"
>
> Don't you already have the "staticOrImplicitlyStatic" var for this?

right done

>
> * Symbol.java
>
> Note that javac already has the capability of tracking if a type is a 
> varargs type - see this code in Type.java
>
> public ArrayType makeVarargs() {
>             return new ArrayType(elemtype, tsym, metadata) {
>                 @Override
>                 public boolean isVarargs() {
>                     return true;
>                 }
>             };
>         }
>
> In other words, from the element type, if you call 'makeVarargs' you 
> will get a special ArrayType whose isVarargs() will return true. I 
> think that could be used to drop the new boolean flag and make the 
> code generally tighter.

I have removed the field and relied on invoking the isVarargs method on 
the type.

>
> * Attr.java
>
> We already have a compiler diagnostic for when you try to override a 
> method with weaker access:
>
> # 0: message segment, 1: set of flag or string
> compiler.err.override.weaker.access=\
>     {0}\n\
>     attempting to assign weaker access privileges; was {1}
>
> I think the wording for the new diagnostic for when you use stronger 
> access could be borrowed from this one? (e.g. take the above 
> diagnostic and replace weaker with stronger?)

sounds good, done

>
> * LocalStaticDeclaration
>
> Since you are there, you might want to also add in the template some 
> missing cases:
>
> * instance initializer
> * constructor
> * static method
>
> I know some of the underlying code is shared - but better be safe than 
> sorry.

yep added.

>
>
> Cheers
> Maurizio
>
>
Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8242478/webrev.01/
[2] 
http://cr.openjdk.java.net/~vromero/8242478/webrev.01.forbid.field.assigment/
[3] http://cr.openjdk.java.net/~vromero/8242478/webrev.01.review.iteration/
[4] 
http://cr.openjdk.java.net/~gbierman/records2/20200501/specs/records-jls.html
> On 30/04/2020 16:10, Vicente Romero wrote:
>> Hi all,
>>
>> Please review the patch for the second preview of records at [1], the 
>> bug entry is at [2] the related spec can be found at [3], the JEP is 
>> at [4]. This patch implements the following changes:
>>
>> - local enums and interfaces, along with local records
>> - removed possibility of final modifier for record components
>> - removed requirement that canonical constructor must be public. Any 
>> access modifier must provide at least as much access as the record 
>> class. If a canonical constructor is implicitly declared, then its 
>> access modifier is the same as the record class
>> - check that there is a iff relation between a varargs record 
>> component and the corresponding parameter in the canonical 
>> constructor so:
>>
>>    record R(int... i) { R(int... i) {...}} is allowed but:
>>    record R(int... i) { R(int[] i) {...}} is not
>>
>> - new case for using @Override annotation to declare that a method is 
>> an accessor method for a record component
>>
>> These are the main changes, plus:
>> - there is one liner change to java.io.ObjectStreamClass, this is 
>> related to the fact that now the canonical record don't necessarily 
>> has to be public thus the use of Class::getDeclaredConstructor
>> - there is a one liner change concerning the flags of the generated 
>> toString method, this is a bug fix, toString didn't have the same 
>> flags as hashCode and equals
>> - I have added several tests to enforce other rules in the spec like 
>> the one saying that in a compact constructor fields have to be 
>> initialized in the same order in which the corresponding record 
>> component has been declared, plus some additional tests
>> - test RecordCompilationTests is executed twice now, first in a mode 
>> that is equivalent to the original test and then using an "empty" 
>> annotation processor. This is because some regressions have been 
>> discovered in the past when an annotation processor is present. I 
>> have also made some changes to the libraries this test uses.
>>
>> Thanks,
>> Vicente
>>
>> [1] http://cr.openjdk.java.net/~vromero/8242478/webrev.00
>> [2] https://bugs.openjdk.java.net/browse/JDK-8242478
>> [3] 
>> http://cr.openjdk.java.net/~gbierman/records2/20200428/specs/records-jls.html
>> [4] https://bugs.openjdk.java.net/browse/JDK-8242303