RFR: JDK-8230105: -XDfind=diamond crashes

[Maurizio Cimadamore]

Looks good - I've checked all the usages of the tri-state flag and they 
look good (and I noted the actual resolution of this issue, which is to 
tweak the check in ArgumentAttr.setResult).

Maurizio

On 29/08/2019 09:53, Jan Lahoda wrote:
> 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
>>>>
>>