Unhandled RuntimeException: $stackPtr is not a class member var

[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": "^7.0.2"
    }
}
EOF

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

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

class PhpCodeSnifferFailure
{
    public static $x;

    public function test()
    {
        if (true) {
            return;
        } else if (true) {
            return;
        } elseif (true) {
            self::$x = 1;
            return;
        }
    }
}

PhpCodeSnifferFailure::$x = 1;
EOF

composer install

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

vendor/bin/phpcbf PhpCodeSnifferFailure.php

The run results in the following error:

Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: $stackPtr is not a class member var in vendor/squizlabs/php_codesniffer/src/Files/File.php:1742
Stack trace:
#0 vendor/squizlabs/php_codesniffer/src/Standards/Squiz/Sniffs/NamingConventions/ValidVariableNameSniff.php(108): PHP_CodeSniffer\Files\File->getMemberProperties(88)
#1 vendor/squizlabs/php_codesniffer/src/Sniffs/AbstractVariableSniff.php(144): PHP_CodeSniffer\Standards\Squiz\Sniffs\NamingConventions\ValidVariableNameSniff->processMemberVar(Object(PHP_CodeSniffer\Files\LocalFile), 88)
#2 vendor/squizlabs/php_codesniffer/src/Sniffs/AbstractScopeSniff.php(138): PHP_CodeSniffer\Sniffs\AbstractVariableSniff->processTokenWithinScope(Object(PHP_CodeSniffer\Files\LocalFile), 88, 10)
#3 vendor/squizlabs/php_codesniffer/src/Files/File.php(496): PHP_CodeSniffer\Sniffs\AbstractScopeSniff->process(Object(PHP_CodeSniffer\Files\LocalFile), 88)
#4 vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(91): PHP_CodeSniffe in vendor/squizlabs/php_codesniffer/src/Files/File.php on line 1742

I’m filing it here because the stack trace doesn’t contain any non-squizlabs/php_codesniffer resources.

[jrfnl]

Ok, so this took some investigation, but the issue is actually caused by the SlevomatCodingStandard.ControlStructures.EarlyExit sniff which adds a stray } during fixing, causing a parse error.

The problem is here (last line):

        => Changeset started by SlevomatCodingStandard.ControlStructures.EarlyExit:250
                Q: SlevomatCodingStandard.ControlStructures.EarlyExit:256 replaced token 61 (T_WHITESPACE on line 13) " elseif" => "elseif"
                Q: SlevomatCodingStandard.ControlStructures.EarlyExit:259 replaced token 60 (T_CLOSE_CURLY_BRACKET on line 13) "}" => "}\n"
                Q: SlevomatCodingStandard.ControlStructures.EarlyExit:260 replaced token 60 (T_CLOSE_CURLY_BRACKET on line 13) "}" => "}\n\n"
                Q: SlevomatCodingStandard.ControlStructures.EarlyExit:262 replaced token 62 (T_ELSEIF on line 13) "elseif" => "        } else if"
                A: SlevomatCodingStandard.ControlStructures.EarlyExit:264 replaced token 61 (T_WHITESPACE on line 13) " elseif" => "elseif"
                A: SlevomatCodingStandard.ControlStructures.EarlyExit:264 replaced token 60 (T_CLOSE_CURLY_BRACKET on line 13) "}" => "}\n\n"
                A: SlevomatCodingStandard.ControlStructures.EarlyExit:264 replaced token 62 (T_ELSEIF on line 13) "elseif" => "        } else if"
        => Changeset ended: 3 changes applied

They also change an elseif to else if while their own sniffs do not support else if.

The problem is that this:

	    } elseif (true) {
	        self::$x = 1;
	        return;
	    }

Becomes:

	    }
            } else if (true) {
	        self::$x = 1;
	        return;
	    }

[morozov]

Thank you for the explanation, @jrfnl! The issue in the EarlyExit sniff may have been recently fixed by as slevomat/coding-standard#890. I’ll check against the development version of the Slevomat Coding Standard later to make sure.

On the PHP_CodeSniffer side, would it make sense to run something like php -l on each intermediate result to avoid performing the next attempt to fix the code on something that is already broken? IIRC, something similar happened in #2165.

[jrfnl]

@morozov I think I may have tested with master as well and that I couldn't reproduce it with that, so yes, please test with the develop version, it may well have been fixed already.

Running php -l between loops of the fixer would slow it down a lot, so I don't think that's going to happen.

May I suggest closing this issue and #2852 ?

[morozov]

Not reproducible on dev-master of the Slevomat Coding standard (slevomat/coding-standard@a741c09).

[morozov]

Running php -l between loops of the fixer would slow it down a lot, so I don't think that's going to happen.

I understand. Would it still make sense to lint the state after the last attempt in the case of a RuntimeException?

[jrfnl]

@morozov An uncaught RuntimeException will result in a fatal error and stop the PHPCS run dead, in other words: PHPCS could not initiate a call to the linter after that.

Or is that not what you meant ?

[morozov]

@jrfnl it is what I mean. The question is, what prevents PHPCS from catching runtime exceptions at the runner level? Or at whatever level that schedules the attempts to fix the file:

while (i < 50) {
    try {
        if (fix()) {
            return
        }
    } catch (RuntimeException e) {
        lint()
        throw e
    }

    i++
}

[jrfnl]

@morozov I see what you mean.

PHPCS has no context on how to handle the error in the runner.
Basically RuntimeExceptions are there to tell sniff developers they are doing something wrong and should fix their code (and add more unit tests 😎 ). For phpcs, while they are caught and added to the report for the file, they also stop the PHPCS run for the file completely.

I haven't looked at the code for the PHPCBF runner recently, but I imagine something similar could be done, but it would also need to prevent overwriting the file in that case (which is now prevented by the fatal error). I'd need to look into that more deeply to be sure what would actually be needed.

[morozov]

Basically RuntimeExceptions are there to tell sniff developers they are doing something wrong and should fix their code (and add more unit tests 😎).

I agree. But right now, a user who runs PHPCBF doesn't have a clue about where exactly the bug is and where to report it. If PHPCBF catches the exception, explicitly tells the user that there is a bug and provides all the needed context (e.g. the names of the sniff fixers applied at the last attempt), there's a higher chance that the bug will be reported in the correct repository.

[jrfnl]

explicitly tells the user that there is a bug and provides all the needed context (e.g. the names of the sniff fixers applied at the last attempt)

Well, to do that I think a lot more is needed than simply catching the exception as I don't think PHPCS keeps track of which fixers were applied before.

Either way, I do think this is an interesting feature request. You may want to open a dedicated issue for the feature request "to provide more information to the end-user on a run without verbosity when PHPCBF encounters a fatal error".