RFR 8189708: class reading issue with named and annotated parameters

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
Hello,

Please consider the following fix for JDK-8189708, which is a class reading issue that prevents parameter names from being recorded for methods that have both MethodParameters and Runtime{Visible,Invisible}ParameterAnnotations attributes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8189708
Webrev: http://cr.openjdk.java.net/~cushon/8189708/webrev.00/
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Jan Lahoda
Hi Liam,

Thanks for looking at this.

FWIW, I was looking at this problem quite a long time ago under
JDK-8007720, but unfortunately didn't have time to fully finish that.
For completeness, I've put the patch I had at that time here:
http://cr.openjdk.java.net/~jlahoda/8007720/8007720

On 20.10.2017 03:51, Liam Miller-Cushon wrote:
> Hello,
>
> Please consider the following fix for JDK-8189708, which is a class
> reading issue that prevents parameter names from being recorded for
> methods that have both MethodParameters and
> Runtime{Visible,Invisible}ParameterAnnotations attributes.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189708
> Webrev: http://cr.openjdk.java.net/~cushon/8189708/webrev.00/

A few comments on this patch:
-what happens if there are both runtime invisible and visible
annotations of method's parameters? Seems those that appear later will
overwrite those that appear sooner?
-the MethodSymbol.savedParameterAnnotations is only used during reading
inside the ClassReader, right? It seems wasteful to have it as a field
on each MethodSymbol, better would be a field in ClassReader.
-please check what happens for annotations on constructors of
enums/non-static innerclasses

Thanks,
     Jan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
Thanks for the comments,

On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda <[hidden email]> wrote:
-what happens if there are both runtime invisible and visible annotations of method's parameters? Seems those that appear later will overwrite those that appear sooner?

Oops, thanks. The way your patch handles that looks good to me.
 
-the MethodSymbol.savedParameterAnnotations is only used during reading inside the ClassReader, right? It seems wasteful to have it as a field on each MethodSymbol, better would be a field in ClassReader.

Sounds good. I'll try to avoid having savedParameterNames as a field in MethodSymbol also. Do you remember if you encountered any issues with that in your patch?
 
-please check what happens for annotations on constructors of enums/non-static innerclasses

Will do. (Also, note that there appears to be an issue with reading MethodParameters on constructors of enums/non-static inner classes: JDK-8177486)
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
Here's an updated patch that incorporates your approach: http://cr.openjdk.java.net/~cushon/8007720/webrev.01/

I included the fix for JDK-8177486 so I could test the inner class / enum constructor case. If this looks like it's on the right track I'll move that part (and the corresponding tests) back into a separate change.

On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon <[hidden email]> wrote:
Thanks for the comments,

On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda <[hidden email]> wrote:
-what happens if there are both runtime invisible and visible annotations of method's parameters? Seems those that appear later will overwrite those that appear sooner?

Oops, thanks. The way your patch handles that looks good to me.
 
-the MethodSymbol.savedParameterAnnotations is only used during reading inside the ClassReader, right? It seems wasteful to have it as a field on each MethodSymbol, better would be a field in ClassReader.

Sounds good. I'll try to avoid having savedParameterNames as a field in MethodSymbol also. Do you remember if you encountered any issues with that in your patch?
 
-please check what happens for annotations on constructors of enums/non-static innerclasses

Will do. (Also, note that there appears to be an issue with reading MethodParameters on constructors of enums/non-static inner classes: JDK-8177486)

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
Hi, is there any more feedback on this?

On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]> wrote:
Here's an updated patch that incorporates your approach: http://cr.openjdk.java.net/~cushon/8007720/webrev.01/

I included the fix for JDK-8177486 so I could test the inner class / enum constructor case. If this looks like it's on the right track I'll move that part (and the corresponding tests) back into a separate change.

On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon <[hidden email]> wrote:
Thanks for the comments,

On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda <[hidden email]> wrote:
-what happens if there are both runtime invisible and visible annotations of method's parameters? Seems those that appear later will overwrite those that appear sooner?

