RFR: JDK-8230105: -XDfind=diamond crashes

[Maurizio Cimadamore]

I agree with Vicente, maybe replacing NON_SPECULATIVE with NORMAL (or 
FULL) :-)

Maurizio

On 28/08/2019 20:03, Vicente Romero wrote:
> 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
>