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

Fatal error if you try to get the declaration name of an arrow function #3766

Closed antonioeatgoat closed 1 year ago

antonioeatgoat commented 1 year ago

Describe the bug If you try to get the declaration name of an arrow function using the method File::getDeclarationName(), it will throws an exception.

It looks like it is legit to use the method for a closure, since there is this condition here, so probably the same condition should be added also for T_FN.

Custom code sniff sample

use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Files\File;

class MyCustomCodeSniff implements Sniff
{
    public function register()
    {
        return [T_FUNCTION, T_CLOSURE, T_FN];
    }

    public function process(File $file, $position)
    {
        /** @var array<int, array<string, mixed>> $tokens */
        $tokens = $file->getTokens();

        if (!in_array(($tokens[$position]['code'] ?? null), [T_FUNCTION, T_CLOSURE, T_FN], true)) {
            return;
        }

        $functionStart = (int)($tokens[$position]['scope_opener'] ?? 0);
        $functionEnd = (int)($tokens[$position]['scope_closer'] ?? 0);

        if (($functionStart < 0) || ($functionEnd <= 0)) {
            return;
        }

        $declarationName = $file->getDeclarationName($position);

        // ... do other thins here ...
    }
}

To reproduce Steps to reproduce the behavior:

  1. Create a file called test.php which contain a simple fn() => 'x';
  2. Add the custom sniff rule
  3. Run phpcs test.php ...
  4. See error message displayed
    An error occurred during processing; checking has been aborted. The error message was: Token type "T_FN" is not T_FUNCTION, T_CLASS, T_INTERFACE, T_TRAIT or T_ENUM (Internal.Exception)

Expected behavior A condition for T_FN is added as explained above, so that the method File::getDeclarationName() can return null instead of throwing an exception, when used on an arrow function.

Versions (please complete the following information):

jrfnl commented 1 year ago

@antonioeatgoat Eh... In my opinion, that would be a hard "no".

Arrow functions are anonymous by nature, so don't have a name, so a sniff should not try to retrieve one for it.

As far as I know, the ONLY reason closures are handled in the File::getDeclarationName() function, is because PHP natively tokenizes those tokens as T_FUNCTION and originally that meant there was no difference between the token for named and the token for anonymous functions.

Since then (and a long time ago now), PHPCS has updated the tokenizer to retokenize T_FUNCTION tokens for closures to T_CLOSURE. The only reason that T_CLOSURE is still handled in the File::getDeclarationName() is for BC reasons and - again IMO - the handling of T_CLOSURE should be removed for PHPCS 4.x (actually, it should have been removed for 3.x).

antonioeatgoat commented 1 year ago

Thank you @jrfnl for the context, I didn't know it, it makes more sense now.

Then I guess that the correct way is to check that position is an actual T_FUNCTION before trying to get a declaration name.

If so this issue can be closed for me.