RFR: JDK-8230105: -XDfind=diamond crashes

[Vicente Romero]

Hi Jan,

I like the patch, I think it is very sensible. I just have a cosmetic 
comment. I think that rebranding speculative attribution to a concept 
based on its side-effects could bring confusion in the future as the 
concept of speculative attribution is very deep in the understanding we 
have of the attribution process. So what I'm proposing is just a change 
in the name to the three states you defined, just a proposal:

enum AttributionMode {
     NON_SPECULATIVE(false),
     ANALYZER(true)
     SPECULATIVE(true);

     AttributionMode(boolean isSpeculative) {
         this.isSpeculative = isSpeculative;
     }

     boolean isSpeculative() {
         return isSpeculative;
     }

     final isSpeculative;
}

we can work on the names but my proposal is to put the speculative-ness 
of the attribution foremost and the side-effect-ness in second place. 
What do you think?

Vicente

On 8/26/19 6:50 AM, Jan Lahoda wrote:
> Hi,
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8230105
> Proposed webrev: http://cr.openjdk.java.net/~jlahoda/8230105/webrev.00/
>
> Consider this code:
> ---
> public class AnalyzerNotQuiteSpeculative {
>     private void test() {
>         Subclass1 c1 = null;
>         Subclass2 c2 = null;
>         Base b = null;
>
>         t(new C<Base>(c1).set(c2));
>         t(new C<Base>(b).set(c2));
>     }
>
>     public static class Base {}
>     public static class Subclass1 extends Base {}
>     public static class Subclass2 extends Base {}
>     public class C<T extends Base> {
>         public C(T t) {}
>         public C<T> set(T t) { return this; }
>     }
>     <T extends Base> void t(C<? extends Base> l) {}
> }
> ---
>
> When run with -XDfind=diamond, this crashes with:
> ---
>
>     at 
> jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:162)
>     at 
> jdk.compiler/com.sun.tools.javac.comp.Analyzer.doAnalysis(Analyzer.java:581)
> ...
> ---
>
> The embedded (thrown away) exception is:
> ---
> java.lang.NullPointerException
>     at 
> jdk.compiler/com.sun.tools.javac.comp.Analyzer$DiamondInitializer.process(Analyzer.java:261)
>     at 
> jdk.compiler/com.sun.tools.javac.comp.Analyzer$DiamondInitializer.process(Analyzer.java:225)
>     at 
> jdk.compiler/com.sun.tools.javac.comp.Analyzer.doAnalysis(Analyzer.java:579)
>     at 
> jdk.compiler/com.sun.tools.javac.comp.Analyzer$2.flush(Analyzer.java:552)
>     at 
> jdk.compiler/com.sun.tools.javac.comp.Analyzer.flush(Analyzer.java:594)
>     at 
> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1412)
>     at 
> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1380)
>     at 
> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:972)
>     at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:318)
>     at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
>     at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:57)
>     at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:43)
> ---
>
> The reason for this exception seems to be that when the Analyzer is 
> speculatively analyzing "t(new C<Base>(c1).set(c2));", 
> ArgumentAttr.setResult will set a deferred type to "new 
> C<Base>(c1).set(c2)" (because isSpeculative is true), which in turn 
> will prevent the tree from being actually attributed. So when the 
> analyzer asks for type of "new C<Base>(c1)", it gets null.
>
> It ultimate problem appears to be that analyzers want to analyze a 
> piece of AST "as if" it was part of the source code, but without 
> having permanent side effects. But, currently, there is only the 
> AttrContext.isSpeculative boolean, which means both "no permanent 
> sideeffects" and "the tree is a scratch tree, that can be modified in 
> any way", and the latter is not quite the case when doing spec 
> attribution for analyzers.
>
> The proposed solution is to make the isSpeculative flag 3 tristate (by 
> using enum and renaming the field), which should allow us to 
> differentiate between "actual AST", "AST in analyzer" and "actual 
> speculative AST".
>
> I also created a patch that runs analyzers on all source code which 
> building and testing JDK, and fixed a few smallish trouble it found. 
> The patch to run the analyzers is not part of the proposed patch, but 
> can be seen here:
> http://cr.openjdk.java.net/~jlahoda/8230105/webrev.runall/
>
> How does this look?
>
> Thanks,
>     Jan