RFR: JEP 359-Records: compiler code

[Vicente Romero]

Hi Maurizio,

Thanks for the comments

On 10/27/19 6:09 PM, Maurizio Cimadamore wrote:
>
> 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
>

this is what the current spec for records mandates:
8.10.4 Record Constructor Declarations
...
"If a record type R declares constructors other the canonical 
constructor, then they must satisfy both of the
following:
The constructor body must start with an explicit constructor invocation 
of the canonical constructor for the
record type R."

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

ditto

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

I can do that if you are pretty strong about this but if we go for it 
those methods you are proposing will be almost useless as it is more 
straight forward to just do a bitwise and with the flag rather than 
having to cast symbols to have access to the particular flavor of the 
method, worst than that we would have to check if the owner of a method 
is actually a class before doing the cast, in preparation of local 
methods. What about renaming the method to: hasRecordFlagSet or similar. 
Or I can just remove the method and do the old faithful bitwise.

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

cool, after shaking the box and all the pieces found their place! :)

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

I have updated the code to do the same we do for enums, I just found my 
approach simpler, but it is true that is less consistent

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

the comment in Lower refers to accessors only, I will document all the 
cases that uses the RECORD flag, as we are running low of flags entry I 
have reused this one for several different uses but you have a point 
that it could be tricky to understand when it is used and why

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

yep I removed them from both, there was a spec change now neither 
accessors nor canonical constructor can have a throws clause
>
> 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;
> +
>
>

yep thanks, removed

> I have not checked the j.l.m API.
>
> Thanks
> Maurizio
>

Thanks,
Vicente

PS, I will be sending a new iteration soon

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191028/11b6fc75/attachment-0001.html>