RFR: JEP 359: Records (Preview) (full code)

Thu Nov 28 18:35:26 UTC 2019

Hi Vicente,
generally looks good - few comments below; I tried to focus on areas 
where the compiler code seemed to diverge from the spec, as well on 
pieces of code which look a leftover from previous spec rounds.

* canonical constructors *can* have return statements - compact 
constructors cant; the spec went a bit back and forth on this, but now 
it has settled. Since compact constructors are turned into ordinary 
canonical ones by the parser, I think you need to add an extra check for 
COMPACT_RECORD_CONSTRUCTOR; in other words, this is ok:

record Foo() {
    Foo() { return; } //canonical
}

this isn't

record Foo() {
    Foo { return; } //compact
}

but the compiler rejects both. This probably means tweaking the 
diagnostic a bit to say "a compact constructor must not contain |return| 
statements"

* in general, all diagnostics speak about 'canonical constructor' 
regardless of whether the constructor is canonical of compact; while I 
understand the reason behind what we get now, some of the error messages 
can be confusing, especially if you look at the spec, where canonical 
constructor and compact constructor are two different concepts. This 
should be fixed (even if not immediately, in which case I'd recommend to 
file a JBS issue to track that)

* static accessors are allowed - this means that I can do this:

record Foo(int x) {
public static int x() {return 0; }

public static void main(String[] args) {
    System.err.println(new Foo(42).x());
}
}

This will compile and print 0. The classfile will contain the following 
members:

final class Foo extends java.lang.Record {
   public Foo(int);
   public static int x();
   public static void main(java.lang.String[]);
   public java.lang.String toString();
   public final int hashCode();
   public final boolean equals(java.lang.Object);
}

I believe this is an issue in the compiler, but also in the latest spec 
draft, if I'm not mistaken.

* [optional - style] the env.info.isSerializableLambda could become an 
enum { NONE, LAMBDA, SERIALIZABLE_LAMBDA } instead of two boolean parameters

* this code is still rejected with --enable-preview _disabled_:

class X {
     record R(int i) {
         return null;
     }
}
class record {}

This gives the error:

Error:
|  records are a preview feature and are disabled by default.
|    (use --enable-preview to enable records)
|  record R(int i) { return null } }
|  ^
|  Error:
|  illegal start of type
|  record R(int i) { return null } }
|                    ^

In other words, the parsing logic for members is too aggressive - we 
shouldn't call isRecordStart() in there. If this is not fixed in this 
round, we should keep track with a JBS issue.

* Are the changes in Tokens really needed?

* Check::checkUnique doesn't seem to use the added 'env' parameter - 
changes should be reverted

* Names.jave - the logic for having forbiddenRecordComponentNames could 
use some refresh - in the latest spec we basically have to ban 
components that have same name as j.l.Object members - so I think we can 
implement the check more directly (e.g. w/o having a set of names). 
Also, the serialization names are not needed (although I guess they will 
come back at some point). And, not sure what "get" and "set" are needed for?

Maurizio

On 28/11/2019 16:05, Vicente Romero wrote:
> Hi again,
>
> Sorry but I realized that I forgot to remove some code on the compiler 
> side. The code removed is small, before we were issuing an error if 
> some serialization methods were declared as record members. That 
> section was removed from the spec. I have prepared another iteration 
> with this change at [1]
>
> Thanks,
> Vicente
>
> [1] 
> http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01/
>
> On 11/27/19 11:37 PM, Vicente Romero wrote:
>> Hi,
>>
>> Please review the code for the records feature at [1]. This webrev 
>> includes all: APIs, runtime, compiler, serialization, javadoc, and 
>> more! Must of the code has been reviewed but there have been some 
>> changes since reviewers saw it. Also this is the first time an 
>> integral webrev is sent out for review. Last changes on top of my 
>> mind since last review iterations:
>>
>> On the compiler implementation:
>> - it has been adapted to the last version of the language spec [2], 
>> as a reference the JVM spec is at [3]. This implied some changes in 
>> determining if a user defined constructor is the canonical or not. 
>> Now if a constructor is override-equivalent to a signature derived 
>> from the record components, then it is considered the canonical 
>> constructor. And any canonical constructor should satisfy a set of 
>> restrictions, see section 8.10.4 Record Constructor Declarations of 
>> the specification.
>> - It was also added a check to make sure that accessors are not generic.
>> - And that the canonical constructor, if user defined, is not 
>> explicitly invoking any other constructor.
>> - The list of forbidden record component names has also been updated.
>> - new error messages have been added
>>
>> APIs:
>> - there have been some API editing in java.lang.Record, 
>> java.lang.runtime.ObjectMethods and 
>> java.lang.reflect.RecordComponent, java.io.ObjectInputStream, 
>> javax.lang.model (some visitors were added)
>>
>> On the JVM implementation:
>> - some logging capabilities have been added to classFileParser.cpp to 
>> provide the reason for which the Record attribute has been ignored
>>
>> Reflection:
>> - there are several new changes to the implementation of 
>> java.lang.reflect.RecordComponent apart from the spec changes 
>> mentioned before.
>>
>> bug fixes in
>> - compiler
>> - serialization,
>> - JVM, etc
>>
>> As a reference the last iteration of the previous reviews can be 
>> found at [4] under folders: compiler, hotspot_runtime, javadoc, 
>> reflection and serialization,
>>
>> TIA,
>> Vicente
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.00/
>> [2] 
>> http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jls.html
>> [3] 
>> http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jvms.html
>> [4] http://cr.openjdk.java.net/~vromero/records.review/
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191128/ae4c196c/attachment-0001.html>