RFR: JEP 359-Records: compiler code

[Maurizio Cimadamore]

Hi Vicente, looks a lot better, thanks.

Some comments:

Attr.java - Errors.FirstStatementMustBeCallToCanonical - not sure about 
this message , but in general, all we require is that a constructor 
delegates to another constructor - it can also be something other than 
the canonical - e.g contsructor1 -> constructor2 -> canonical

Attr.java - the above situation seems to also affect some other code in 
Attr - where you actually check that the invoked constructor in 'this' 
has same signature of the canonical constructor. I think these checks 
should be removed - the important thing is that the canonical will be 
called _somehow_ - but can also be call indirectly - at least this was 
my understanding

Attr.java - not sure about the tree.sym.isRecord in the visitMethod - it 
seems like the same flag is used for both record (class) and canonical 
constructor? I'm ok with sharing the flag, but the method name looks a 
bit confusing when applied to a method symbol. I suggest putting 
'isRecord' inside ClassSymbol, and 'isCanonicalRecordConstructor' on 
MethodSymbol, so that client code cannot make mistakes.

Attr.java/TypeEnter.java - overall, the checks here look nice, and they 
'blend' in with existing code nicely, I think. Well done!

Enter.java - I guess my question here on records and treatment with 
STATIC is a general question as to why isn't the code doing the same 
thing for records and enums - aren't the rules similar? (e.g. enums must 
be static, etc)? Enums are not set STATIC by default in javac parser, 
and that is, I think the difference here. What pushed you in this direction?

TypeEnter.java: I suggest renaming "getCanonicalInitDecl" to 
"getCanonicalConstructorDecl"

TypeEnter.java - it seems like the result of getCanonicalInitDecl is 
always given the RECORD flag - but later on (in Lower) I saw a comment 
which says that only auto-generated symbols have the RECORD flag set. 
Which one is it?

TypeEnter.java - I think the isUnchecked methods are a leftover from 
previous code - now they seem to be in Attr.java (btw, I think we must 
have those checks somewhere else in javac, look in Check::isUnchecked)

Names.java - I think this bunch of fields is also no longer used?

+ public final Name where;
+ public final Name non;
+ public final Name ofLazyProjection;
+


I have not checked the j.l.m API.

Thanks
Maurizio




