Failures in child processes are not handled by the runner

[morozov]

Unfortunately, I don't know how to reproduce this issue with the OOTB standards, but here's the minimal required setup:

cat > composer.json << 'EOF'
{
    "require-dev": {
        "doctrine/coding-standard": "^4.0"
    }
}
EOF

cat > phpcs.xml.dist << 'EOF'
<?xml version="1.0"?>
<ruleset>
    <rule ref="Doctrine"/>
</ruleset>
EOF

cat > PhpCodeSnifferFailure.php << 'EOF'
<?php

namespace Doctrine\DBAL;

class PhpCodeSnifferFailure
{
    public function foo()
    {
        return [
        <<<HERE
HERE
            ,
        ];
    }
}
EOF

composer install

vendor/bin/phpcs --version
# PHP_CodeSniffer version 3.3.1 (stable) by Squiz (http://www.squiz.net)

vendor/bin/phpcbf --parallel=2 PhpCodeSnifferFailure.php

The run results in the following error:

1. PHP_CodeSniffer\Exceptions\RuntimeException: Undefined index: scope_closer in vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/UseStatementHelper.php on line 156 in /home/morozov/t1/vendor/squizlabs/php_codesniffer/src/Runner.php on line 562

   This error happens in the child process outside of the PHP_CodeSniffer codebase because the T_CLASS token doesn't have the scope_closer attribute defined.

2. PHP_CodeSniffer\Exceptions\RuntimeException: Undefined variable: childOutput in vendor/squizlabs/php_codesniffer/src/Runner.php on line 705 in vendor/squizlabs/php_codesniffer/src/Runner.php on line 562

   This error happens in PHP_CodeSniffer itself. The parent creates an empty shared file before running the child:

   PHP_CodeSniffer/src/Runner.php

   Line 418 in 4725c01

   $childOutFilename = tempnam(sys_get_temp_dir(), 'phpcs-child');

   but the child dies before writing its contents due to the error above. However, the parent only checks if the file exists:

   PHP_CodeSniffer/src/Runner.php

   Line 677 in 4725c01

   if (file_exists($procData['out']) === true) {

Besides the bug in the runner, is the fact that a T_CLASS token doesn't have scope_closer also a bug?

[gsherwood]

A RuntimeException being thrown would end the run if you were not running with the parallel option as well, so you shouldn't expect a clean shutdown or valid report in this case.

The real error here is the T_CLASS missing the scope_closer. But in my testing, PHPCS tokenizes the sample code and finds the scope_closer correctly. I don't have time to debug this to see if there is a problem with the custom sniff, but I'd look there first to see what's going on.

[gsherwood]

You might also want to try running phpcbf with the -vvv option, which will show you what changes the fixer is making. That might help you track down a sniff that is fixing the file incorrectly.

[morozov]

@gsherwood how do you store the tokens produced from a given file? IIRC, there was an issue where PHPCS had different logic in debug mode.

[gsherwood]

how do you store the tokens produced from a given file? IIRC, there was an issue where PHPCS had different logic in debug mode.

I'm not sure what you mean, and I'm not sure what different logic you are referring to.

[morozov]

But in my testing, PHPCS tokenizes the sample code and finds the scope_closer correctly.

How can I produce a list of PHPCS tokens from a file to share it or see if there's the needed attribute? When debugging the custom sniff, I can see that there's no closer in the token.

I'm not sure what you mean, and I'm not sure what different logic you are referring to.

I'm referring to #903 (comment).

[gsherwood]

I'm referring to #903 (comment).

That's just the ScopeIndent sniff running in debug mode. You aren't using that special debug flag for it, so that's not going to be the issue.

How can I produce a list of PHPCS tokens from a file to share it or see if there's the needed attribute?

Try running phpcs --report=diff -vvv .... That will give you the tokenizer output for the original file, all the fixes being made, and the contents of the fixed file after each pass. That output will show what is going wrong. Note this is PHPCS and not PHPCBF.

[morozov]

@gsherwood here's the interesting fragment from the output of phpcs --report=diff -vvv:

	* fixed 2 violations, starting loop 2 *
---START FILE CONTENT---
 1|<?php
 2|declare(strict_types = 1);
 3|
 4|namespace Doctrine\DBAL;
 5|
 6|class PhpCodeSnifferFailure
 7|{
 8|    public function foo()
 9|    {
10|        return [
11|        <<<HERE
12|HERE
13|,
14|        ];
15|    }
16|}
17|
--- END FILE CONTENT ---

--------------8<------------------

	E: [Line 13] Expected 0 spaces between "HERE" and comma; newline found (Squiz.Arrays.ArrayDeclaration.SpaceBeforeComma)
	PHP_CodeSniffer\Standards\Squiz\Sniffs\Arrays\ArrayDeclarationSniff (line 449) replaced token 46 (T_WHITESPACE) "\n········," => "········,"
	* fixed 8 violations, starting loop 3 *
---START FILE CONTENT---
 1|<?php
 2|
 3|declare(strict_types=1);
 4|
 5|namespace Doctrine\DBAL;
 6|
 7|class PhpCodeSnifferFailure
 8|{
 9|    public function foo()
10|    {
11|        return [
12|        <<<HERE
13|HERE        ,
14|        ];
15|    }
16|}
17|
--- END FILE CONTENT ---

By the beginning of the loop 2, the closing HERE identifier and the comma are on separate lines which is correct, but during the loop, ArrayDeclarationSniff removes the new line character which makes the closing identifier invalid. Therefore (as far as I understand), on the next iteration, the T_CLASS loses its scope closer.

[morozov]

A RuntimeException being thrown would end the run if you were not running with the parallel option as well, so you shouldn't expect a clean shutdown or valid report in this case.

This is true but the output should be the same in both parallel and non-parallel modes. Currently, there's one error in non-parallel mode, and two errors in parallel. This is confusing and requies additional debugging to find the real issue.

[gsherwood]

This is true but the output should be the same in both parallel and non-parallel modes.

I'm not going to silence the error complaining about childOutput being missing. If I did, I'd possibly not be reporting an error when something else goes wrong with the child process, and it may silently fail but look to succeed.

I am going to look into the array sniff and fix it up.

[morozov]

I'm not going to silence the error complaining about childOutput being missing.

I didn't mean the errors should be suppressed. What I meant is that the runner should account for the fact the children may fail and not produce any additional unhandled errors.

For instance, it could show the available part of the report and additionally say that processes X, Y and Z failed, therefore the report may be incomplete. It also could suggest running the same report without parallel execution. With this approach, once a child process fails, a developer could focus on the error from the child and not debug the error in the runner.

[gsherwood]

For instance, it could show the available part of the report and additionally say that processes X, Y and Z failed, therefore the report may be incomplete.

Sorry, understand you now.