Oops, thanks. The way your patch handles that looks good to me.
 
-the MethodSymbol.savedParameterAnnotations is only used during reading inside the ClassReader, right? It seems wasteful to have it as a field on each MethodSymbol, better would be a field in ClassReader.

Sounds good. I'll try to avoid having savedParameterNames as a field in MethodSymbol also. Do you remember if you encountered any issues with that in your patch?
 
-please check what happens for annotations on constructors of enums/non-static innerclasses

Will do. (Also, note that there appears to be an issue with reading MethodParameters on constructors of enums/non-static inner classes: JDK-8177486)


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Jan Lahoda
Sorry for late reply.

Seems OK to me, only:
-the tests should be in:
test/langtools/tools/javac/MethodParameters/8189708
not in:
test/tools/javac/MethodParameters/8189708

-test/tools/javac/MethodParameters/8189708/Test.java has a weird license
header

Vicente, could you please take a look as well?

Thanks,
     Jan

On 4.12.2017 22:37, Liam Miller-Cushon wrote:

> Hi, is there any more feedback on this?
>
> On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Here's an updated patch that incorporates your approach:
>     http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
>     <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/>
>
>     I included the fix for JDK-8177486 so I could test the inner class /
>     enum constructor case. If this looks like it's on the right track
>     I'll move that part (and the corresponding tests) back into a
>     separate change.
>
>     On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
>     <[hidden email] <mailto:[hidden email]>> wrote:
>
>         Thanks for the comments,
>
>         On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
>         <[hidden email] <mailto:[hidden email]>> wrote:
>
>             -what happens if there are both runtime invisible and
>             visible annotations of method's parameters? Seems those that
>             appear later will overwrite those that appear sooner?
>
>
>         Oops, thanks. The way your patch handles that looks good to me.
>
>             -the MethodSymbol.savedParameterAnnotations is only used
>             during reading inside the ClassReader, right? It seems
>             wasteful to have it as a field on each MethodSymbol, better
>             would be a field in ClassReader.
>
>
>         Sounds good. I'll try to avoid having savedParameterNames as a
>         field in MethodSymbol also. Do you remember if you encountered
>         any issues with that in your patch?
>
>             -please check what happens for annotations on constructors
>             of enums/non-static innerclasses
>
>
>         Will do. (Also, note that there appears to be an issue with
>         reading MethodParameters on constructors of enums/non-static
>         inner classes: JDK-8177486)
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
Thanks,

I updated the copyright header and moved the tests to the correct location:

Also - note that the patch currently includes a fix for JDK-8177486 as well, since you suggested I add test coverage for enum and inner class constructors, and mandated parameters aren't handled correctly without fixing the other bug. Is it OK to have one changeset for both issues, or should they be separated out? I could remove the enum and inner class test coverage and the fix, and then add it back in as a follow-up change if you prefer.

On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <[hidden email]> wrote:
Sorry for late reply.

Seems OK to me, only:
-the tests should be in:
test/langtools/tools/javac/MethodParameters/8189708
not in:
test/tools/javac/MethodParameters/8189708

-test/tools/javac/MethodParameters/8189708/Test.java has a weird license header

Vicente, could you please take a look as well?

Thanks,
    Jan

On 4.12.2017 22:37, Liam Miller-Cushon wrote:
Hi, is there any more feedback on this?

