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

[Maurizio Cimadamore]

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