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

match() nested within if() statement breaks setting of "conditions" key #3750

Open momala454 opened 1 year ago

momala454 commented 1 year ago

Describe the bug The rule SlevomatCodingStandard.ControlStructures.EarlyExit will cause the following error at line 1 (but there is nothing on line 1), and there shouldn't be any error, the code is valid and doesn't contains any if without a curly brace

FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: "if" without curly braces is not supported.
------------------------------------------------------------------------------------------------------------------------------------------------

Code sample

function testBugCurly(): void
{
    foreach ([1,2] as $step) {
        if (1 !== 1) {
            continue;
        }

        if (1 !== null) {
            if (!match (1) {
                default => 1,
            }) {
            }
        }
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="PSR12" xsi:noNamespaceSchemaLocation="../../../phpcs.xsd">
    <description>The PSR-12 coding standard.</description>
    <arg name="tab-width" value="4"/>

    <!-- Exclude the Composer Vendor directory. -->
    <exclude-pattern>/vendor/*</exclude-pattern>

    <!-- CUSTOM -->
    <config name="installed_paths" value="vendor/slevomat/coding-standard"/>

    <rule ref="SlevomatCodingStandard.ControlStructures.EarlyExit">
    </rule>

</ruleset>

Strangely, adding anything (even a comment) on the last line inside foreach avoid the error. Like this :

function testBugCurly(): void
{
    foreach ([1,2] as $step) {
        if (1 !== 1) {
            continue;
        }

        if (1 !== null) {
            if (!match (1) {
                default => 1,
            }) {
            }
        }
        // hello
    }
}

To reproduce Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php ...
  3. See error message displayed
    PHPCS output here

Expected behavior A clear and concise description of what you expected to happen.

Versions (please complete the following information):

Additional context Add any other context about the problem here.

Initially reported there https://github.com/slevomat/coding-standard/issues/1506 . They say :

         It looks like a bug in PHPCS. The `if (!match (1) {` is missing `scope_closer`.

Originally posted by @kukulich in https://github.com/slevomat/coding-standard/issues/1506#issuecomment-1408648561

jrfnl commented 1 year ago

Thanks for reporting this @momala454 !

I've been able to confirm the bug. Smallest reproduction example:

if (!match (1) {
    default => 1,
}) {
}

The match correctly gets scope opener/closers and gets added to conditions. The if does not.

jrfnl commented 1 year ago

I haven't had a chance to properly debug this yet (and may be a while before I can). Relevant part of the verbose output:

        Start scope map at 2:T_IF => if
        => Begin scope map recursion at token 2 with depth 1
        Process token 3 on line 3 []: T_WHITESPACE =>
        Process token 4 on line 3 []: T_OPEN_PARENTHESIS => (
        Process token 5 on line 3 []: T_BOOLEAN_NOT => !
        Process token 6 on line 3 []: T_MATCH => match
        => Found new opening condition before scope opener for 2:T_IF, backtracking

... and then the backtrace doesn't appear to happen ?

I can see that for closures the debug output from the Tokenizer::recurseScopeMap() method says "handled manually", so maybe for nested match structures it should be too ?

Note: as I said, haven't done proper debugging yet, so I may well be wrong.

jrfnl commented 1 year ago

3763 may be a duplicate of this (unconfirmed), but definitely looks related. Probably a good idea to look at both code samples when addressing either these.

momala454 commented 1 year ago

hello, any updates ?