RFR: JDK-8198945: Invalid RuntimeVisibleTypeAnnotations for annotation on anonymous class type parameter

[Werner Dietl]

Hi Liam, Alex, all,

it would be nice if there were a correspondence between the type of
the object that is created and the NEW type annotations that we
generate.

For method f() from the latest example, we generate bytecode:

    Code:
      stack=3, locals=1, args_size=1
         0: new           #2                  // class Test$1
         ...
         9: new           #4                  // class Test$J

Class Test$1 has no type arguments, so I would find having NEW type
annotations on a type argument confusing.
In contrast, class Test$J takes type arguments, so having a NEW type
annotation on that type argument is sensible.

So I would be for either no NEW type annotations for anonymous class
instantiations, or duplicating only the top-level type annotations.
For method f() this would generate three type annotations:

      RuntimeInvisibleTypeAnnotations:
        0: #22(): NEW, offset=9
          Test$TA
        1: #23(): NEW, offset=9, location=[TYPE_ARGUMENT(0)]
          Test$TB
        2: #22(): NEW, offset=0
          Test$TA

Duplicating only the top-level type annotations would be my slight
preference over no NEW annotations at all.

This is what I had suggested in the 2/12/2013 message quoted in
http://mail.openjdk.java.net/pipermail/type-annotations-spec-experts/2013-February/000063.html
and still seems sensible to me.

Best,
cu, WMD.
On Mon, Jul 2, 2018 at 8:43 PM Liam Miller-Cushon <cushon at google.com> wrote:
>
> Hi Alex,
>
> Thanks for digging up those threads, I hadn't seen the earlier discussions.
>
> I do not have a strong opinion about the decision to emit annotations on anonymous class supertypes as both CLASS_EXTENDS and NEW, vs. just CLASS_EXTENDS. Unless others on this thread are interested in revisiting it, I'm now leaning towards keeping the current approach.
>
> Here's the updated webrev to do that: http://cr.openjdk.java.net/~cushon/8198945/webrev.01/index.html
>
> The patch to Annotate is still necessary to prevent annotations like @TB from being attached to the enclosing method (rather than the code attribute). The revised patch to TypeAnnotations keeps the copyNewClassAnnotationsToOwner helper that propagates annotations on anonymous classes to the 'new' expression, but fixes it to preserve the relative position information of annotations that are not on the top-level type.
>
> Here's a demo. Note that the 'NEW' annotations are identical for both the anonymous and named new expressions:
>
> ```
> import java.lang.annotation.*;
>
> class Test {
>   @Target(ElementType.TYPE_USE) @interface TA {}
>   @Target(ElementType.TYPE_USE) @interface TB {}
>
>   interface I<T> {}
>   static class J<T> {}
>
>   void f() {
>     new @TA I<@TB Object>() {};
>     new @TA J<@TB Object>();
>   }
> }
> ```
>
> $ javap -v -p Test
>   void f();
> ...
>       RuntimeInvisibleTypeAnnotations:
>         0: #22(): NEW, offset=9
>           Test$TA
>         1: #23(): NEW, offset=9, location=[TYPE_ARGUMENT(0)]
>           Test$TB
>         2: #23(): NEW, offset=0, location=[TYPE_ARGUMENT(0)]
>           Test$TB
>         3: #22(): NEW, offset=0
>           Test$TA
>
> $ javap -v -p 'Test$1'
> ...
> RuntimeInvisibleTypeAnnotations:
>   0: #21(): CLASS_EXTENDS, type_index=0, location=[TYPE_ARGUMENT(0)]
>     Test$TB
>   1: #24(): CLASS_EXTENDS, type_index=0
>     Test$TA
>
>
> On Fri, Jun 29, 2018 at 4:39 PM Alex Buckley <alex.buckley at oracle.com> wrote:
>>
>> On 6/29/2018 3:55 PM, Liam Miller-Cushon wrote:
>> > The handling of @TB in particular is clearly incorrect:
>> >
>> > * @TB appears with a `NEW` type path that leads to the top-level type of
>> > the 'new' expression, even though @TB appeared on a type argument of
>> > that type and not at the top level.
>> >
>> > * @TB also appears on 'f' with a `CLASS_EXTENDS` path and no bytecode
>> > offset, which does not provide enough information to associate it with
>> > the annotated location.
>>
>> I agree with both points. It is egregious that the method_info structure
>> for `f` (as opposed to the Code attribute therein) as a
>> Runtime*TypeAnnotations attribute.
>>
>> > The handling of @TA is also questionable. It makes some sense to use the
>> > same type path for an annotation on the type of an anonymous class
>> > declaration as the type of a regular 'new' expression. However that
>> > results in the same use of @TA appearing twice in bytecode, and
>> > conflates the type of the anonymous class with its supertype.
>> >
>> > I think it is more correct to only emit @TA as a `CLASS_EXTENDS`
>> > annotation on Test$1, and not include a copy of it at the enclosing method.
>>
>> I did some digging and found three threads from 2013:
>>
>> 1. "Annotations on anonymous classes"
>> http://mail.openjdk.java.net/pipermail/type-annotations-spec-experts/2013-February/000063.html
>>
>> 2. "Desugaring of anonymous classes"
>> http://mail.openjdk.java.net/pipermail/type-annotations-spec-experts/2013-April/000092.html
>>
>> 3. "Desugaring of anonymous classes" (followup)
>> http://mail.openjdk.java.net/pipermail/type-annotations-spec-experts/2013-August/000127.html
>>
>> It seems like I changed my mind on the matter of recording @TA:
>>
>> - In February I agreed with your view, Liam, that @TA should appear only
>> as a CLASS_EXTENDS path in the implicit class declaration (Test$1), and
>> not as a NEW path in the code of `f`.
>>
>> - But in April, I said that @TA should be recorded as both paths. Per
>> the followup mail in August, this was driven by a longstanding policy
>> that "In the classfile, the type annotations are copied to the class
>> declaration and to its instantiation (new expression)."
>>
>> In June 2018, I do not mind whether @TA appears as CLASS_EXTENDS only,
>> or as CLASS_EXTENDS + NEW. If you and Werner have agreed on
>> CLASS_EXTENDS only, and if your fix (which is mostly deletion) makes the
>> recording of both @TA and @TB more reliable, then I defer to your decision.
>>
>> Alex



-- 
http://www.google.com/profiles/wdietl