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

[bsrbnd]

Thanks,
Your changeset (http://hg.openjdk.java.net/jdk9/dev/langtools/rev/d034f4347b09)
looks fine:-)

bsrbnd

2015-10-05 5:09 GMT+02:00 Srikanth <srikanth.adayapalam at oracle.com>:
>
>
> 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
>>>>
>>>>
>>
>