RFR: JEP 359-Records: compiler code
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Wed Oct 30 08:55:59 UTC 2019
On 28/10/2019 21:37, Vicente Romero wrote:
> 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."
Then I would argue the spec is overly restrictive? Is it something we
can fix?
>
>> 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.
I think isRecord() for 'is this a record class' makes sense. But for
everything else feels obscure, and perhaps reverting to bitwise is not
too bad...
>
>> 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
thanks - I'm pushing for this because if we ever allow things like local
enums, it will be easier if the code has more commonalities with records.
>
>> 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
ok
>>
>> 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;
>> +
>>
>>
>
Maurizio
> 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/
>>>>>>
>>>>
>>>
>
More information about the amber-dev
mailing list