Quantcast

RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

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

RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Liam Miller-Cushon
Hello,

The JavacMessager API allows a message to be generated for a source position defined by a combination of Element + AnnotationMirror + AnnotationValue. The AnnotationValue is currently ignored, and the position is defined using only the Element and AnnotationMirror.

This fix causes the AnnotationValue to be considered when calculating the diagnostic position. The value must be a direct value of the given annotation, or an element in an array initializer. Values of nested annotation mirrors are not supported.

For example, given `@A(x = {1}, y = @B(2))` positions will be calculated for the AnnotationMirror + AnnotationValue pairs @A + {1}, @A + 1, and @B + 2, but not for @A + 2.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

joe darcy
While not a review, since the API was designed to support the more
precise message reporting, it would be good to see that functionality
present in javac.

Thanks,

-Joe

On 1/3/2017 1:21 PM, Liam Miller-Cushon wrote:

> Hello,
>
> The JavacMessager API allows a message to be generated for a source
> position defined by a combination of Element + AnnotationMirror +
> AnnotationValue. The AnnotationValue is currently ignored, and the
> position is defined using only the Element and AnnotationMirror.
>
> This fix causes the AnnotationValue to be considered when calculating
> the diagnostic position. The value must be a direct value of the given
> annotation, or an element in an array initializer. Values of nested
> annotation mirrors are not supported.
>
> For example, given `@A(x = {1}, y = @B(2))` positions will be
> calculated for the AnnotationMirror + AnnotationValue pairs @A + {1},
> @A + 1, and @B + 2, but not for @A + 2.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-6388543
> Webrev: http://cr.openjdk.java.net/~cushon/6388543/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ecushon/6388543/webrev.00/>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Jonathan Gibbons
In reply to this post by Liam Miller-Cushon


On 01/03/2017 01:21 PM, Liam Miller-Cushon wrote:

> Hello,
>
> The JavacMessager API allows a message to be generated for a source
> position defined by a combination of Element + AnnotationMirror +
> AnnotationValue. The AnnotationValue is currently ignored, and the
> position is defined using only the Element and AnnotationMirror.
>
> This fix causes the AnnotationValue to be considered when calculating
> the diagnostic position. The value must be a direct value of the given
> annotation, or an element in an array initializer. Values of nested
> annotation mirrors are not supported.
>
> For example, given `@A(x = {1}, y = @B(2))` positions will be
> calculated for the AnnotationMirror + AnnotationValue pairs @A + {1},
> @A + 1, and @B + 2, but not for @A + 2.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-6388543
> Webrev: http://cr.openjdk.java.net/~cushon/6388543/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ecushon/6388543/webrev.00/>


I guess I am slightly surprised by the proposed solution, as regards the
choice of what it means to find a match. I have always assumed that any
implementation would iterate over the element values of the annotation,
looking for a value that is .equals to the given value, for some
appropriate definition of AnnotationValue.equals.  It does not seem
obvious to me that you should be searching within the individual element
values for the match. I am also surprised that you might match for @B +
2; I would expect the annotation mirror to only match at the top
level.   For example, assuming a "first one wins" policy, I would have
expected @B + 2 to match the second annotation in the pair of
annotations `@A(x = {1}, y = @B(2)) @B(2)`

So, before getting into the details of the proposed code, I think it
would be worthwhile to determine what the semantics of this somewhat
loosely-specified "hint" should actually be.

-- Jon




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Liam Miller-Cushon
Thanks for the feedback,

On Tue, Jan 3, 2017 at 9:00 PM, Jonathan Gibbons <[hidden email]> wrote:
I guess I am slightly surprised by the proposed solution, as regards the choice of what it means to find a match. I have always assumed that any implementation would iterate over the element values of the annotation, looking for a value that is .equals to the given value, for some appropriate definition of AnnotationValue.equals.  It does not seem obvious to me that you should be searching within the individual element values for the match. I am also surprised that you might match for @B + 2; I would expect the annotation mirror to only match at the top level.   For example, assuming a "first one wins" policy, I would have expected @B + 2 to match the second annotation in the pair of annotations `@A(x = {1}, y = @B(2)) @B(2)`

I thought the proposed approach was consistent with the existing handling of AnnotationMirror. The overloads of matchAnnoToTree use reference equality to find a matching AnnotationMirror, which could be either a top-level or nested annotation on the element. So given `@A(x = {1}, y = @B(2)) @B(2)`, passing an AnnotationMirror for @B to printMessage(..., Element, AnnotationMirror) could match either appearance of @B depending on which instance the argument was.

Is there a definition of AnnotationValue.equals that would be more appropriate than reference equality? Value equality would be ambiguous for e.g. `@A(x=1, y=1, z=1)` and @A + 1.

