RFR: JEP 359-Records: compiler code
Vicente Romero
vicente.romero at oracle.com
Sat Oct 26 20:14:07 UTC 2019
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/20191026/1e06745a/attachment-0001.html>
More information about the compiler-dev
mailing list