RFR: JEP 359-Records: compiler code
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Oct 21 20:44:19 UTC 2019
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 compiler-dev
mailing list