RFR: JDK-8230105: -XDfind=diamond crashes

[Vicente Romero]

looks good,

Thanks,
Vicente

On 8/29/19 6:13 AM, Maurizio Cimadamore wrote:
> 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
>>>>>
>>>