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

[Maurizio Cimadamore]

Looks good. Thanks.

Maurizio


On 26/09/18 09:43, Jan Lahoda wrote:
> 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
>>>>
>>