<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Maurizio,<br>
<br>
I have uploaded another iteration [1], some comments inlined below.<br>
<br>
<div class="moz-cite-prefix">On 10/27/19 6:33 PM, Maurizio
Cimadamore wrote:<br>
</div>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>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):</p>
<pre><span class="new">+compiler.err.record.cant.declare.duplicate.fields=\</span>
<span class="new">+ records cannot declare components with the same name</span>
<span class="new">+</span></pre>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">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?</div>
</blockquote>
<br>
not sure what to do here, there are several error messages about
duplicated elements that are also pretty specific, see for modules,
annotations etc.<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<div class="moz-cite-prefix"><br>
<pre><span class="new">compiler.err.record.cant.declare.field.modifiers=\</span>
<span class="new">+ record components can not have modifiers
</span></pre>
</div>
<div class="moz-cite-prefix">"can not" -> "cannot"</div>
</blockquote>
<br>
done<br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">
<pre><span class="new">+compiler.err.record.fields.must.be.in.header=\</span>
<span class="new">+ instance fields in a record must be declared in the header
</span></pre>
</div>
<div class="moz-cite-prefix">Should it say that fields should be
in the header, or should it say that a record cannot declare
fields?</div>
</blockquote>
<br>
yep fixed<br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<div class="moz-cite-prefix"><br>
Invalid field declaration in record</div>
<div class="moz-cite-prefix">(consider replacing field with record
component)</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Or something like that.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">
<pre><span class="new">+compiler.err.canonical.constructor.must.be.public=\</span>
<span class="new">+ canonical constructor must be public
</span><span class="new"></span>
</pre>
I think I'd prefer to qualify "canonical _record_ constructor"<br>
</div>
</blockquote>
<br>
done<br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<div class="moz-cite-prefix">
<pre><span class="new"></span><span class="new"></span>Â <span class="new">+</span>
<span class="new">+compiler.err.canonical.with.name.mismatch=\</span>
<span class="new">+ constructor with same signature as canonical does not match by parameter names</span></pre>
</div>
<div class="moz-cite-prefix">This is a tricky one. Perhaps let's
try to simplify:</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">"Invalid parameter names in record
constructor"<br>
(parameter names must match the names of the declared record
components)<br>
</div>
</blockquote>
<br>
done<br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<div class="moz-cite-prefix"> </div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Or something similar.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">
<pre><span class="new">+compiler.err.accessor.return.type.doesnt.match=\</span>
<span class="new">+ type returned by the accessor is not the same as the type of the corresponding record component\n\</span>
<span class="new">+ required: {0}\n\</span>
<span class="new">+ found: {1}\n\</span>
<span class="new">+
</span></pre>
</div>
<p>Again, maybe this can be simpler:</p>
<p>"Unexpected type in record accessor<span class="new"><br>
+ required: {0}\n\</span><span class="new"><br>
+ found: {1}\n\<br>
"<br>
</span></p>
</blockquote>
<br>
yep agreed<br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<p><span class="new"> </span></p>
<pre><span class="new">+compiler.err.illegal.record.component.name=\</span>
<span class="new">+ record {0}, declares an illegal record component name: {1}</span></pre>
<p><br>
Better to flip the sentence around?</p>
<p>Illegal record component name {1}<br>
(record components cannot have same name as Object methods, ...)</p>
<p>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)\\</p>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<pre><span class="new">+compiler.err.invalid.supertype.record=\</span>
<span class="new">+ no class can explicitly extend java.lang.Record</span></pre>
<p>-> "Cannot extend j.l.Record"</p>
</blockquote>
done<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<p><br>
</p>
<pre><span class="new">+compiler.err.canonical.cant.have.return.statement=\</span>
<span class="new">+ canonical constructor can not have return statements</span></pre>
<p>"can not" -> "cannot"</p>
</blockquote>
fixed<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<p><br>
</p>
<p>Maurizio<br>
</p>
</blockquote>
<br>
Thanks,<br>
Vicente<br>
<br>
[1]
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.02/">http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.02/</a><br>
<br>
<blockquote type="cite"
cite="mid:415e23ea-8414-5964-3d5a-66a7e33cb8e8@oracle.com">
<p> <span class="new"></span> </p>
<pre><span class="new"></span></pre>
<div class="moz-cite-prefix">On 27/10/2019 22:09, Maurizio
Cimadamore wrote:<br>
</div>
<blockquote type="cite"
cite="mid:c93c974e-92c1-e4a3-c3a7-0ea5c5f26c2f@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>Hi Vicente, looks a lot better, thanks.</p>
<p>Some comments:</p>
<p>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</p>
<p>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<br>
</p>
<p>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.</p>
<p>Attr.java/TypeEnter.java - overall, the checks here look
nice, and they 'blend' in with existing code nicely, I think.
Well done!</p>
<p>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?</p>
<p>TypeEnter.java: I suggest renaming "getCanonicalInitDecl" to
"getCanonicalConstructorDecl"<br>
<br>
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?</p>
<p>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)</p>
<p>Names.java - I think this bunch of fields is also no longer
used?</p>
<pre><span class="new">+ public final Name where;</span>
<span class="new">+ public final Name non;</span>
<span class="new">+ public final Name ofLazyProjection;</span>
<span class="new">+</span></pre>
<p><br>
I have not checked the j.l.m API.</p>
<p>Thanks<br>
Maurizio<br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 26/10/2019 21:14, Vicente Romero
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:b072fc50-43e3-ae97-3cb6-c4c49e60edc0@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
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,<br>
<br>
Thanks,<br>
Vicente<br>
<br>
<div class="moz-cite-prefix">On 10/26/19 3:55 PM, Vicente
Romero wrote:<br>
</div>
<blockquote type="cite"
cite="mid:c265f401-36a9-dba7-5e0d-e95a11a3820f@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
Hi Maurizio,<br>
<br>
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.<br>
<br>
[1] <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.01/</a><br>
<br>
<div class="moz-cite-prefix">On 10/21/19 2:01 PM, Maurizio
Cimadamore wrote:<br>
</div>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>Hi Vicente,<br>
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:<br>
</p>
<p>* 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<br>
</p>
</blockquote>
removed<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p> </p>
<p>* Symbol.java - I think the override for 'erasure' is
redundant - isn't that the impl from supertype?</p>
</blockquote>
yep, removed too<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">*
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 </blockquote>
<br>
agreed, done<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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)</p>
</blockquote>
implemented in this iteration<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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.</p>
</blockquote>
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<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* Enter.java - why are you removing the static flag on
records? I don't see anything similar around for enums.</p>
</blockquote>
<br>
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<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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<br>
<br>
record Foo(int x) {<br>
  Foo(int x) {<br>
      print(this.x);<br>
  }<br>
}</p>
<p>Still give errors?</p>
</blockquote>
<br>
yes it gives an error<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p> If this works correctly, which errors does the 'guard'
around the error generation is supposed to protect
against?</p>
</blockquote>
<br>
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<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* MemberEnter.java - why the filter for HYPOTHETICAL ?
It's only used here...</p>
</blockquote>
removed<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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)</p>
</blockquote>
<br>
right we should be consistent, I have moved that code to
Attr<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* TypeEnter.java - on finishClass - you are calling
memberEnter on record fields, which I think you already
did in the new RecordsPhase</p>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* TypeEnter.java - (stylistic) addRecordsMemberIfNeeded
should deal with _all_ record members (e.g. including
accessors), not just some?</p>
</blockquote>
yep changed that too<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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 :-))</p>
</blockquote>
moved to Attr<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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.</p>
</blockquote>
<br>
moved to Attr<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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:</p>
<pre>if ((sym.flags() & INTERFACE) == 0 &&
928 !TreeInfo.hasConstructors(tree.defs)) {</pre>
<p>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:</p>
<p>1) check if default/canonical constructor generation is
required<br>
2) if so, use the appropriate helper to generate the
code<br>
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)</p>
</blockquote>
<br>
done, in order to do that I had to enter constructors at the
RecordPhase, as mentioned earlier.<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>*TypeEnter.java - addAccessor can be simplified if we
only worry about getters. Again, the checks in here feel
more Attr check than MemberEnter checks.</p>
</blockquote>
agreed, done<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>*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?<br>
</p>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p> </p>
<p>*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</p>
</blockquote>
yep removed<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* ClassReader.java - should we just silently ignore
record attributes when not in preview mode - or should
we issue classfile errors?</p>
<p>* 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</p>
</blockquote>
on ClassReader, we can discuss what to do in a language
meeting, I don't have any strong preference<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* 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<br>
</p>
</blockquote>
<br>
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<br>
<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p> </p>
<p>* TreeInfo.java - how is 'isCanonicalConstructor' not
returning 'true' for all constructors inside a record,
as opposed to only return true for the canonical one?</p>
</blockquote>
I have added a comment to clarify what this method is doing<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* TreeInfo.java - There is some code reuse possible
between "recordFieldTypes" and "recordFields"</p>
</blockquote>
yep done<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* Names.java - what is 'oldEquals' ?</p>
</blockquote>
removed, old code<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* JavacParser.java - timing of checks; I don't think we
should check for illegal record component names in here</p>
</blockquote>
removed from there<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p>* JavacParser.java - code can be simplified somewhat by
getting rid of accessors in VarDef AST.<br>
</p>
</blockquote>
<br>
done<br>
<br>
Thanks again for taking the time to do this long review,
will answer the other mails separately<br>
<br>
Vicente<br>
<blockquote type="cite"
cite="mid:0be9bfc0-70b1-c1f8-5af1-5c88913dbb9d@oracle.com">
<p> </p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 21/10/2019 13:31, Vicente
Romero wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1cfed5da-a0eb-be75-9008-8a0a5666a6f9@oracle.com">Hi,
<br>
<br>
Please review the compiler code for JEP 359 (Records)
[1] <br>
<br>
Thanks in advance for the feedback, <br>
Vicente <br>
<br>
[1] <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.00/</a>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</body>
</html>