On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]
<mailto:[hidden email]>> wrote:

    Here's an updated patch that incorporates your approach:
    http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
    <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/>

    I included the fix for JDK-8177486 so I could test the inner class /
    enum constructor case. If this looks like it's on the right track
    I'll move that part (and the corresponding tests) back into a
    separate change.

    On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
    <[hidden email] <mailto:[hidden email]>> wrote:

        Thanks for the comments,

        On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
        <[hidden email] <mailto:[hidden email]>> wrote:

            -what happens if there are both runtime invisible and
            visible annotations of method's parameters? Seems those that
            appear later will overwrite those that appear sooner?


        Oops, thanks. The way your patch handles that looks good to me.

            -the MethodSymbol.savedParameterAnnotations is only used
            during reading inside the ClassReader, right? It seems
            wasteful to have it as a field on each MethodSymbol, better
            would be a field in ClassReader.


        Sounds good. I'll try to avoid having savedParameterNames as a
        field in MethodSymbol also. Do you remember if you encountered
        any issues with that in your patch?

            -please check what happens for annotations on constructors
            of enums/non-static innerclasses


        Will do. (Also, note that there appears to be an issue with
        reading MethodParameters on constructors of enums/non-static
        inner classes: JDK-8177486)




Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Vicente Romero-2
looks good to me, only one minor comment, consider using a more meaningful name for:

test/langtools/tools/javac/MethodParameters/8189708/Test.java

there is no need for another review iteration for this change,

Thanks,
Vicente

On 12/13/2017 09:02 PM, Liam Miller-Cushon wrote:
Thanks,

I updated the copyright header and moved the tests to the correct location:

Also - note that the patch currently includes a fix for JDK-8177486 as well, since you suggested I add test coverage for enum and inner class constructors, and mandated parameters aren't handled correctly without fixing the other bug. Is it OK to have one changeset for both issues, or should they be separated out? I could remove the enum and inner class test coverage and the fix, and then add it back in as a follow-up change if you prefer.

On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <[hidden email]> wrote:
Sorry for late reply.

Seems OK to me, only:
-the tests should be in:
test/langtools/tools/javac/MethodParameters/8189708
not in:
test/tools/javac/MethodParameters/8189708

-test/tools/javac/MethodParameters/8189708/Test.java has a weird license header

Vicente, could you please take a look as well?

Thanks,
    Jan

On 4.12.2017 22:37, Liam Miller-Cushon wrote:
Hi, is there any more feedback on this?

On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]
<mailto:[hidden email]>> wrote:

    Here's an updated patch that incorporates your approach:
    http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
    <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/>

    I included the fix for JDK-8177486 so I could test the inner class /
    enum constructor case. If this looks like it's on the right track
    I'll move that part (and the corresponding tests) back into a
    separate change.

    On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
    <[hidden email] <mailto:[hidden email]>> wrote:

        Thanks for the comments,

        On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
        <[hidden email] <mailto:[hidden email]>> wrote:

            -what happens if there are both runtime invisible and
            visible annotations of method's parameters? Seems those that
            appear later will overwrite those that appear sooner?


        Oops, thanks. The way your patch handles that looks good to me.

            -the MethodSymbol.savedParameterAnnotations is only used
            during reading inside the ClassReader, right? It seems
            wasteful to have it as a field on each MethodSymbol, better
            would be a field in ClassReader.


        Sounds good. I'll try to avoid having savedParameterNames as a
        field in MethodSymbol also. Do you remember if you encountered
        any issues with that in your patch?

            -please check what happens for annotations on constructors
            of enums/non-static innerclasses


        Will do. (Also, note that there appears to be an issue with
        reading MethodParameters on constructors of enums/non-static
        inner classes: JDK-8177486)





Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
Thanks for the reviews. I renamed the test, and mentioned both of the associated bugs in the test and the commit message.

The formatted changeset is attached.

On Thu, Dec 14, 2017 at 9:28 AM, Vicente Romero <[hidden email]> wrote:
looks good to me, only one minor comment, consider using a more meaningful name for:

test/langtools/tools/javac/MethodParameters/8189708/Test.java

there is no need for another review iteration for this change,

Thanks,
Vicente


On 12/13/2017 09:02 PM, Liam Miller-Cushon wrote:
Thanks,

I updated the copyright header and moved the tests to the correct location:

Also - note that the patch currently includes a fix for JDK-8177486 as well, since you suggested I add test coverage for enum and inner class constructors, and mandated parameters aren't handled correctly without fixing the other bug. Is it OK to have one changeset for both issues, or should they be separated out? I could remove the enum and inner class test coverage and the fix, and then add it back in as a follow-up change if you prefer.

