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

[Maurizio Cimadamore]

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.

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