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