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

[Maurizio Cimadamore]

First, congrats on the work on error messages - it really paid off; the 
diagnostics now look very consistent and easy to grasp. One minor not on 
that side is this message:

"first statement of this constructor must invoke another constructor"

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'" ?

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?


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

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.



> 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!


Maurizio

>
> 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/93f7a2ee/attachment.html>