On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <[hidden email]> wrote:
Sorry for late reply.

Seems OK to me, only:
-the tests should be in:
test/langtools/tools/javac/MethodParameters/8189708
not in:
test/tools/javac/MethodParameters/8189708

-test/tools/javac/MethodParameters/8189708/Test.java has a weird license header

Vicente, could you please take a look as well?

Thanks,
    Jan

On 4.12.2017 22:37, Liam Miller-Cushon wrote:
Hi, is there any more feedback on this?

On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]
<mailto:[hidden email]>> wrote:

    Here's an updated patch that incorporates your approach:
    http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
    <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/>

    I included the fix for JDK-8177486 so I could test the inner class /
    enum constructor case. If this looks like it's on the right track
    I'll move that part (and the corresponding tests) back into a
    separate change.

    On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
    <[hidden email] <mailto:[hidden email]>> wrote:

        Thanks for the comments,

        On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
        <[hidden email] <mailto:[hidden email]>> wrote:

            -what happens if there are both runtime invisible and
            visible annotations of method's parameters? Seems those that
            appear later will overwrite those that appear sooner?


        Oops, thanks. The way your patch handles that looks good to me.

            -the MethodSymbol.savedParameterAnnotations is only used
            during reading inside the ClassReader, right? It seems
            wasteful to have it as a field on each MethodSymbol, better
            would be a field in ClassReader.


        Sounds good. I'll try to avoid having savedParameterNames as a
        field in MethodSymbol also. Do you remember if you encountered
        any issues with that in your patch?

            -please check what happens for annotations on constructors
            of enums/non-static innerclasses


        Will do. (Also, note that there appears to be an issue with
        reading MethodParameters on constructors of enums/non-static
        inner classes: JDK-8177486)







8007720.patch (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Vicente Romero-2
Hi,

Looks good,
Vicente

PS. do you need me to push it or can you push already?

On 12/14/2017 06:55 PM, Liam Miller-Cushon wrote:
Thanks for the reviews. I renamed the test, and mentioned both of the associated bugs in the test and the commit message.

The formatted changeset is attached.

On Thu, Dec 14, 2017 at 9:28 AM, Vicente Romero <[hidden email]> wrote:
looks good to me, only one minor comment, consider using a more meaningful name for:

test/langtools/tools/javac/MethodParameters/8189708/Test.java

there is no need for another review iteration for this change,

Thanks,
Vicente


On 12/13/2017 09:02 PM, Liam Miller-Cushon wrote:
Thanks,

I updated the copyright header and moved the tests to the correct location:

Also - note that the patch currently includes a fix for JDK-8177486 as well, since you suggested I add test coverage for enum and inner class constructors, and mandated parameters aren't handled correctly without fixing the other bug. Is it OK to have one changeset for both issues, or should they be separated out? I could remove the enum and inner class test coverage and the fix, and then add it back in as a follow-up change if you prefer.

On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <[hidden email]> wrote:
Sorry for late reply.

Seems OK to me, only:
-the tests should be in:
test/langtools/tools/javac/MethodParameters/8189708
not in:
test/tools/javac/MethodParameters/8189708

-test/tools/javac/MethodParameters/8189708/Test.java has a weird license header

Vicente, could you please take a look as well?

Thanks,
    Jan

On 4.12.2017 22:37, Liam Miller-Cushon wrote:
Hi, is there any more feedback on this?

On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]
<mailto:[hidden email]>> wrote:

    Here's an updated patch that incorporates your approach:
    http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
    <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/>

    I included the fix for JDK-8177486 so I could test the inner class /
    enum constructor case. If this looks like it's on the right track
    I'll move that part (and the corresponding tests) back into a
    separate change.

    On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
    <[hidden email] <mailto:[hidden email]>> wrote:

        Thanks for the comments,

        On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
        <[hidden email] <mailto:[hidden email]>> wrote:

            -what happens if there are both runtime invisible and
            visible annotations of method's parameters? Seems those that
            appear later will overwrite those that appear sooner?


        Oops, thanks. The way your patch handles that looks good to me.

            -the MethodSymbol.savedParameterAnnotations is only used
            during reading inside the ClassReader, right? It seems
            wasteful to have it as a field on each MethodSymbol, better
            would be a field in ClassReader.


        Sounds good. I'll try to avoid having savedParameterNames as a
        field in MethodSymbol also. Do you remember if you encountered
        any issues with that in your patch?

            -please check what happens for annotations on constructors
            of enums/non-static innerclasses


        Will do. (Also, note that there appears to be an issue with
        reading MethodParameters on constructors of enums/non-static
        inner classes: JDK-8177486)







Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189708: class reading issue with named and annotated parameters

