Internal.Exception on valid switch statements with curly braces

[r1pped]

Describe the bug
Running sniffer with PSR-12 standard on some valid switch statements (mixing curly braces syntax with classic syntax) shows an Internal.Exception, which aborts checking rest of the code

Code sample

<?php

switch (true) {
    case 1 === 1: {
        echo "foo";
        // omitting break; causes Internal.Exception
    }
    case 2 === 2:
        echo "bar";
        break;
}

// code below is not checked anymore
if(false){
    echo 'foo'.'bar';
}

To reproduce
Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs --standard=psr12 test.php
3. See error message displayed

FILE: test.php
--------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: Undefined array key
   |       | "scope_condition" in
   |       | /home/sample/scripts/test/vendor/squizlabs/php_codesniffer/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php
   |       | on line 146 (Internal.Exception)
 4 | ERROR | CASE statements must be defined using a colon (PSR2.ControlStructures.SwitchDeclaration.WrongOpenercase)
--------------------------------------------------------------------------------------------------------------------------------------

Time: 31ms; Memory: 8MB

Expected behavior
Maybe still show PSR2/PSR12 errors in lines 12-14 where if(false){ is?

Versions (please complete the following information):

• OS: Ubuntu 20.04.5 LTS
• PHP: 8.1.16
• PHPCS: 3.7.2
• Standard: PSR12

Additional context
Even if we ignore PSR2.ControlStructures.SwitchDeclaration.WrongOpenercase sniffer which occurs in line 4, the exception still happens and the rest of the code is not checked anymore.

[jrfnl]

@r1pped Thanks for reporting this. I can confirm the issue.

It's actually the combination of the curly braces being used for the first case AND the deliberately fall-through (no break/continue) which causes the issue.

It results in the second case having a the T_COLON as a scope_opener, the T_BREAK as the scope_closer, but the T_BREAK doesn't get its scope_condition set to the first case, which is what the sniff expects and what should have happened.

This will need some digging into what exactly is going wrong with the setting of the scope_* indexes.

[jrfnl]

I'm noticing another inconsistency in the token arrays related to this.

switch (true) {
    case 2 === 2:
        echo "bar";
        break;
    case 3 === 3:
        echo "bar";
        break;
}

In the above, the T_COLON and the T_BREAK tokens have only the T_SWITCH in the conditions array. The tokens in between have T_SWITCH and T_CASE.

The T_CASE and the associated T_COLON and T_BREAK each have the same set of scope_condition (T_CASE), scope_opener (T_COLON) and scope_closer (T_BREAK) set.

switch (true) {
    case 1 === 1: {
        echo "foo";
        break;
    }
    case 2 === 2: {
        echo "bar";
        break;
    }
}

In this second code sample, the T_OPEN_CURLY_BRACKET has both the T_SWITCHand theT_CASEin theconditionsarray, while theT_CLOSE_CURLY_BRACKETonly has theT_SWITCHin theconditionsarray. TheT_CASEand the associatedT_COLONandT_BREAKeach have the same set ofscope_condition (T_CASE), scope_opener (T_COLON) and scope_closer (T_BREAK`) set.

👉🏻 IMO it would be more consistent if the T_OPEN_CURLY_BRACKET would not have the T_CASE in the conditions array.

If we then take (a variation on) the original code sample:

switch (true) {
    case 1 === 1: {
        echo "foo";
        // omitting break; causes Internal.Exception
    }
    case 2 === 2:
        echo "bar";
        break;
    case 3 === 3:
        echo "bar";
        break;
}

The first case behaves like the second code sample above (open curly has two conditions, close curly one, rest consistent).

The second case is where things get weird:

• The T_COLON has only the T_SWITCH in the conditions array, while the T_BREAK has both the T_SWITCH as well as the (second) T_CASE in the conditions array.
• The T_COLON has the scope_condition set to the second T_CASE, the scope_opener to itself and the scope_closer to the T_BREAK (in the second case).
• The T_BREAK (in the second case) has NO scope_condition, scope_opener or scope_closer set AT ALL.

The third case behaves as expected, exactly like the first code sample above (everything consistent).

I'm fairly confident that the culprit is somewhere in the last part of the Tokenizer\PHP::processAdditional() method (after the // Only interested in CASE and DEFAULT statements from here on in. comment), but as there are no dedicated tests covering that part of the code, changing anything there has a high risk of breaking things elsewhere.

@gsherwood Got an opinion on this ?