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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Sat Nov 30 22:51:25 UTC 2019


Looks good - but you need diagnostic fragments for the "canonical", 
"compact" diagnostic bits (no need for re-review).

Maurizio

On 30/11/2019 18:29, Vicente Romero wrote:
> 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/
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191130/280d2d98/attachment.html>


More information about the compiler-dev mailing list