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

[Maurizio Cimadamore]

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 ?

* Check.java

"if (implicitlyStatic || (flags & STATIC) != 0) {"

Don't you already have the "staticOrImplicitlyStatic" var for this?

* 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.

* 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?)

* 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.


Cheers
Maurizio


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