RFR: JEP 359-Records: compiler code
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Sun Oct 27 22:33:19 UTC 2019
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?
compiler.err.record.cant.declare.field.modifiers=\
+ record components can not have modifiers
"can not" -> "cannot"
+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?
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"
+
+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)
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\
"
+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)
+compiler.err.invalid.supertype.record=\
+ no class can explicitly extend java.lang.Record
-> "Cannot extend j.l.Record"
+compiler.err.canonical.cant.have.return.statement=\
+ canonical constructor can not have return statements
"can not" -> "cannot"
Maurizio
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/20191027/06cf462e/attachment-0001.html>
More information about the compiler-dev
mailing list