RFR; JDK-8214031: Assertion error in value break statement with conditional operator in switch expression

[Jan Lahoda]

Hi,

A patch updated according to the comments is here:
http://cr.openjdk.java.net/~jlahoda/8214031/webrev.01/

A delta patch from the previous patch is here:
http://cr.openjdk.java.net/~jlahoda/8214031/webrev.delta.00.01/

How does this look?

Thanks,
    Jan

On 29.11.2018 13:42, Jan Lahoda wrote:
> Hi Maurizio,
>
> Thanks for the comments. I'll work on these.
>
> Jan
>
> On 28.11.2018 13:12, Maurizio Cimadamore wrote:
>> Hi Jan,
>> the patch looks very good and, albeit complex, I don't think there's
>> another way to implement shortcircuiting of boolean switch expression
>> w/o going deep in Gen (which is what this patch does). So well done.
>>
>>  From a code perspective I have only minor comments, as the code is
>> generally well done:
>>
>> 1) In Flow, since now you are making AssignAnalyzer also extends from
>> BaseAnalyzer<PendingExit> (used to be BaseAnalyzer<AssignExit>), I think
>> there is no need now for having a type parameter in BaseAnalyzer. Let
>> all exits be PendingExit - then subclasses can come up with variants
>> (like AssignExit) when needed. But it seems like the 1-1 relationship
>> between analyzer type and exit type is gone and the code should probably
>> be more upfront about that.
>>
>> 2) The valueBreakHandler abstraction in Gen is nice, but I wonder if the
>> code would be clearer by encoding the desired logic directly in
>> visitBreak. That is, visitBreak could fetch the switch expression it
>> points to, and determine whether we are inside a 'cond' or not, and then
>> act accordingly. I think that, with a couple of Gen instance fields
>> (shared trueChain/falseChain), you could be able to get there. Not 100%
>> sure it will make the code better, but I thought it could be worth a try.
>>
>> 3) In the test DefiniteAssignment1.java (and also, but to lesser extent,
>> DefiniteAssignment1.java)  I guess it would also be nice to test some &&
>> condition in the break expression - e.g.
>>
>> true && (x = 1)
>>
>> Also, might be worth replacing the true with some other variable which
>> the compiler doesn't know to be true statically (as that affects code
>> generation).
>>
>> Cheers
>> Maurizio
>>
>> On 27/11/2018 20:21, Jan Lahoda wrote:
>>> Hi,
>>>
>>> javac does not handle switch expressions that return booleans
>>> correctly. According to the spec, javac should track definite
>>> [un]assignment when true/false for switch expressions, but fails to do
>>> so. Moreover, for more complex code, like:
>>>
>>> int v = ...;
>>> int x;
>>> boolean b = switch (v) {
>>>      case 0: x = 12; break true;
>>>      case 1: x = 13; break true;
>>>      default: break false;
>>> } && x == 12;
>>>
>>> The current switch expression desugaring is not powerful enough to
>>> generate valid bytecode for this input. The issue is that the current
>>> desugaring will translate the switch expression into code like:
>>> (
>>>  boolean $result;
>>>  switch (v) {
>>>      case 0: x = 12; $result = true; break;
>>>      case 1: x = 13; $result = true; break;
>>>      default: $result = false; break;
>>>  }
>>>  $result   //"return" expression
>>> ) && x == 12
>>>
>>> But by assigning the true/false result to a variable, we are loosing
>>> track of which variables are assigned (when true/false) and which are
>>> not. The bytecode needs to ensure the "x == 12" is jumped over when
>>> the switch expression's value is false, and that, I believe, is not
>>> possible when desugaring in Lower.
>>>
>>> So, the proposed fix is to avoid desugaring in Lower, letting switch
>>> expressions go to Gen and produce bytecode for them, including
>>> handling them in Gen.genCond for true/false switch expressions. This
>>> is mostly modelled after the conditional expression handling. Some
>>> generalizations in Lower are required as well, to handle String and
>>> enum switch expressions. Handling of definite assignment in Flow is
>>> fixed as well.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8214031
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8214031/webrev.00/
>>>
>>> Any feedback is welcome!
>>>
>>> Thanks,
>>>    Jan