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

[B. Blaser]

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);