Liam Miller-Cushon
I'm not a committer yet, do you mind pushing it?

On Fri, Dec 15, 2017 at 10:08 AM, Vicente Romero <[hidden email]> wrote:
Hi,

Looks good,
Vicente

PS. do you need me to push it or can you push already?


On 12/14/2017 06:55 PM, Liam Miller-Cushon wrote:
Thanks for the reviews. I renamed the test, and mentioned both of the associated bugs in the test and the commit message.

The formatted changeset is attached.

On Thu, Dec 14, 2017 at 9:28 AM, Vicente Romero <[hidden email]> wrote:
looks good to me, only one minor comment, consider using a more meaningful name for:

test/langtools/tools/javac/MethodParameters/8189708/Test.java

there is no need for another review iteration for this change,

Thanks,
Vicente


On 12/13/2017 09:02 PM, Liam Miller-Cushon wrote:
Thanks,

I updated the copyright header and moved the tests to the correct location:

Also - note that the patch currently includes a fix for JDK-8177486 as well, since you suggested I add test coverage for enum and inner class constructors, and mandated parameters aren't handled correctly without fixing the other bug. Is it OK to have one changeset for both issues, or should they be separated out? I could remove the enum and inner class test coverage and the fix, and then add it back in as a follow-up change if you prefer.

On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <[hidden email]> wrote:
Sorry for late reply.

Seems OK to me, only:
-the tests should be in:
test/langtools/tools/javac/MethodParameters/8189708
not in:
test/tools/javac/MethodParameters/8189708

-test/tools/javac/MethodParameters/8189708/Test.java has a weird license header

Vicente, could you please take a look as well?

Thanks,
    Jan

On 4.12.2017 22:37, Liam Miller-Cushon wrote:
Hi, is there any more feedback on this?

On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <[hidden email]
<mailto:[hidden email]>> wrote:

    Here's an updated patch that incorporates your approach:
    http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
    <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/>

    I included the fix for JDK-8177486 so I could test the inner class /
    enum constructor case. If this looks like it's on the right track
    I'll move that part (and the corresponding tests) back into a
    separate change.

    On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
    <[hidden email] <mailto:[hidden email]>> wrote:

        Thanks for the comments,

        On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
        <[hidden email] <mailto:[hidden email]>> wrote:

            -what happens if there are both runtime invisible and
            visible annotations of method's parameters? Seems those that
            appear later will overwrite those that appear sooner?


        Oops, thanks. The way your patch handles that looks good to me.

            -the MethodSymbol.savedParameterAnnotations is only used
            during reading inside the ClassReader, right? It seems
            wasteful to have it as a field on each MethodSymbol, better
            would be a field in ClassReader.


        Sounds good. I'll try to avoid having savedParameterNames as a
        field in MethodSymbol also. Do you remember if you encountered
        any issues with that in your patch?

            -please check what happens for annotations on constructors
            of enums/non-static innerclasses


        Will do. (Also, note that there appears to be an issue with
        reading MethodParameters on constructors of enums/non-static
        inner classes: JDK-8177486)