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

[Aggelos Biboudis]

On Fri, 1 Sep 2023 11:06:37 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> > 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.
>>> 
>>> Btw, looking at the comment in SwitchBootstraps::createRepeatIndexSwitch, it seems that's already the case:
>>> 
>>> ```
>>> /*
>>>      * Construct test chains for labels inside switch, to handle switch repeats:
>>>      * switch (idx) {
>>>      *     case 0 -> if (selector matches label[0]) return 0; else if (selector matches label[1]) return 1; else ...
>>>      *     case 1 -> if (selector matches label[1]) return 1; else ...
>>>      *     ...
>>>      * }
>>>      */
>>> ```
>> 
>> Also, in the original test here:
>> https://bugs.openjdk.org/browse/JDK-8314226
>> 
>> It seems to me that the problem is slightly different - e.g. the client is passing a String, but the `when` clause doesn't seem to be applied by javac. Which seems to conflict with the original generated code you report above: if javac currently generates this:
>> 
>> 
>>  case 1 when !((String)obj).isEmpty():
>>         return 1;
>> 
>> 
>> How can javac go ahead with this case, considering that the provided string (in the original test) is empty?
>> 
>> I wonder if the real issue is that there is no real `when` guard emitted for the second `case` (because of javac getting confused with cascading).
>
>> I wonder if the real issue is that there is no real `when` guard emitted for the second `case` (because of javac getting confused with cascading).
> 
> To be clear, IMHO the generated code should be:
> 
> 
> 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 (!(true && !((String)obj).isEmpty())) {
>         index$1 = 2;
>         continue;
>     }
>     return 1;
> }

I think you are right @mcimadamore. We do not need to generate the extraneous condition in the `if`. Removed and updated the PR.

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

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