RFR: 8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly

[Maurizio Cimadamore]

On Fri, 1 Sep 2023 10:02:20 GMT, Aggelos Biboudis <abimpoudis at openjdk.org> wrote:

> The `switch` structure is translated in `handleSwitch` where we rewrite pattern matching switches. In some occasions a switch has multiple cases with multiple patterns where the n-th case does not complete normally and falls through to the n+1-st case:
> 
> 
> switch (obj) {
>    case Integer _ when ((Integer) obj) > 0:
>    case String _ when !((String) obj).isEmpty():
>       return 1;
>    default:
>       return -1;
> }
> 
> 
> This PR addresses that by translating the second case correctly and also replicates the body of the latter to the former (which we can do because no bindings are introduced in either statement blocks by the definition of unnamed variables.).
> 
> Previously the code was translated into:
> 
> 
> switch (java.lang.runtime.SwitchBootstraps.typeSwitch(selector0$temp, index$1)) {
>     case 0:
>         Integer _;
>         if (!(true && ((Integer)obj) > 0)) {
>             index$1 = 1;
>             continue;
>         }
> 
>     case 1 when !((String)obj).isEmpty():
>         return 1;
> 
>     default:
>         return -1;
> }
> 
> 
> This PR adjusts the translation into:
> 
> 
> switch (java.lang.runtime.SwitchBootstraps.typeSwitch(selector0$temp, index$1)) {
> case 0:
>     Integer _;
>     if (!(true && ((Integer)obj) > 0)) {
>         index$1 = 1;
>         continue;
>     }
>     return 1;
> 
> case 1:
>     String _;
>     if (!((selector0$temp instanceof String || false) && (true && !((String)obj).isEmpty()))) {
>         index$1 = 2;
>         continue;
>     }
>     return 1;
> }

So, as we discussed offline, type tests are normally enforced in the switch bootstrap. The first time we hit the test, we know we have some object and that the first label expects an Integer, so we test for that. But if the `when` clause fails, javac tells us to try again from the second label. Now, while the proposed code to the generated code looks ok - I wonder if this condition shouldn't instead be detected by the switch bootstrap: e.g. we're asking it to start again from a `case` which expects a `String`, but we really have some other Object on our hands, so that label index should never be returned by the switch bootstrap, which should either pick another applicable label index (if it exists) or just pick the default.

This would allow us to keep the generated code as is, and avoid introducing coupling between cases in the generated code.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15532#issuecomment-1702557839