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

[Maurizio Cimadamore]

Looks good - a minor nit is that we have a golden file test for checking 
that enums in methods are not allowed, but we don't for other static 
constructs.

Maurizio

On 03/05/2020 21:57, Vicente Romero wrote:
> 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
>