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

[Maurizio Cimadamore]

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?

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