RFR: JEP 359-Records: compiler code

Mon Oct 21 18:01:58 UTC 2019

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

* Symbol.java - I think the override for 'erasure' is redundant - isn't 
that the impl from supertype?

* Symbol.java (and others) in general this webrev shuld be updated as 
soon as Jan push the @Preview work, as I see that methods implementing 
preview API are using the 'deprecate for removal' annotation

* 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

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

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

* Enter.java - why are you removing the static flag on records? I don't 
see anything similar around for enums.

* 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? If this works correctly, which errors does the 
'guard' around the error generation is supposed to protect against?

* MemberEnter.java - why the filter for HYPOTHETICAL ? It's only used 
here...

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

* TypeEnter.java - on finishClass - you are calling memberEnter on 
record fields, which I think you already did in the new RecordsPhase

* TypeEnter.java - (stylistic) addRecordsMemberIfNeeded should deal with 
_all_ record members (e.g. including accessors), not just some?

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

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

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

*TypeEnter.java - addAccessor can be simplified if we only worry about 
getters. Again, the checks in here feel more Attr check than MemberEnter 
checks.

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

*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

* 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

* 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

* TreeInfo.java - how is 'isCanonicalConstructor' not returning 'true' 
for all constructors inside a record, as opposed to only return true for 
the canonical one?

* TreeInfo.java - There is some code reuse possible between 
"recordFieldTypes" and "recordFields"

* Names.java - what is 'oldEquals' ?

* JavacParser.java - timing of checks; I don't think we should check for 
illegal record component names in here

* JavacParser.java - code can be simplified somewhat by getting rid of 
accessors in VarDef AST.

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/20191021/ad14b0a2/attachment-0001.html>