RFR: JEP 359-Records: compiler code
Vicente Romero
vicente.romero at oracle.com
Sun Oct 27 01:24:21 UTC 2019
Hi Jan,
On 10/22/19 11:21 AM, Jan Lahoda wrote:
> Hi,
>
> In addition to Maurizio's comments, a few more comments:
> -for tests, "--enable-preview -source 14" is used on many places. This
> will cause issues when JDK 15 is started (and --enable-preview -source
> 14 will be replaced with --enable-preview -source 15). On all places
> where that is possible, "--enable-preview -source ${jdk.version}"
> should be used, or a programmatic equivalent.
done
> -for code like this:
> ---$ cat R.java
> public record R(int i) {}
> ---
> compiling without --enable-preview:
> --- $ javac R.java
> /tmp/R.java:1: error: class, interface, or enum expected
> public record R(int i) {}
> ^
> 1 error
> ---
> it would be nice if we could provide some helpful note that to get
> support for records, --enable-preview needs to be used. It may not be
> possible to embed that into the error esp. for nested records, but at
> least a warning. Just so that the user would get a hint what they are
> doing wrong if records don't work for them.
I will check similar code in other preview projects,
> -in Flags, it would be nice to document on which Symbols given flag
> may appear. That would be useful in order to partition the Flags into
> separate Symbol-kind specific sub-sets.
done
> -in Lower, there is method "recordVars, which looks at the
> superclasses of a record, to see if these have a state component - is
> a record superclass of a record allowed? (I thought it isn't.)
I removed that method
> -there seem to be commented debugs in Check, like:
> + //System.out.println("at Check.validateAnnotation: flags: " +
> Flags.toString(s.flags_field) + ", declaration tree " + declarationTree);
>
> Ideally, these would be removed
done
> -in:
> test/langtools/tools/javac/launcher/SourceLauncherTest.javathere are
> changes like:
> - file + ":1: error: class, interface, or enum expected\n" +
> + file + ":1: error: class, interface, enum expected\n" +
>
> are these intentional? These seem suspicious to me.
right, fixed
>
> -in:
> test/langtools/tools/javac/processing/model/element/TestRecord.java
> and:
> test/langtools/tools/javac/processing/model/element/TestRecordDesugar.java
>
>
> there is @bug 8888888 - that seems like a placeholder number.
yep removed
>
> Thanks,
> Jan
Thanks for the review,
Vicente
PS, current iteration:
http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.01/
>
>
> On 21. 10. 19 22:44, 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/
More information about the amber-dev
mailing list