JDK-8189335: NPE in Lower due to class name clash

[Vicente Romero]

Hi Bernard,

Usually when we touch this area of the compiler we do the following 
test: build the whole JDK, another big corpus could make it too, with 
and without the fix applied and then we compare the class files 
obtained, if no new tag classes are generated for cases where they 
shouldn't and if there are no unjustified changes to the rest of the 
classes, then we are more confident about the fix.

Thanks,
Vicente

On 02/22/2018 03:09 PM, B. Blaser wrote:
> On 21 February 2018 at 16:58, B. Blaser <bsrbnd at gmail.com> wrote:
>> On 19 February 2018 at 15:53, B. Blaser <bsrbnd at gmail.com> wrote:
>>> On 19 February 2018 at 00:42, B. Blaser <bsrbnd at gmail.com> wrote:
>>>> Hi,
>>>>
>>>> As explained in JDK-8189335, the following example crashes javac with
>>>> a NPE in Lower:
>>>>
>>>> class R {
>>>>      private class R$1 {}
>>>>      void f() {
>>>>          new R$1();
>>>>      }
>>>> }
>>>> class R$1 {}
>> Some precisions related to my laconic explanation about the problem and the fix.
>>
>> All starts in 'Lower.accessConstructorTag()' when searching for the
>> anonymous class 'R$1':
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l1238
>>
>> The top level class 'R$1' (not anonymous) is found and wrongly added
>> to 'accessConstrTags'.
>>
>> Then, when checking the access constructor tags in
>> 'Lower.checkAccessConstructorTags()', the 'outermostClass()' of the
>> top level class 'R$1' is requested:
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l778
>>
>> 'R$1' being a top level class, its outermost class doesn't exist,
>> causing the NPE in 'Lower.makeEmptyClass()'.
> Errata: 'R$1' being a top level class, its outermost class definition
> ('R$1') doesn't exist within 'R', causing the NPE in
> 'Lower.makeEmptyClass()':
>
> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l658
>
>> The fix I suggested two days ago (here under) is then quite
>> straightforward. If a class 'R$1' exists but isn't anonymous, it has
>> to be skipped and a fresh anonymous class 'R$2' has to be created as
>> access constructor tag:
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l1239
>>
>> Nothing seems to be broken and the provided example compiles successfully.
> Here is a small variant of the previous patch that makes sure to reuse
> the same tag when multiple private constructors are present and 'R$1'
> isn't anonymous, for example:
>
> class R {
>      private class A {}
>      private class B {}
>      void f() {
>          new A();
>          new B();
>      }
> }
> class R$1{}
>
> Any comment or review is welcome!
>
> Cheers,
> Bernard
>
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> @@ -1232,15 +1232,19 @@
>       ClassSymbol accessConstructorTag() {
>           ClassSymbol topClass = currentClass.outermostClass();
>           ModuleSymbol topModle = topClass.packge().modle;
> -        Name flatname = names.fromString("" + topClass.getQualifiedName() +
> -                                         target.syntheticNameChar() +
> -                                         "1");
> -        ClassSymbol ctag = chk.getCompiled(topModle, flatname);
> -        if (ctag == null)
> -            ctag = makeEmptyClass(STATIC | SYNTHETIC, topClass).sym;
> -        // keep a record of all tags, to verify that all are
> generated as required
> -        accessConstrTags = accessConstrTags.prepend(ctag);
> -        return ctag;
> +        for (int i = 1; ; i++) {
> +            Name flatname = names.fromString("" + topClass.getQualifiedName() +
> +                                            target.syntheticNameChar() +
> +                                            i);
> +            ClassSymbol ctag = chk.getCompiled(topModle, flatname);
> +            if (ctag == null)
> +                ctag = makeEmptyClass(STATIC | SYNTHETIC, topClass).sym;
> +            else if (!ctag.isAnonymous())
> +                continue;
> +            // keep a record of all tags, to verify that all are
> generated as required
> +            accessConstrTags = accessConstrTags.prepend(ctag);
> +            return ctag;
> +        }
>       }
>
>       /** Add all required access methods for a private symbol to
> enclosing class.
>
>> What do you think (I hope this is clearer)?
>>
>> Thanks,
>> Bernard
>>
>>
>>> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>> @@ -1236,7 +1236,7 @@
>>>                                            target.syntheticNameChar() +
>>>                                            "1");
>>>           ClassSymbol ctag = chk.getCompiled(topModle, flatname);
>>> -        if (ctag == null)
>>> +        if (ctag == null || !ctag.isAnonymous())
>>>               ctag = makeEmptyClass(STATIC | SYNTHETIC, topClass).sym;
>>>           // keep a record of all tags, to verify that all are
>>> generated as required
>>>           accessConstrTags = accessConstrTags.prepend(ctag);