[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-compiler-dev
Subject: Re: RFR JDK14-8236597: issues inferring type annotations on records
From: Vicente Romero <vicente.romero () oracle ! com>
Date: 2020-01-13 13:35:50
Message-ID: 4995cc04-94db-6a37-4b76-de5120ef1090 () oracle ! com
[Download RAW message or body]
On 1/10/20 7:58 PM, Maurizio Cimadamore wrote:
>
> Uhm I see - you copy the tree as is from one place to another? This
> might be a bit suboptimal (e.g. tree position and all that), and can
> misfire if the two trees are ever added into a map of some kind.
>
true I can fix that,
> But I take that this code has nothing to do with the patch under review.
>
> Maurizio
>
Vicente
> On 10/01/2020 23:55, Vicente Romero wrote:
> > while we wait on the decision regarding the change in the spec I
> > realized that for the constructor I was already setting the type as
> > defined in the record component, see method:
> > RecordConstructorHelper::finalAdjustment,
> >
> > public JCMethodDecl finalAdjustment(JCMethodDecl md) {
> > List<JCVariableDecl> tmpRecordFieldDecls =recordFieldDecls;
> > for (JCVariableDecl arg : md.params) {
> > /* at this point we are passing all the annotations in the field to
> > the corresponding * parameter in the constructor. */ arg.mods.annotations = \
> > tmpRecordFieldDecls.head.mods.annotations; arg.vartype = \
> > tmpRecordFieldDecls.head.vartype; <------------------------- here \
> > tmpRecordFieldDecls = tmpRecordFieldDecls.tail; }
> > return md;
> > }
> >
> > Thanks,
> > Vicente
> >
> > On 1/10/20 2:23 PM, Maurizio Cimadamore wrote:
> > > Looks good - but I have a question - I assume the changes in
> > > TypeEnter are to 'copy' the tree for the return type 'as is' from
> > > the record component declaration to the accessor return type. That
> > > is, is one is a qualified name, the other should be too, and viceversa.
> > >
> > > Ok, my question then is: shouldn't something like this be said
> > > somewhere in the spec too? After all, since @Nullable foo.bar.Baz is
> > > invalid but @Nullable Baz is not (or should use generate code for
> > > foo.bar.@Nullable Baz ?)
> > >
> > > And... what about parameter types? Shouldn't they need same
> > > treatment too?
> > >
> > > Maurizio
> > >
> > > On 10/01/2020 00:34, Vicente Romero wrote:
> > > > Hi,
> > > >
> > > > Please review this patch [1] to fix a couple of issues regarding
> > > > inference of type annotations on records [2]. There where two cases
> > > > where type annotations were reported as missing. For compact
> > > > records whose arguments are not inheriting the type annotations
> > > > from the corresponding record component and for accessors for which
> > > > the annotation was present but just as a declaration annotation
> > > > applied to the accessor. Not as a type annotation applied to the
> > > > return type which was the expected outcome.
> > > >
> > > > In the case of the compact constructor, the solution was just to
> > > > copy the annotations to the parameters which were missing. In the
> > > > case of the accessor the solution was a bit more complicated.
> > > > Accessors are created but not added to the list definitions
> > > > belonging to the record tree. This is done to make them invisible
> > > > to type attribution as they are not fully fledge methods, but as a
> > > > side effect they are also invisible to the type annotations
> > > > machinery. For this reason type annotations were not recognized as
> > > > such. The solution here has been to make the type annotations
> > > > machinery to visit accessors for records and set the type
> > > > annotations correctly. In order to do that the accessor method
> > > > created at TypeEnter is stored at the record component and visited
> > > > at the same time type annotations are being classified for the rest
> > > > of the code.
> > > >
> > > > Thanks,
> > > > Vicente
> > > >
> > > > [1] http://cr.openjdk.java.net/~vromero/8236597/webrev.00/
> > > > [2] https://bugs.openjdk.java.net/browse/JDK-8236597
> >
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 1/10/20 7:58 PM, Maurizio Cimadamore
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:89027832-5aec-3e5b-1c44-94d83b567900@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Uhm I see - you copy the tree as is from one place to another?
This might be a bit suboptimal (e.g. tree position and all
that), and can misfire if the two trees are ever added into a
map of some kind. </p>
</blockquote>
<br>
true I can fix that,<br>
<br>
<blockquote type="cite"
cite="mid:89027832-5aec-3e5b-1c44-94d83b567900@oracle.com">
<p>But I take that this code has nothing to do with the patch
under review.<br>
</p>
<p>Maurizio<br>
</p>
</blockquote>
<br>
Vicente<br>
<br>
<blockquote type="cite"
cite="mid:89027832-5aec-3e5b-1c44-94d83b567900@oracle.com">
<p> </p>
<div class="moz-cite-prefix">On 10/01/2020 23:55, Vicente Romero
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:863e4a8f-4714-bff8-54aa-a3faa0619451@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
while we wait on the decision regarding the change in the spec I
realized that for the constructor I was already setting the type
as defined in the record component, see method:
RecordConstructorHelper::finalAdjustment,<br>
<br>
<pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans \
Mono';font-size:11.3pt;"><span style="color:#000080;font-weight:bold;">public \
</span>JCMethodDecl finalAdjustment(JCMethodDecl md) { List<JCVariableDecl> \
tmpRecordFieldDecls = <span \
style="color:#660e7a;font-weight:bold;">recordFieldDecls</span>; <span \
style="color:#000080;font-weight:bold;">for </span>(JCVariableDecl arg : md.<span \
style="color:#660e7a;font-weight:bold;">params</span>) {
<span style="color:#808080;font-style:italic;">/* at this point we are \
passing all the annotations in the field to the corresponding </span><span \
style="color:#808080;font-style:italic;"> * parameter in the constructor. \
</span><span style="color:#808080;font-style:italic;"> */ </span><span \
style="color:#808080;font-style:italic;"> </span>arg.<span \
style="color:#660e7a;font-weight:bold;">mods</span>.<span \
style="color:#660e7a;font-weight:bold;">annotations </span>= \
tmpRecordFieldDecls.<span style="color:#660e7a;font-weight:bold;">head</span>.<span \
style="color:#660e7a;font-weight:bold;">mods</span>.<span \
style="color:#660e7a;font-weight:bold;">annotations</span>; arg.<span \
style="color:#660e7a;font-weight:bold;">vartype </span>= tmpRecordFieldDecls.<span \
style="color:#660e7a;font-weight:bold;">head</span>.<span \
style="color:#660e7a;font-weight:bold;">vartype</span>; \
<------------------------- here
tmpRecordFieldDecls = tmpRecordFieldDecls.<span \
style="color:#660e7a;font-weight:bold;">tail</span>; }
<span style="color:#000080;font-weight:bold;">return </span>md;
}</pre>
<br>
Thanks,<br>
Vicente<br>
<br>
<div class="moz-cite-prefix">On 1/10/20 2:23 PM, Maurizio
Cimadamore wrote:<br>
</div>
<blockquote type="cite"
cite="mid:5965e4cb-9443-3a1d-144b-42aded43a581@oracle.com">Looks
good - but I have a question - I assume the changes in
TypeEnter are to 'copy' the tree for the return type 'as is'
from the record component declaration to the accessor return
type. That is, is one is a qualified name, the other should be
too, and viceversa. <br>
<br>
Ok, my question then is: shouldn't something like this be said
somewhere in the spec too? After all, since @Nullable
foo.bar.Baz is invalid but @Nullable Baz is not (or should use
generate code for foo.bar.@Nullable Baz ?) <br>
<br>
And... what about parameter types? Shouldn't they need same
treatment too? <br>
<br>
Maurizio <br>
<br>
On 10/01/2020 00:34, Vicente Romero wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Please review this patch [1] to fix a couple of issues
regarding inference of type annotations on records [2].
There where two cases where type annotations were reported
as missing. For compact records whose arguments are not
inheriting the type annotations from the corresponding
record component and for accessors for which the annotation
was present but just as a declaration annotation applied to
the accessor. Not as a type annotation applied to the return
type which was the expected outcome. <br>
<br>
In the case of the compact constructor, the solution was
just to copy the annotations to the parameters which were
missing. In the case of the accessor the solution was a bit
more complicated. Accessors are created but not added to the
list definitions belonging to the record tree. This is done
to make them invisible to type attribution as they are not
fully fledge methods, but as a side effect they are also
invisible to the type annotations machinery. For this reason
type annotations were not recognized as such. The solution
here has been to make the type annotations machinery to
visit accessors for records and set the type annotations
correctly. In order to do that the accessor method created
at TypeEnter is stored at the record component and visited
at the same time type annotations are being classified for
the rest of the code. <br>
<br>
Thanks, <br>
Vicente <br>
<br>
[1] <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~vromero/8236597/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~vromero/8236597/webrev.00/</a>
<br>
[2] <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8236597"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8236597</a>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic