RFR: JEP 359-Records: compiler code

Sat Oct 26 19:55:06 UTC 2019

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/