[PATCH] 8133135: NPE on anonymous class defined by qualified instance creation expression with diamond

[Srikanth]

On Monday 07 September 2015 04:22 PM, Srikanth wrote:
> Hello !
>
> Thanks for the patch. I can help take it forward. (I implemented
> <> with anonymous classes - I have been away on leave for some
> time and expect to have some cycles beginning this week.)

Hello, sorry for the delay, I am finally back online full time to look into
this issue.

Unfortunately, the proposed patch fails some of the diamond tests so
cannot be accepted as is - I'll investigate a fix and take it forward.

Thanks!
Srikanth

>
> Thanks!
> Srikanth
>
> On Saturday 05 September 2015 08:49 PM, bsrbnd wrote:
>> Just a little precision;
>> clazz is reassigned with clazzid1 in method Attr.visitNewClass() (on
>> line 1973) which implies that it differs from tree.clazz:
>>
>>          if (tree.encl != null) {
>>              // We are seeing a qualified new, of the form
>>              //    <expr>.new C <...> (...) ...
>>
>>              [...]
>>
>>              clazz = clazzid1;
>>          }
>>
>>
>> It is then passed to Attr.visitAnonymousClassDefinition() and used to
>> perform type inference.
>> This is why recurcivity condition could be made on clazz.type instead
>> of tree.clazz.type as shown in the first patch.
>>
>> But, I was also wondering if tree.clazz should really differs from
>> clazz as we assign clazz.type to tree.clazz.type in
>> Attr.visitNewClass() on line 2088 before calling
>> Attr.visitAnonymousClassDefinition()?
>>
>>                  if (!constructorType.isErroneous()) {
>>                      tree.clazz.type = clazz.type =
>> constructorType.getReturnType();
>>                      tree.constructorType =
>> types.createMethodTypeWithReturn(constructorType, syms.voidType);
>>                  }
>>
>> If it shouldn't, tree.clazz have to match clazz and then recurcivity
>> condition on tree.clazz.type could be right...
>> This leads me to give next a second patch (only in
>> Attr.visitNewClass()) with a slightly different meaning as explained
>> above:
>>
>> diff --git 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> @@ -1970,7 +1970,7 @@
>>                                 ((JCTypeApply) clazz).arguments);
>>               }
>>
>> -            clazz = clazzid1;
>> +            tree.clazz = clazz = clazzid1;
>>           }
>>
>>           // Attribute clazz expression and store
>>
>> Regards,
>> bsrbnd
>>
>> 2015-08-31 16:32 GMT+02:00 Maurizio Cimadamore 
>> <maurizio.cimadamore at oracle.com>:
>>> Thanks, I'll take a look.
>>>
>>> Maurizio
>>>
>>>
>>> On 31/08/15 14:40, bsrbnd wrote:
>>>> Hi,
>>>>
>>>> As explained in issue 8133135, the following code produces a null
>>>> pointer exception because the symbol of the anonymous class is not set
>>>> during the attribute phase:
>>>>
>>>> class List<T> {}
>>>>
>>>> class ClassOut {
>>>>       class ClassIn<T> { public ClassIn() {}}
>>>> }
>>>>
>>>> public class TestBug {
>>>>       public static <T> List<T> m(List<T> list, T item) {return list;}
>>>>
>>>>
>>>>       public static void main(String[] args) {
>>>>           m(new List<ClassOut.ClassIn<String>>(), new ClassOut().new
>>>> ClassIn<>(){});
>>>>       }
>>>> }
>>>>
>>>> Method Attr.visitAnonymousClassDefinition() doesn't perform well type
>>>> inference with diamond. Recurcivity condition on tree.clazz.type seems
>>>> to be wrong because it never changes... instead, it should be made on
>>>> clazz.type (or clazztype) to go deeper on the type resolution.
>>>>
>>>> The following patch shows a possible solution, I think (to be checked
>>>> by someone who knows the code better than me...):
>>>>
>>>> diff --git
>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>>>> --- 
>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>>>> +++ 
>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>>>> @@ -2173,7 +2173,7 @@
>>>>                final boolean isDiamond = TreeInfo.isDiamond(tree);
>>>>                if (isDiamond
>>>>                        && ((tree.constructorType != null &&
>>>> inferenceContext.free(tree.constructorType))
>>>> -                    || (tree.clazz.type != null &&
>>>> inferenceContext.free(tree.clazz.type)))) {
>>>> +                    || (clazz.type != null &&
>>>> inferenceContext.free(clazz.type)))) {
>>>>                    final ResultInfo resultInfoForClassDefinition =
>>>> this.resultInfo;
>>>>
>>>> inferenceContext.addFreeTypeListener(List.of(tree.constructorType,
>>>> tree.clazz.type),
>>>>                            instantiatedContext -> {
>>>>
>>>> Regards,
>>>>
>>>> bsrbnd
>>>
>