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

[Joe Darcy]

Hi Vicente,

I've taken a pass over the javax.lang.model portions of the change.

Elements.recordComponentFor should have an @implSpec tag.

I suggest a double-check of the existing visitors/scanners that used to 
extend one of the "FooVisitor9" types and was updated to extend 
"FooVisitor14". In prior work, I had to add a new method override for 
visitRecordComponent to the printing processor to get the proper 
semantics of that class. For example, PubapiVisitor might need an update.

Also, please add the new "FooVisitor14" classes for types.

In terms of how the tests are placed, I'd prefer to see tests of, say 
annotation processing of records, groups with the existing annotation 
processing tests as opposed to grouped with pure records tests. I think 
this arrangement provides better test discoverability long term; for 
example, if someone is updating annotation processing and wants to make 
sure all the annotation processing tests are run.

I recommend the annotation processing tests more consistently subclass 
JavaTestingAbstractProcessor as opposed to directly subclassing 
AbstractProcessor.


In TestRecord.java, please add newlines to the string arguments to the 
options method on line 146.

Thanks,

-Joe

On 10/31/2019 9:11 PM, 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
>
> 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
>
> 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.
>
> 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.
>
> 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?
>
> 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.
>
> 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/