The motivation for supporting array elements was the same one mentioned in the bug [1], which I've heard repeated elsewhere: for annotations containing large array initializers it'd be helpful to place the caret at a specific element instead of the start of the array.

Liam

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Liam Miller-Cushon
Any more thoughts on this? I'm happy to revise the approach if there's a better way to use the hint.

On Tue, Jan 3, 2017 at 10:28 PM, Liam Miller-Cushon <[hidden email]> wrote:
Thanks for the feedback,

On Tue, Jan 3, 2017 at 9:00 PM, Jonathan Gibbons <[hidden email]> wrote:
I guess I am slightly surprised by the proposed solution, as regards the choice of what it means to find a match. I have always assumed that any implementation would iterate over the element values of the annotation, looking for a value that is .equals to the given value, for some appropriate definition of AnnotationValue.equals.  It does not seem obvious to me that you should be searching within the individual element values for the match. I am also surprised that you might match for @B + 2; I would expect the annotation mirror to only match at the top level.   For example, assuming a "first one wins" policy, I would have expected @B + 2 to match the second annotation in the pair of annotations `@A(x = {1}, y = @B(2)) @B(2)`

I thought the proposed approach was consistent with the existing handling of AnnotationMirror. The overloads of matchAnnoToTree use reference equality to find a matching AnnotationMirror, which could be either a top-level or nested annotation on the element. So given `@A(x = {1}, y = @B(2)) @B(2)`, passing an AnnotationMirror for @B to printMessage(..., Element, AnnotationMirror) could match either appearance of @B depending on which instance the argument was.

Is there a definition of AnnotationValue.equals that would be more appropriate than reference equality? Value equality would be ambiguous for e.g. `@A(x=1, y=1, z=1)` and @A + 1.

The motivation for supporting array elements was the same one mentioned in the bug [1], which I've heard repeated elsewhere: for annotations containing large array initializers it'd be helpful to place the caret at a specific element instead of the start of the array.

Liam


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Jonathan Gibbons
Hi Liam,

I have reviewed the spec for the method, the existing code, the bug report and its comments, and your notes in this email thread.

I note that reference equality is being used, and I agree it seems to give the best semantics, as regards avoiding ambiguities. I also note that the search for the annotation mirror is recursive. Although not functionally necessary, this makes me wonder whether we should not also search recursively for the annotation value. That would seem to give the best flexibility to the user, allowing them to find a match for @A + 2, to use the example in this thread.

Is there any good reason not to do so?

-- Jon


On 01/12/2017 11:23 AM, Liam Miller-Cushon wrote:
Any more thoughts on this? I'm happy to revise the approach if there's a better way to use the hint.

On Tue, Jan 3, 2017 at 10:28 PM, Liam Miller-Cushon <[hidden email]> wrote:
Thanks for the feedback,

On Tue, Jan 3, 2017 at 9:00 PM, Jonathan Gibbons <[hidden email]> wrote:
I guess I am slightly surprised by the proposed solution, as regards the choice of what it means to find a match. I have always assumed that any implementation would iterate over the element values of the annotation, looking for a value that is .equals to the given value, for some appropriate definition of AnnotationValue.equals.  It does not seem obvious to me that you should be searching within the individual element values for the match. I am also surprised that you might match for @B + 2; I would expect the annotation mirror to only match at the top level.   For example, assuming a "first one wins" policy, I would have expected @B + 2 to match the second annotation in the pair of annotations `@A(x = {1}, y = @B(2)) @B(2)`

I thought the proposed approach was consistent with the existing handling of AnnotationMirror. The overloads of matchAnnoToTree use reference equality to find a matching AnnotationMirror, which could be either a top-level or nested annotation on the element. So given `@A(x = {1}, y = @B(2)) @B(2)`, passing an AnnotationMirror for @B to printMessage(..., Element, AnnotationMirror) could match either appearance of @B depending on which instance the argument was.

Is there a definition of AnnotationValue.equals that would be more appropriate than reference equality? Value equality would be ambiguous for e.g. `@A(x=1, y=1, z=1)` and @A + 1.

The motivation for supporting array elements was the same one mentioned in the bug [1], which I've heard repeated elsewhere: for annotations containing large array initializers it'd be helpful to place the caret at a specific element instead of the start of the array.

Liam



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Liam Miller-Cushon
On Thu, Jan 12, 2017 at 1:46 PM, Jonathan Gibbons <[hidden email]> wrote:
Is there any good reason not to do so?

I don't think so, thanks. I had thought that it would add implementation complexity and wasn't as obviously useful as supporting top-level and array elements values. However I realized that matchAnnoToTree can be generalized to search recursively for values, so it ends up being simpler. And as you point out, it offers the best flexibility.

I have uploaded a new version of the patch that searches recursively:
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Liam Miller-Cushon
Hi, have you had a chance to look at the latest version of the patch?

