RFR: 8225559: assertion error at TransTypes.visitApply

[Srikanth Adayapalam]

On Thu, 1 Jul 2021 05:41:40 GMT, Vicente Romero <vromero at openjdk.org> wrote:

> Please review this PR that is fixing a bug in javac. Depending on variations of the input the compiler fails with an assertion error or with the infamous NPE. So for code like:
> 
> src/pkg/Bar.java:
> 
> package pkg;
> public abstract class Bar<T> {
>     protected Bar() {}
>     protected Bar(Class<?> c) {}
> }
> 
> src/Test.java:
> 
> import pkg.Bar;
> import java.util.function.Supplier;
> 
> class Test {
>     public void foo() {
>         supply(getSupplier(new Bar<>(){}));
>     }
> 
>     static <U> Supplier<U> getSupplier(Bar<U> t) {
>         return null;
>     }
> 
>     static <U> void supply(Supplier<U> supplier) {}
> }
> 
> see that the new class expression is defining an anonymous class which is invoking a protected constructor in the super class, all of this should just work, and it works if the user doesn't use diamond and instead the type parameters are provided explicitly. So what happens with the diamond?
> 
> The reason is deep into the speculative attribution machinery, oh boy.... So when we are dealing with diamond expression that define anonymous classes, the speculative attribution will at some point clone that expression but it will nuke its class definition basically in the expression: `new Bar<>(){}` the `{}` will be removed.
> 
> so we have:
> `new Bar<>(){}  (Original)`
> `new Bar<>()    (Copy1)`
> 
> but in order for the rest of the code to know that the original expression was actually defining an anonymous class, the compiler does a trick. Which can be seen at: `com.sun.tools.javac.tree.TreeMaker::SpeculativeNewClass` basically an anonymous class which overrides method `JCTree.JCNewClass::classDeclRemoved` is created. But the bug presented itself for cases when the speculative attribution needs to clone `Copy1` and now it doesn't have the `{}`, the class definition, for the cloning code to know that that expression was copied from `Original` which was defining an anonymous class. So we need to double check by making use of method JCTree.JCNewClass::classDeclRemoved which is what this fix is doing.
> 
> Also it is important to know if the new class expression was defining an anonymous inner class or not because that makes the difference in case the new class expression is accessing a protected constructor as in this case.
> 
> TIA

Looks reasonable, 

+1 assuming sanity check done against original test case (and not just the simplified stripped down test case)

-------------

Marked as reviewed by sadayapalam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4647