Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
r1pped opened this issue Apr 5, 2023 · 2 comments
Open

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

r1pped opened this issue Apr 5, 2023 · 2 comments

Comments

@r1pped
Copy link

r1pped commented Apr 5, 2023

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
Copy link
Contributor

jrfnl commented Apr 5, 2023

@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
Copy link
Contributor

jrfnl commented Apr 5, 2023

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 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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants