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