RFR: JEP 359-Records: compiler code
Vicente Romero
vicente.romero at oracle.com
Mon Oct 28 22:48:19 UTC 2019
Hi Maurizio,
I have uploaded another iteration [1], some comments inlined below.
On 10/27/19 6:33 PM, Maurizio Cimadamore wrote:
>
> Also some comments on error messages (some feedback from Alex/Jon
> would also be appreciated here; error messages are also crucial to
> convey important bits of the programming model, so I think we should
> try to get them right):
>
> +compiler.err.record.cant.declare.duplicate.fields=\
> + records cannot declare components with the same name
> +
>
> I think this should be less record specific - it's just a duplicate
> declaration - so we should issue same error as with method/field
> duplicate decl?
not sure what to do here, there are several error messages about
duplicated elements that are also pretty specific, see for modules,
annotations etc.
>
> compiler.err.record.cant.declare.field.modifiers=\
> + record components can not have modifiers
> "can not" -> "cannot"
done
>
> +compiler.err.record.fields.must.be.in.header=\
> + instance fields in a record must be declared in the header
> Should it say that fields should be in the header, or should it say
> that a record cannot declare fields?
yep fixed
>
> Invalid field declaration in record
> (consider replacing field with record component)
>
> Or something like that.
>
> +compiler.err.canonical.constructor.must.be.public=\
> + canonical constructor must be public
> I think I'd prefer to qualify "canonical _record_ constructor"
done
> +
> +compiler.err.canonical.with.name.mismatch=\
> + constructor with same signature as canonical does not match by
> parameter names
> This is a tricky one. Perhaps let's try to simplify:
>
> "Invalid parameter names in record constructor"
> (parameter names must match the names of the declared record components)
done
>
> Or something similar.
>
> +compiler.err.accessor.return.type.doesnt.match=\
> + type returned by the accessor is not the same as the type of the
> corresponding record component\n\
> + required: {0}\n\
> + found: {1}\n\
> +
>
> Again, maybe this can be simpler:
>
> "Unexpected type in record accessor
> + required: {0}\n\
> + found: {1}\n\
> "
>
yep agreed
> +compiler.err.illegal.record.component.name=\
> + record {0}, declares an illegal record component name: {1}
>
>
> Better to flip the sentence around?
>
> Illegal record component name {1}
> (record components cannot have same name as Object methods, ...)
>
> Maybe the first line could be enough, but if we could list the names
> that are 'reserved' that could be good too, assuming we can find a
> nice way to do that)\\
>
I would say lets let only the first part: "Illegal record component name
{1}" because there are other serialization related methods and fields
that cant be used as component names. Listing all the possible sources
would be muddy business I think.
> +compiler.err.invalid.supertype.record=\
> + no class can explicitly extend java.lang.Record
>
> -> "Cannot extend j.l.Record"
>
done
>
>
> +compiler.err.canonical.cant.have.return.statement=\
> + canonical constructor can not have return statements
>
> "can not" -> "cannot"
>
fixed
>
>
> Maurizio
>
Thanks,
Vicente
[1] http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.02/
> On 27/10/2019 22:09, 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
>>
>> 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
>>
>> 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.
>>
>> Attr.java/TypeEnter.java - overall, the checks here look nice, and
>> they 'blend' in with existing code nicely, I think. Well done!
>>
>> 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?
>>
>> 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?
>>
>> 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)
>>
>> 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;
>> +
>>
>>
>> I have not checked the j.l.m API.
>>
>> Thanks
>> Maurizio
>>
>>
>>
>>
>> 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/93458c6d/attachment-0001.html>
More information about the compiler-dev
mailing list