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

[Jan Lahoda]

On 26.9.2018 00:38, Maurizio Cimadamore wrote:
>
>
> On 25/09/18 20:05, Jan Lahoda wrote:
>> 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.
> I'm ok with keeping them together, but I'd add a test with the -XDfind
> option, just in case. E.g. something that makes sure that we don't
> generate 'finder' messages for features that are not supported in a
> given source level.

Sure, updated webrev:
http://cr.openjdk.java.net/~jlahoda/8211102/webrev.01/

Thanks,
     Jan

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