RFR: JDK-8211102: Crash with -XDfind=lambda and -source 7

[Jan Lahoda]

On 25.9.2018 19:16, Maurizio Cimadamore wrote:
> What you say makes sense, but the tests do not reflect what you say in
> the email.
>
> First, none of the test seems to use -XDfind=lambda. That means that no
> test is actually checking that the condition described in 8211102 does
> not occur?
>
> Secondly, it seems like the issue you are describing with conditional
> happens w/ or w/o analyzer - e.g. this looks like a standalone issue,
> and using the analyzer bug to fix it seems misleading (similarly for the
> 3rd issue fixed by this patch).
>
> So, while I don't dispute any of the changes in the patch, I'm having
> hard time following which is which - can you please clarify?

The Analyzer testcase is what I've found first, and it is probably the 
most serious thing, as javac crashes on valid source (albeit with 
somewhat non-standard options). While looking at the reasons, I realized 
that a) the lambda analyzer runs with -source 7 (which is suspicious), 
and also b) that there is a crash that can be reproduced without using 
analyzer (which is also the root cause for the analyzer crash). So I 
went to fix both of these. The third problem, I've ran into it while 
trying to find a testcase without analyzer, so I went to fix it as well.

I can split the patch (and bug) into three parts if desired.

Thanks,
     Jan

>
> Maurizio
>
>
> On 25/09/18 16:38, Jan Lahoda wrote:
>> Hi,
>>
>> I'd like to ask for a review of changes that relate to code like:
>> ---
>> import java.util.*;
>>
>> public class AnalyzerBug {
>>     private void t(boolean b) {
>>         (b ? Collections.emptyList() : new Iterable<String>() { public
>> Iterator<String> iterator() { return null; } }).toString();
>>     }
>> }
>> ---
>>
>> When this code is compiled like:
>> $ javac -XDfind=lambda -source 7 /tmp/AnalyzerBug.java
>>
>> javac crashes in Analyzer. There are two immediate causes for the crash:
>> a) when AnalyzerModes are read form -XDfind, if an analyzer key is
>> explicitly mentioned there, the source level is not checked, and the
>> analyzer is enabled. So, in the case above, the analyzer with try to
>> convert the Iterable<String> to a lambda and process the source,
>> despite the -source 7. I suspect this is not intended, so proposing to
>> always disable an analyzer if it does not have a meaning in the
>> current source level.
>>
>> b) if there's a lambda in a standalone conditional (i.e. without a
>> target), in JDK 12 we try to attribute it anyway, but as a result, the
>> type of the lambda will be the recovery type. And the computation of
>> the type of the standalone conditional chokes on that, as the recovery
>> type does not have a Symbol. So proposing to make the lambda's type an
>> erroneous type in such cases (as was before 12), while still
>> attributing it.
>>
>> There's also related problem, when (very broken) code like this:
>>         int j = i instanceof ArrayList ? (ArrayList<String>) i : () ->
>> { return null; };
>>
>> is attributed, visitLambda will keep JCFunctionalExpression.target
>> unfilled, which seems fine as JCFunctionalExpression.getDescriptorType
>> is prepared for that (and there indeed is no reasonable). But
>> Attr.postAttr will fill it with unknownType, and then calls to
>> getDescriptorType (e.g. from Flow.FlowAnalyzer.visitLambda) lead to
>> FunctionDescriptorLookupError. Seems it might be better to keep the
>> field unfilled, which is what the proposed patch is doing. (This is
>> somewhat less important, as I am not aware of a way to reproduce this
>> without -XDshould-stop.at=FLOW.)
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211102
>> Webrev: http://cr.openjdk.java.net/~jlahoda/8211102/webrev.00/
>>
>> Any feedback is welcome!
>>
>> Thanks,
>>     Jan
>