On 26/10/2019 21:14, Vicente Romero wrote:
> I forgot to mention that I have added javax.lang.model code to this 
> iteration that wasn't part of the first iteration. I was planning to 
> publish it as part of a different review but I realized that there was 
> some code affected, tests, API implementation etc, which belonged to 
> the compiler code. So I added that code in this iteration,
>
> Thanks,
> Vicente
>
> On 10/26/19 3:55 PM, Vicente Romero wrote:
>> Hi Maurizio,
>>
>> Thanks again for the comments I have published another iteration at 
>> [1]. I focused on this iteration more on the implemenation than on 
>> the tests to first have an agreement on the implementation, timing, 
>> etc. Some comments inlined below.
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.01/
>>
>> On 10/21/19 2:01 PM, Maurizio Cimadamore wrote:
>>>
>>> Hi Vicente,
>>> I did a pretty thorough pass on most of the code. I didn't look at 
>>> tests, and I also didn't look at Lower. Comments below:
>>>
>>> * Flags.java - VARARGS flag for records components; I wonder, 
>>> instead of a new flag, can we use the internal VARARGS flag we have 
>>> for methods, and attach that to the record symbol? That should also 
>>> lead to more direct code in TypeHelper
>>>
>> removed
>>
>>> * Symbol.java - I think the override for 'erasure' is redundant - 
>>> isn't that the impl from supertype?
>>>
>> yep, removed too
>>
>>> * Symbol.java - I wonder if accessor list with Pair<Kind, Symbol> 
>>> isn't a premature generalization; we should just add a getter symbol 
>>> and that's it 
>>
>> agreed, done
>>
>>> * Attr.java - I think we might want to leave the door open for a 
>>> check which forces all constructors of a record to go through the 
>>> canonical one (depending on where the spec lands)
>>>
>> implemented in this iteration
>>
>>> * Check.java - understanding checkpoint: when we see an annotation 
>>> on a record component, first we check it's one of the kinds which 
>>> are allowed (if not, error), and, if it's allowed, we add all record 
>>> component annotations to record component elements, and we also 
>>> filter away all annotations that have nothing to do with the element 
>>> in which they appear. If my understanding is correct, I think this 
>>> logic should be documented more clearly; I found the comment after 
>>> the "if (isRecordField)" to be a bit obscure.
>>>
>> yes that's the idea, annotations that are originally applied to 
>> record components are pushed down to all generated elements in 
>> TypeEnter, and then in Check the ones that are off-site are removed
>>>
>>> * Enter.java - why are you removing the static flag on records? I 
>>> don't see anything similar around for enums.
>>>
>>
>> the static flag is added to all records but if the record is a top 
>> level class, it is not needed, that's why that code is there
>>
>>> * Flow.java - not sure I get the changes to checkInit; typically 
>>> checkInit is called at the use-site of DA/DU variables. Here it 
>>> seems you suppress some of the errors emitted for accessing record 
>>> fields inside the canonical constructor - but I hope that code like this
>>>
>>> record Foo(int x) {
>>>    Foo(int x) {
>>>        print(this.x);
>>>    }
>>> }
>>>
>>> Still give errors?
>>>
>>
>> yes it gives an error
>>>
>>> If this works correctly, which errors does the 'guard' around the 
>>> error generation is supposed to protect against?
>>>
>>
>> checkInit it not used only for what you mentioned above but also, see 
>> AssignAnalyzer::visitMethodDef, to check if an initial constructor, 
>> as the canonical constructor is in records, have initialized all the 
>> fields. The guard is there to don't issue an error if a canonical 
>> constructor hasn't initialized some fields, as the compiler will 
>> generate code to initialize those fields later in Lower
>>>
>>> * MemberEnter.java - why the filter for HYPOTHETICAL ? It's only 
>>> used here...
>>>
>> removed
>>>
>>> * TypeEnter.java - implicit super calls are added in 
>>> Attr::visitMethod for regular calls; we should do the same for 
>>> records (or add all in TypeEnter - that is records and class should 
>>> be consistent)
>>>
>>
>> right we should be consistent, I have moved that code to Attr
>>
>>> * TypeEnter.java - on finishClass - you are calling memberEnter on 
>>> record fields, which I think you already did in the new RecordsPhase
>>>
>>
>> nope, what I'm doing there is invoking MemberEnter to enter the 
>> members that hasn't been entered so far. Anyway that code changed a 
>> bit because I'm entering the constructors now at RecordPhase too but 
>> I have changed the code a bit to make more clear what is happening.
>>
>>> * TypeEnter.java - (stylistic) addRecordsMemberIfNeeded should deal 
>>> with _all_ record members (e.g. including accessors), not just some?
>>>
>> yep changed that too
>>>
>>> * TypeEnter.java - checkForSerializationMember should probably be 
>>> moved to MemberEnter::visitVar, or even to Attr (note that the code 
>>> for the check is doing a little visit :-))
>>>
>> moved to Attr
>>>
>>> * TypeEnter.java - again on check timings; while it's ok for the 
>>> code in here to add new synthetic members, I think it's less ok to 
>>> add more global error checks (such as make sure that the canonical 
>>> declaration whose parameter names match the record components in 
>>> order); these should live in Attr. More generally, I think that we 
>>> should only check stuff here if we think that the check will add any 
>>> value to annotation processing. Every other check can be deferred, 
>>> and take place in a more 'deterministic' part of javac.
>>>
>>
>> moved to Attr
>>
>>> * TypeEnter.java - I think finishClass should be a bit better at 
>>> determining as to whether default constructor is needed or not - for 
>>> instance, this check:
>>>
>>> if ((sym.flags() & INTERFACE) == 0 &&
>>>   928                 !TreeInfo.hasConstructors(tree.defs)) {
>>>
>>> Should be generalized to something that works for both classes and 
>>> records; for classes you need to check if there's no other 
>>> constructor; for records you need to check if there's no other 
>>> constructor _with same signature_ as the canonical one. Then you can 
>>> simplify addRecordMembers and remove the dependency on the boolean 
>>> 'generatedConstructor' parameter. In other words the code should:
>>>
>>> 1) check if default/canonical constructor generation is required
>>> 2) if so, use the appropriate helper to generate the code
>>> 3) at the end, add the remaining record members (under the 
>>> assumption that the canonical constructor has already been added in 
>>> (1), if that was missing)
>>>
>>
>> done, in order to do that I had to enter constructors at the 
>> RecordPhase, as mentioned earlier.
>>>
>>> *TypeEnter.java - addAccessor can be simplified if we only worry 
>>> about getters. Again, the checks in here feel more Attr check than 
>>> MemberEnter checks.
>>>
>> agreed, done
>>
>>> *TypeEnter.java - in addRecordMembersIfNeeded, I don't get why we 
>>> create a tree for a member, and then we visit the member tree with 
>>> memberEnter, just to add it to the scope. I understand that, 
>>> currently addEnumMembers does the same, but this looks very 
>>> roundabout; I wonder if there's a way to make all this process a bit 
>>> simpler - create a symbol and add that to the scope. Or are there 
>>> important checks in MemberEnter that we would lose?
>>>
>>
>> yes there are several checks we would lose, plus we would lose 
>> consistency, but I tried to do that and several things fell apart, we 
>> need to enter not only the generated method, also it's parameters 
>> etc, which is what MemberEnter is doing.
>>
>>> *JCTree.java/TreeMaker.java - I don't think there's any need to 
>>> store accessors in the field AST; these are only used from 
>>> TypeEnter, and TypeEnter can do whatever it does by looking at which 
>>> record components there are in the record class, and add a getter 
>>> for each. Let's make the code simpler and more direct
>>>
>> yep removed
>>>
>>> * ClassReader.java - should we just silently ignore record 
>>> attributes when not in preview mode - or should we issue classfile 
>>> errors?
>>>
>>> * ClassReader.java - what kind of validation should we do on record 
>>> attributes? Currently javac does nothing. Should we check that we 
>>> have (i) getters (ii) toString/hashCode/equals implementations and 
>>> (iii) a canonical constructor (ad fail if we don't) ? At the very 
>>> least I would add code to _parse_ the attribute, even if we do 
>>> nothing with it, so that at least we throw a classfile error if the 
>>> attribute is badly broken
>>>
>> on ClassReader, we can discuss what to do in a language meeting, I 
>> don't have any strong preference
>>>
>>> * Tokens.java - for "var", "yields" and other context-dependent 
>>> keywords we never added a token. We just handled that in 
>>> JavacParser. Why the difference here? I think it's best to stick to 
>>> current style and maybe fix all of them (assuming that's what we 
>>> want to do) in a followup cleanup. Actually, after looking at 
>>> parser, it seems like you already handle that manually, so I just 
>>> suggest to revert the changed to Tokens
>>>
>>
>> I added the token to add it as a parameter to an error message, but I 
>> removed the token and now I'm passing a string
>>
>>> * TreeInfo.java - how is 'isCanonicalConstructor' not returning 
>>> 'true' for all constructors inside a record, as opposed to only 
>>> return true for the canonical one?
>>>
>> I have added a comment to clarify what this method is doing
>>>
>>> * TreeInfo.java - There is some code reuse possible between 
>>> "recordFieldTypes" and "recordFields"
>>>
>> yep done
>>>
>>> * Names.java - what is 'oldEquals' ?
>>>
>> removed, old code
>>>
>>> * JavacParser.java - timing of checks; I don't think we should check 
>>> for illegal record component names in here
>>>
>> removed from there
>>>
>>> * JavacParser.java - code can be simplified somewhat by getting rid 
>>> of accessors in VarDef AST.
>>>
>>
>> done
>>
>> Thanks again for taking the time to do this long review, will answer 
>> the other mails separately
>>
>> Vicente
>>>
>>>
>>>
>>>
>>> On 21/10/2019 13:31, Vicente Romero wrote:
>>>> Hi,
>>>>
>>>> Please review the compiler code for JEP 359 (Records) [1]
>>>>
>>>> Thanks in advance for the feedback,
>>>> Vicente
>>>>
>>>> [1] 
>>>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.00/
>>
>