RFR: JEP 359-Records: compiler code

[Vicente Romero]

Hi Maurizio,

Thanks a lot for the review, working on your comments,

Vicente

On 10/21/19 4:44 PM, Maurizio Cimadamore wrote:
> And here are some comments on Lower
>
> - findMethodOrFailSilently  doesn't seem to be used anywhere; this 
> should be removed and associated changes in Resolve reverted
>
> - findUserDefinedAccessors - this seems to have to do with setting the 
> record component symbol straight - this should happen well before 
> Lower, otherwise I'm not even sure what annotations processor will 
> see? This code should go in TypeEnter, where you already look up for 
> existing accessor.
>
> - related; not 100% as to why in visitRecordDef you protect against 
> accessor not being there - which means you need to do a lookup. You 
> need to get to this part of the code where all accessors have been 
> set. Then the code can be simplified.
>
> - As pointed out previously, getting rid of the Pair<Kind, Symbol> 
> accessor will result in cascading simplification in few methods in 
> Lower too
>
> - both the signature generator and the indy machinery are shared 
> between LambdaToMethod and Lower - so we should probably put them 
> somewhere in a common superclass which can be used by the various 
> backend steps
>
> - I guess the main translation strategy for record members is to 
> generate an indy - where the runtime gives you back some constant 
> callsite which wraps a method handle with the right signature. If so, 
> some comments should be sprinkled around to clarify that this is 
> indeed the case.
>
> - I also guess that the if/else in generateRecordMethod is to avoid 
> generating a tree if an explicit member has been declared by the user 
> - again, correct, but some comments please ;-)
>
>
> Also some comments  on tests:
>
> * test/langtools/tools/javac/6402516/CheckLocalElements.java - why the 
> change?
>
> * test/langtools/tools/javac/AnonymousClass/AnonymousClassFlags.java 
> -  why the change from @run to @compile?
>
> * 
> test/langtools/tools/javac/annotations/repeatingAnnotations/combo/TargetAnnoCombo.java 
> - who is using the new target?
>
> * diags/** in general, for all new diagnostics added it would be nice 
> to have an html of the output (I have a script for doing that, let me 
> know if you need it)
>
> * examples-not-yet - why no test for local records? That should be 
> easy to add (I hope)?
>
> * test/langtools/tools/javac/parser/JavacParserTest.java - here I 
> wonder if we should have different messages depending on the version 
> (eg. we don't want to say 'expected records' if compiling with -source 
> 12?)
>
> *  test/langtools/tools/javac/tree/JavacTreeScannerTest.java, 
> test/langtools/tools/javac/tree/SourceTreeScannerTest.java, 
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Accessors.java 
> - seems like these probably depend on the accessor pairs being in the 
> AST?
>
> * test/langtools/tools/javac/doctree/AccessorsTest.java - not sure 
> about this, does it even belong to this patch? I'd be surprised if 
> DocTree does anything special with accessors?
>
> * test/langtools/tools/javac/doctree/AccessorsTest.java - this tests 
> that ElementFilter and getAccessor() agree, but doesn't test that they 
> actually yield the correct result
>
> * more generally, certain tests (e.g. signature mismatches, record 
> component names order mismatches, reflection tests, serialization 
> tests) have a certain ad-hoc nature to them - in the sense that they 
> test one record shape or two and that's it. E.g.
>
> test/langtools/tools/javac/records/mandated_members/read_resolve_method/CheckReadResolveMethodTest.java 
>
>
> I'd like to see a more combinatorial-oriented approach to such tests, 
> where at least we tests all primitive types plus a reference type of 
> choice, with varying degrees of arity (and w/, w/o varargs).
>
>
> That's all for now
>
> Thanks
> Maurizio
>
> On 21/10/2019 19:01, 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
>>
>> * Symbol.java - I think the override for 'erasure' is redundant - 
>> isn't that the impl from supertype?
>>
>> * Symbol.java (and others) in general this webrev shuld be updated as 
>> soon as Jan push the @Preview work, as I see that methods 
>> implementing preview API are using the 'deprecate for removal' 
>> annotation
>>
>> * 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
>>
>> * 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)
>>
>> * 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.
>>
>> * Enter.java - why are you removing the static flag on records? I 
>> don't see anything similar around for enums.
>>
>> * 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? If this works correctly, which errors does the 
>> 'guard' around the error generation is supposed to protect against?
>>
>> * MemberEnter.java - why the filter for HYPOTHETICAL ? It's only used 
>> here...
>>
>> * 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)
>>
>> * TypeEnter.java - on finishClass - you are calling memberEnter on 
>> record fields, which I think you already did in the new RecordsPhase
>>
>> * TypeEnter.java - (stylistic) addRecordsMemberIfNeeded should deal 
>> with _all_ record members (e.g. including accessors), not just some?
>>
>> * 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 :-))
>>
>> * 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.
>>
>> * 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)
>>
>> *TypeEnter.java - addAccessor can be simplified if we only worry 
>> about getters. Again, the checks in here feel more Attr check than 
>> MemberEnter checks.
>>
>> *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?
>>
>> *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
>>
>> * 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
>>
>> * 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
>>
>> * TreeInfo.java - how is 'isCanonicalConstructor' not returning 
>> 'true' for all constructors inside a record, as opposed to only 
>> return true for the canonical one?
>>
>> * TreeInfo.java - There is some code reuse possible between 
>> "recordFieldTypes" and "recordFields"
>>
>> * Names.java - what is 'oldEquals' ?
>>
>> * JavacParser.java - timing of checks; I don't think we should check 
>> for illegal record component names in here
>>
>> * JavacParser.java - code can be simplified somewhat by getting rid 
>> of accessors in VarDef AST.
>>
>>
>>
>>
>>
>> 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/