RFR: JDK-8230105: -XDfind=diamond crashes

[Jan Lahoda]

Thanks for the comments!

Updated webrev using the AttributionMode.FULL/ANALYZER/SPECULATIVE:
http://cr.openjdk.java.net/~jlahoda/8230105/webrev.01/

Jan

On 28. 08. 19 22:02, Vicente Romero wrote:
> 
> 
> 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
>>>
>