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

Mon May 4 19:53:06 UTC 2020

Hi Jan,

Thanks for your comments, I have created a small webrev addressing them. 
Please check it at [5]

Vicente

[5] http://cr.openjdk.java.net/~vromero/8242478/webrev.02.jan.comments/

On 5/4/20 10:21 AM, Jan Lahoda wrote:
> To me, looks reasonable. Some minor nits:
> -in JavacParser there is:
> +                (allowRecords && !isRecordStart() || !allowRecords)) {
>
> I would find it more understandable, if it was written as:
> +                (!allowRecords || !isRecordStart())) {
>
> -in tests like IllegalAnnotation.java, I would suggest to add an 
> action that will run the test with the new semantics:
> * @compile --enable-preview -source ${jdk.version} IllegalAnnotation.java
>
> This should show both the behaviors, old and new, and it will be 
> easier to find an adjust these tests when records go public (because 
> then the test will start to fail, as it uses the newest source level. 
> But then having two @compile actions would be good - one with an older 
> source level and failing, and the other one with a new source level, 
> passing).
>
> Jan
>
> On 03. 05. 20 22: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
>>