RFR: JDK-8230105: -XDfind=diamond crashes

[Vicente Romero]

On 8/28/19 4:00 PM, Maurizio Cimadamore wrote:
> I agree with Vicente, maybe replacing NON_SPECULATIVE with NORMAL (or 
> FULL) :-)
yep I like FULL too
>
> Maurizio

Vicente
>
> 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
>>