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

[Vicente Romero]

Hi,

I have created another iteration at [1], this is just a delta from last 
iteration [2]. Some comments below

[1] 
http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01_delta.01/
[2] http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01

On 11/28/19 1:35 PM, Maurizio Cimadamore wrote:
>
> 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"
>

yes I have modified the code so that the error messages can show 
"canonical" or "compact" depending on the case

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

yes this is a bug, we are considering updating both the spec and the 
compiler. I will submit another iteration as soon as this change is 
reflected in both

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

will do that later, I remember that there were some interactions between 
these flags, they are not exclusive
>
> * 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.
>

I have created a follow up issue: 
https://bugs.openjdk.java.net/browse/JDK-8235149

> * Are the changes in Tokens really needed?
>
no removed
>
> * Check::checkUnique doesn't seem to use the added 'env' parameter - 
> changes should be reverted
>
yep done
>
> * 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).
>

right done, I have created a method that check if a record component 
name matches with a parameterless method in Object

> Also, the serialization names are not needed (although I guess they 
> will come back at some point).
>

yes they will, I can remove them now but I guess we will need them once 
we implement a lint warning

> And, not sure what "get" and "set" are needed for?
>

removed
>
>
> Maurizio
>

thanks,
Vicente
>
> 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/
>>>
>>