And would it make sense to defer to 10?

On Sat, Jan 14, 2017 at 3:37 PM, Liam Miller-Cushon <[hidden email]> wrote:
On Thu, Jan 12, 2017 at 1:46 PM, Jonathan Gibbons <[hidden email]> wrote:
Is there any good reason not to do so?

I don't think so, thanks. I had thought that it would add implementation complexity and wasn't as obviously useful as supporting top-level and array elements values. However I realized that matchAnnoToTree can be generalized to search recursively for values, so it ends up being simpler. And as you point out, it offers the best flexibility.

I have uploaded a new version of the patch that searches recursively:

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Jonathan Gibbons


On 02/06/2017 10:45 AM, Liam Miller-Cushon wrote:
Hi, have you had a chance to look at the latest version of the patch?

And would it make sense to defer to 10?

On Sat, Jan 14, 2017 at 3:37 PM, Liam Miller-Cushon <[hidden email]> wrote:
On Thu, Jan 12, 2017 at 1:46 PM, Jonathan Gibbons <[hidden email]> wrote:
Is there any good reason not to do so?

I don't think so, thanks. I had thought that it would add implementation complexity and wasn't as obviously useful as supporting top-level and array elements values. However I realized that matchAnnoToTree can be generalized to search recursively for values, so it ends up being simpler. And as you point out, it offers the best flexibility.

I have uploaded a new version of the patch that searches recursively:


http://cr.openjdk.java.net/~cushon/6388543/webrev.01/test/tools/javac/processing/messager/6388543/T6388543.java.html
Nit: in the @modules, you don't need to specify java.compiler as well as jdk.compiler, since the latter requires the former.
Nit: it is not common to see javax.* imports sorted before java.* imports.

Otherwise, looks OK.

I think this is small and safe enough for 9.

-- Jon

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Liam Miller-Cushon
Thanks for the review! I fixed the nits, here's the latest version:


The changeset is attached.

On Mon, Feb 6, 2017 at 3:40 PM, Jonathan Gibbons <[hidden email]> wrote:


On 02/06/2017 10:45 AM, Liam Miller-Cushon wrote:
Hi, have you had a chance to look at the latest version of the patch?

And would it make sense to defer to 10?

On Sat, Jan 14, 2017 at 3:37 PM, Liam Miller-Cushon <[hidden email]> wrote:
On Thu, Jan 12, 2017 at 1:46 PM, Jonathan Gibbons <[hidden email]> wrote:
Is there any good reason not to do so?

I don't think so, thanks. I had thought that it would add implementation complexity and wasn't as obviously useful as supporting top-level and array elements values. However I realized that matchAnnoToTree can be generalized to search recursively for values, so it ends up being simpler. And as you point out, it offers the best flexibility.

I have uploaded a new version of the patch that searches recursively:


http://cr.openjdk.java.net/~cushon/6388543/webrev.01/test/tools/javac/processing/messager/6388543/T6388543.java.html
Nit: in the @modules, you don't need to specify java.compiler as well as jdk.compiler, since the latter requires the former.
Nit: it is not common to see javax.* imports sorted before java.* imports.

Otherwise, looks OK.

I think this is small and safe enough for 9.

-- Jon



6388543.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 6388543: improve accuracy of source positions for AnnotationValue param of Messager.printMessage

Jonathan Gibbons
reviewed, built, tested and pushed

-- Jon

On 02/06/2017 05:51 PM, Liam Miller-Cushon wrote:
Thanks for the review! I fixed the nits, here's the latest version:


The changeset is attached.

On Mon, Feb 6, 2017 at 3:40 PM, Jonathan Gibbons <[hidden email]> wrote:


On 02/06/2017 10:45 AM, Liam Miller-Cushon wrote:
Hi, have you had a chance to look at the latest version of the patch?

And would it make sense to defer to 10?

On Sat, Jan 14, 2017 at 3:37 PM, Liam Miller-Cushon <[hidden email]> wrote:
On Thu, Jan 12, 2017 at 1:46 PM, Jonathan Gibbons <[hidden email]> wrote:
Is there any good reason not to do so?

I don't think so, thanks. I had thought that it would add implementation complexity and wasn't as obviously useful as supporting top-level and array elements values. However I realized that matchAnnoToTree can be generalized to search recursively for values, so it ends up being simpler. And as you point out, it offers the best flexibility.

I have uploaded a new version of the patch that searches recursively:


http://cr.openjdk.java.net/~cushon/6388543/webrev.01/test/tools/javac/processing/messager/6388543/T6388543.java.html
Nit: in the @modules, you don't need to specify java.compiler as well as jdk.compiler, since the latter requires the former.
Nit: it is not common to see javax.* imports sorted before java.* imports.

Otherwise, looks OK.

I think this is small and safe enough for 9.

-- Jon



Loading...