Fwd: Re: RFR: JEP 359-Records: compiler code

Fri Nov 1 13:03:56 UTC 2019

Hi Maurizio,

On 11/1/19 8:06 AM, Maurizio Cimadamore wrote:
>
> First, congrats on the work on error messages - it really paid off; 
> the diagnostics now look very consistent and easy to grasp.
>

good :)

> One minor not on that side is this message:
>
> "first statement of this constructor must invoke another constructor"
>
yep I wasn't happy with it either
>
> This has two problems, I think:
>
> 1) "statement .... must invoke" - a statement does not invoke; maybe 
> rephrase as "first statement ... must be a constructor invocation"
>
> 2) we don't have a blessed term to name a non-canonical constructor. 
> Can we just say non-canonical in the diagnostic? E.g.
>
> "the first statement in a non-canonical constructor must be an 
> invocation of another constructor"
>
> Seems a mouthful.
>
> I noted that we have this diagnostic around:
>
> # 0: name
>   230 compiler.err.call.must.be.first.stmt.in.ctor=\
>   231     call to {0} must be first statement in constructor
>
> Perhaps, inspired by this, we can turn this around and do this:
>
> "First statement in non-canonical constructor must be a call to 'this'" ?
>

will do

> This seems more compact.
>
>
> More comments inline below:
>
>
> On 01/11/2019 04:11, Vicente Romero wrote:
>> Hi all,
>>
>> Thanks for all the feedback so far. I have published another 
>> iteration at [1]. New in this iteration:
>>
>> Flags.java:
>>
>> - I have sync the javadoc and the comment on the RECORD flag
>> - renamed MemberRecordClassFlags -> MemberRecordFlags
>>
>> com.sun.tools.javac.code.Kinds.java:
>> - in order to generate better error messages I have added two new 
>> kinds: RECORD_COMPONENT and RECORD
> Great - this works very well
>>
>> Symbols.java
>> - moved method isRecord to ClassSymbol for the rest of the symbols 
>> moved back to using the bitwise test
>> - removed method ::isDynamic from MethodSymbol
> Good
>>
>> Attr.java
>> - implemented a spec change to relax that constraint that every 
>> non-canonical constructor should call the canonical constructor. Now 
>> the spec forces every non-canonical constructor to invoke a different 
>> constructor as its first statement. This actually implied removing 
>> some code.
>
> The new check looks very good - but I think this comment:
>
> /* this is it for now we still have to verify that the invocation is 
> actually pointing to
> +                     * the canonical constructor as it could be 
> pointing to another constructor, but we need to
> +                     * attribute the arguments first in visitApply
> +                     */
>
> Is no longer valid?
>
yep, removed
>
>
>> Check.java
>> - at method ::duplicateError, there is new code to avoid generating 
>> two errors when the user declares a record with duplicated record 
>> component names. If this happens, then a compiler generated canonical 
>> constructor would have duplicated parameters. This code is preventing 
>> the compiler to generate a second error referring to a method that is 
>> not visible to the user. Another option is to not generate the 
>> canonical constructor at all if there are duplicate record components
>> - at method ::checkUnique, there is new code to improve the error 
>> message when the user defines a constructor with the same erasure as 
>> the canonical constructor. Still the error message is not perfect 
>> because it shows in both cases the erasure of the methods, so they 
>> seem to be the same. But this is not related to records code, that's 
>> a simplification that happens at the moment of reporting the error.
>
> I really don't get why the message shows the erasure of the methods - 
> you are calling:
>
> Errors.ConstructorWithSameErasureAsCanonical(other, canonical)
>
> That is, you are passing symbols - the symbols contain full type info 
> - why is this being erased? This doesn't seem to happen e.g. in 
> ordinary clash errors, e.g.
>
> InheritanceConflict3.java:14:10: compiler.err.name.clash.same.erasure: 
> f(java.lang.Object), f(T)
>
> Is it possible, by any chance that we're generating the canonical 
> symbol with an erased type?
>
>>
>> TypeEnter
>> - removed some commented code
>> - ::addRecordMembersIfNeeded here I added new code to avoid 
>> generating accessor methods for forbidden record component names. 
>> This is to avoid spurious errors later as any way the compiler will 
>> complain because of the forbidden record component names. The issue 
>> with this solution is that the list of forbidden record component 
>> names has been duplicated here. The other location is Attr. We can 
>> probably find a common place to define it. Probably a new utility class?
>
> Looking here
>
> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.03/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java.html
>
> I noted that there are several indentation issues around 
> BasicConstructorHelper (e.g. closing braces seem to be off)
>
> Also, there are some comments like:
>
> 1107                 // public String toString() { return ???; }
>
> Which should be removed, or tweaked.
>
done
>
> As for the list of forbidden names, I think it should live in Names. 
> After all, these are all names, so you can easily build a list there, 
> and share it across javac components.
>

yep you are right that's the natural place

>
>
>> JavacParser:
>> - fixed the bug that the compiler was not allowing final local records
>> - removed the check for abstract modifiers in records, this code was 
>> redundant as Check was already checking this
>> - removed the check for duplicated record component names
>> - added a check to forbid instance initializers in records
>>
>> compiler.properties
>> - I hope I got the error messages right, I tried to reflect all the 
>> comments, suggestions, etc. I was proposed to create a `master` 
>> message for canonical records and use different `causes`, aka 
>> fragments, to be more specific. I decided to follow the same pattern 
>> for accessor methods. Regarding the suggestion of placing the caret 
>> in the throws clause for the corresponding error message, not 
>> possible. We would have to move the check for that error to the parser.
>
> Yep, looks good!
>

cool :)
>
>
> Maurizio
>

Thanks,
Vicente
>
>>
>> New in this iteration:
>> - I have included jdeps related code, I was thinking about putting it 
>> in a separate review bucket but, it is small, it kind of fits with 
>> the rest of the review
>> - a series of supporting libraries for tests under: 
>> test/langtools/lib/combo/tools/javac/combo with also the tests that 
>> needed to be modified due to changes in the lib. These changes were 
>> created by Brian
>> - a minor change done at ToolBox.java
>>
>> Thanks for all the feedback,
>> Vicente
>>
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.03/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191101/3f74b393/attachment.html>