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

[Maurizio Cimadamore]

Looks great!

Thanks
Maurizio

On 30/11/2018 17:11, Jan Lahoda wrote:
> 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