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

[Jan Lahoda]

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
>