squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Internal.Exception on valid switch statements with curly braces #3794

Open r1pped opened 1 year ago

r1pped commented 1 year ago

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 commented 1 year ago

@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 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 commented 1 year ago

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_SWITCH and the T_CASE in the conditions array, while the T_CLOSE_CURLY_BRACKET only has the T_SWITCH in the conditions array. 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.

👉🏻 _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 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 ?