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

[Jan Lahoda]

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