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

[Maurizio Cimadamore]

On Fri, 1 Sep 2023 15:17:15 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;
>> }
>
> Aggelos Biboudis has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Cleanup

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java line 546:

> 544:                 }
> 545: 
> 546:                 if (validCaseLabelList && !previousCompletesNormally || c.guard != null) {

I'd personally add parenthesis to make it obvious which test has priority

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java line 682:

> 680:     // }
> 681:     private static List<JCCase> patchCompletingNormallyCases(List<JCCase> cases) {
> 682:         ListBuffer<JCCase> newCases = new ListBuffer<>();

Do you need to create a new buffer? Isn't this method just side-effecting the various cases?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15532#discussion_r1313196920
PR Review Comment: https://git.openjdk.org/jdk/pull/15532#discussion_r1313195008