RFR: JEP 359-Records: compiler code
Vicente Romero
vicente.romero at oracle.com
Sat Oct 26 20:11:46 UTC 2019
Hi Maurizio,
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
done
>
> - 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.
yep removed. This was to set only user defined accessors which were not
set in TypeEnter as opposed to compiler generated ones. But now all
accessors are set in TypeEnter regardless of origin and so this code was
removed
>
> - 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.
yep, I have simplified that code
>
> - As pointed out previously, getting rid of the Pair<Kind, Symbol>
> accessor will result in cascading simplification in few methods in
> Lower too
yep agreed
>
> - 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 will do that later, yes
>
> - 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.
yes that's the case, I have added some comments
>
> - 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 ;-)
yep I have added some comments there too
>
>
> 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).
I have modified some tests in this iteration but I still have to revisit
some of them.
>
>
> That's all for now
>
> Thanks
> Maurizio
Thanks,
Vicente
>
> 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/
More information about the compiler-dev
mailing list