RFR[9]: 8147585: Annotations with lambda expressions has parameters result in wrong behavior.

[shilpi.rastogi at oracle.com]

Hi All,

Please see updated webrev 
http://cr.openjdk.java.net/~srastogi/8147585/webrev.02/

On 5/31/2016 2:21 PM, Paul Sandoz wrote:
>> On 31 May 2016, at 10:35, shilpi.rastogi at oracle.com wrote:
>>
>> Thanks Paul for comments.
>>
>> Please see http://cr.openjdk.java.net/~srastogi/8147585/webrev.01/
>>
>> Now processing only public abstract methods of interface.
>>
> Thanks. It would be good to get some got feedback from those wiser than I in this regard.
>
> Have you looked at the existing annotation-based tests to see if they test edge cases e.g. annotation classes generated with incorrect methods? that might give us some clues.
   I saw existing annotation-based test, valid modifier for annotations, 
valid method for annotation tests we are checking in javac code.
   default, static, private modifier we are restricting at compile time 
so we can not add test cases for this  (compilation will fail).

So only scenario i can add is for synthetic methods. ( according to my 
assumption)
In AnnotationType.java

public Method[] run() {
      // Initialize memberTypes and defaultValues
      return annotationClass.getDeclaredMethods();
   }
});

As here, calling getDeclaredMethods() on annotationclass and 
getDeclaredMethods() doc says

https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getDeclaredMethods--
"Returns an array containing Method objects reflecting all the declared 
methods of the class or interface represented by this Class object, 
including public, protected, default (package) access, and private 
methods, but excluding inherited methods."

Doc did not mention anything about synthetic methods so i am not sure 
this is expected behavior or not.
If yes, Could you please suggest how to add testcases to test synthetic 
method?
Shall I use ASM?

Regards,
Shilpi

>
> Testing wise:
>
> - you can avoid the filtering of the test method by placing the annotations in another auxiliary class (my preference is to nest rather than using auxiliary classes, up to you)
>
> - there is a couple of minor style inconsistencies e.g. a space here "@ interface LambdaWithoutParameter"
>
> - 30  * @run testng/othervm -ea -esa AnnotationWithLambda
>
> You can just do:
>
>    @run testng AnnotationWithLambda
>
> ?
>
> Thanks,
> Paul.
>
>
>
>
>> Thanks,
>> Shilpi
>>
>> On 5/30/2016 6:35 PM, Paul Sandoz wrote:
>>> Hi Shilpi,
>>>
>>> You have found the right place but i am not sure your fix is entirely correct.
>>>
>>> (Tip: if you use  -Xlog:exceptions=info you can observe the IAE exception when the annotation is processed)
>>>
>>> In your test you have:
>>>
>>> @Target(value = ElementType.METHOD)
>>> @Retention(RetentionPolicy.RUNTIME)
>>> @ interface LambdaWithParameter {
>>>      Consumer<Integer> f1 = a -> {
>>>          System.out.println("lambda has parameter");
>>>      };
>>> }
>>>
>>> @Target(value = ElementType.METHOD)
>>> @Retention(RetentionPolicy.RUNTIME)
>>> @ interface LambdaWithoutParameter {
>>>      Runnable r = () -> System.out.println("lambda without parameter");
>>> }
>>>
>>>
>>> Both of those annotations will have static synthetic methods generated by the compiler that the indy resolves and links to (look at the javap output). The former will have a method with one parameter.
>>>
>>>
>>> The code in sun/reflect/annotation/AnnotationType.java:
>>>
>>> for (Method method : methods) {
>>>      if (method.getParameterTypes().length != 0)
>>>          throw new IllegalArgumentException(method + " has params");
>>>
>>> has thrown the IAE since 2004, but it’s not clear why it was added as opposed to something more general (see below).
>>>
>>>
>>> The correct fix appears to be to skip over any non-abstract/non-public methods. Thus only public abstract methods get processed.
>>>
>>> Your current fix now processes synthetic methods with parameters, in addition to those which were already processed such as synthetic methods without parameters, or even private methods that could have been generated by some tool. I dunno how much say the verifier has in all this, perhaps little or no say.
>>>
>>> Thus non-public/non-abstract methods could add inconsistent information to the data structures of AnnotationType. Perhaps this is mostly harmless?
>>>
>>> Perhaps Joel (CC’ed) can she some more light on this?
>>>
>>> Paul.
>>>
>>>
>>>> On 30 May 2016, at 08:00, shilpi.rastogi at oracle.com wrote:
>>>>
>>>> Hi All,
>>>>
>>>> Please review fix for https://bugs.openjdk.java.net/browse/JDK-8147585
>>>> http://cr.openjdk.java.net/~srastogi/8147585/webrev.00/
>>>>
>>>> Problem: Annotation with Lamda has parameters results into wrong behavior ( not considered as valid annotation) because
>>>> According to JLS "By virtue of the AnnotationTypeElementDeclaration production, a method declaration in an annotation type declaration cannot have formal parameters, type parameters, or a throws clause. The following production from §4.3 is shown here for convenience:"
>>>> (Ref: https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.6.1)
>>>>
>>>>
>>>> Solution: We should skip synthetic methods during Annotation parsing. According to JLS "An annotation type has no elements other than those defined by the methods it explicitly declares."
>>>> (Ref https://docs.oracle.com/javase/specs/jls/se8/html/.html#jls-9jls-9.6.1)
>>>>
>>>>
>>>> Thanks,
>>>> Shilpi