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.66k stars 1.48k forks source link

Reported throw tags either reports as missing or wrong number #3168

Closed dingo-d closed 1 year ago

dingo-d commented 3 years ago

Describe the bug When I sniff the code for errors it reports that I'm missing a throw tag. When I add it, I get wrong number of exceptions found (2 instead of 1).

Code sample

    /**
     * Method that creates actual WPCLI command in terminal
     *
     * @throws \Exception Exception in case the WP_CLI::add_command fails.
     * @throws \RuntimeException Exception in case the WP_CLI::add_command fails.
     *
     * @return void
     */
    public function registerCommand(): void
    {
        if (! class_exists($this->getClassName())) {
            throw new \RuntimeException('Class doesn\'t exist');
        }

        try {
            $reflectionClass = new \ReflectionClass($this->getClassName());
        } catch (\ReflectionException $e) {
            exit("{$e->getCode()}: {$e->getMessage()}");
        }

        $class = $reflectionClass->newInstanceArgs([$this->commandParentName]);

        if (!is_callable($class)) {
            try {
                $className = get_class($class);
                \WP_CLI::error(
                    "The class '{$className}' is not callable. Make sure the command class has an __invoke method."
                );
            } catch (ExitException $e) {
                exit("{$e->getCode()}: {$e->getMessage()}");
            }
        }

        \WP_CLI::add_command(
            $this->commandParentName . ' ' . $this->getCommandName(),
            $class,
            $this->prepareCommandDocs($this->getDoc(), $this->getGlobalSynopsis())
        );
    }

Custom ruleset

<?xml version="1.0"?>
<ruleset name="Eightshift Library">
    <description>Eightshift Library uses extended WordPress coding standards with some minor corrections.</description>

    <rule ref="Infinum"/>

    <exclude-pattern>*/tests/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <exclude-pattern>*/node_modules/*</exclude-pattern>
    <exclude-pattern>*/src/commands/templates/*</exclude-pattern>
    <exclude-pattern>*/src/Build/BuildExample.php</exclude-pattern>
    <exclude-pattern>*/bin/cli.php</exclude-pattern>

    <!-- Additional arguments. -->
    <arg value="sp"/>
    <arg name="basepath" value="."/>
    <arg name="parallel" value="8"/>
    <arg name="extensions" value="php"/>

    <file>.</file>

    <exclude-pattern>/src/CompiledContainer\.php</exclude-pattern>

    <rule ref="WordPress.PHP.DiscouragedPHPFunctions.system_calls_system">
        <exclude-pattern>*/src/**/*Cli</exclude-pattern>
    </rule>

    <rule ref="WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents">
        <exclude-pattern>*/src/**/*Cli</exclude-pattern>
    </rule>

    <rule ref="WordPress.PHP.DiscouragedPHPFunctions.system_calls_shell_exec">
        <exclude-pattern>*/src/Db/*</exclude-pattern>
        <exclude-pattern>*/src/Build/*</exclude-pattern>
        <exclude-pattern>*/cliOutput/bin/Build.php</exclude-pattern>
        <exclude-pattern>*/src/LintPhp/LintPhpCli.php</exclude-pattern>
    </rule>

    <rule ref="WordPress.WP.AlternativeFunctions.file_system_read_fopen">
        <exclude-pattern>*/src/Cli/*</exclude-pattern>
    </rule>

    <rule ref="WordPress.WP.AlternativeFunctions.file_system_read_fwrite">
        <exclude-pattern>*/src/Cli/*</exclude-pattern>
    </rule>

    <rule ref="WordPress.WP.AlternativeFunctions.file_system_read_fclose">
        <exclude-pattern>*/src/Cli/*</exclude-pattern>
    </rule>

    <rule ref="WordPress.Security.EscapeOutput.OutputNotEscaped">
        <exclude-pattern>*/src/**/*Cli.php</exclude-pattern>
        <exclude-pattern>*/src/Cli/*</exclude-pattern>
    </rule>

    <rule ref="Generic.Files.LineLength">
        <exclude name="Generic.Files.LineLength.TooLong"/>
    </rule>

    <rule ref="WordPress.WP.GlobalVariablesOverride">
        <exclude name="WordPress.WP.GlobalVariablesOverride.Prohibited"/>
    </rule>
</ruleset>

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
    FILE: src/Cli/AbstractCli.php
    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
    110 | ERROR | Expected 1 @throws tag(s) in function comment; 2
     |       | found
     |       | (Squiz.Commenting.FunctionCommentThrowTag.WrongNumber)
    ----------------------------------------------------------------------

If I remove the RuntimeException I get

FILE: src/Cli/AbstractCli.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 109 | ERROR | Missing @throws tag for "\RuntimeException"
     |       | exception
     |       | (Squiz.Commenting.FunctionCommentThrowTag.Missing)
----------------------------------------------------------------------

And since it extends the general Exception just throwing one should be ok.

Expected behavior No error when just one throw tag is added.

Versions (please complete the following information):

mundschenk-at commented 3 years ago

I've just noticed a similar problem. I've got a function (function_a) that is documented to throw either an InvalidArgumentException or a RuntimeException. "Delegating" these exceptions upward in the doc block for a wrapping function (function_b) works fine. However, once I add a throw new InvalidArgumentException in the wrapping function (in addition to those potentially thrown by function_a), the sniff starts complaining about the second @throws annotation.

torlokken commented 1 year ago

Any update on this?

dingo-d commented 1 year ago

For me, this still happens, but I think this could be the issue of missing context.

phpcs doesn't know that certain function/method can throw an exception like phpstan does (IIRC I've added the exception in the doc block because of phpstan's suggestion).

Plus the example above is also using a custom ruleset, and the example is not a minimally reproducible one, so I'll close this for now.

@mundschenk-at if you have a better example you can open an issue, but I think the same thing happens in your case (not knowing the context of what is being thrown where).