Squiz.Commenting.FunctionCommentThrowTag.WrongNumber Error for throwing exceptions from called methods

[fsmeier]

Hey,

I wanted to use the sniff Squiz.Commenting.FunctionCommentThrowTag.WrongNumber but I have to enable it since it is not working as expected. Additional to the different expected behaviour for wrongNumber the Missing sniff is not called when wrongNumber is disabled but found a violation (because of a return in FunctionCommentThrowTagSniff.php).

My use-case:

<?php

/**
 * Class MyException
 */
class MyException extends Exception
{

}

/**
 * Class MyOtherException
 */
class MyOtherException extends Exception
{

}

/**
 * Class MyClass
 */
class MyClass
{
    /**
     * MyClass constructor.
     * @param bool $var
     * @throws MyOtherException
     * @throws MyException
     */
    public function __construct(bool $var)
    {
        if ($var) {
            throw new MyOtherException();
        }

        $this->throwException();
    }

    /**
     * @return void
     * @throws MyException
     */
    protected function throwException(): void
    {
        throw new MyException;
    }
}

Example phpcs-file

<?xml version="1.0"?>
<ruleset name="PHPDoc">

    <rule ref="Squiz.Commenting.FunctionCommentThrowTag">
        <!--<exclude name="Squiz.Commenting.FunctionCommentThrowTag.WrongNumber"/>-->
    </rule>

</ruleset>

Error for checking the php-file:
29 | ERROR | Expected 1 @throws tag(s) in function comment; 2 found

My expectation for this sniff would be that it counts in the documented thrown tags from the called methods as well. Also: when switched off the sniff WrongNumber it should still find Missing but it does not. It just finds Missing case if there is no WrongNumber regardless of it being enabled or not.

All the best,
Florian

[gsherwood]

My expectation for this sniff would be that it counts in the documented thrown tags from the called methods as well.

PHPCS does not analyse an entire project to determine method calls, so the sniff will never work like this. You would need to look into using a different tool if you want something that analyses your entire source and determines execution paths.

[fsmeier]

My expectation for this sniff would be that it counts in the documented thrown tags from the called methods as well.

PHPCS does not analyse an entire project to determine method calls, so the sniff will never work like this. You would need to look into using a different tool if you want something that analyses your entire source and determines execution paths.

Yes, I would not expect PHPCS to do a deep analysis of what could be possible thrown somewhere. But since it is parsing the code-block for possible thrown exceptions for me it would make sense to also consider the @throw-tags of the phpdoc of called functions/methods.

The bigger problem here for me is, that if i exclude this sniff-part WrongNumber from the validation it will not show errors for missing ones anymore, if there is a number-missmatch (but it is not shown, because of excluded in config)

[VincentLanglet]

The WrongNumber part has another bug. The following code doesn't count as exception:

$e = new Exception();
throw $e;

It seems better to remove the WrongNumber part of this sniff.
This way, we don't have error for :

/**
     * MyClass constructor.
     * @param bool $var
     * @throws MyOtherException
     * @throws MyException
     */
    public function __construct(bool $var)
    {
        if ($var) {
            throw new MyOtherException();
        }

        $this->throwException();
    }

But still get error with

/**
     * MyClass constructor.
     * @param bool $var
     */
    public function __construct(bool $var)
    {
        if ($var) {
            throw new MyOtherException();
        }
    }

or

/**
     * MyClass constructor.
     * @param bool $var
     * @throws MyException
     */
    public function __construct(bool $var)
    {
        if ($var) {
            throw new MyOtherException();
        }

        $this->throwException();
    }

[gsherwood]

The WrongNumber part has another bug. The following code doesn't count as exception:

$e = new Exception();
throw $e;

The sniff is never going to figure that out. It doesn't trace variables throughout the code, and you could keep coming up with more and more complex failing examples if it tried to.

[gsherwood]

But since it is parsing the code-block for possible thrown exceptions for me it would make sense to also consider the @throw-tags of the phpdoc of called functions/methods.

The sniff is making the assumption that your docblock is wrong. If it assumed the docblock is right, there really isn't any need for it.

[VincentLanglet]

The sniff is making the assumption that your docblock is wrong. If it assumed the docblock is right, there really isn't any need for it.

But sometimes the docblock is wrong and sometimes the docblock is right.
I was expecting this rules to catch some docblock errors without reporting some false-postive.

The sniff is never going to figure that out. It doesn't trace variables throughout the code, and you could keep coming up with more and more complex failing examples if it tried to.

Yeah, I don't expect this sniff to report error for missing docblock. But he should not report error if I use the correct docblock.

I think it's better to catch sometimes error and never having false-positive than catch more often error and having sometimes false-positive.

[gsherwood]

I was expecting this rules to catch some docblock errors without reporting some false-postive.

If your docblock says it throws a second exception but there isn't anywhere in the method where the exception is being thrown, the sniff cant possible know if this docblock is actually correct. It might be correct (an exception thrown from a called method) or it might be a mistake. At some point, the sniff has to decide if it trusts the docblock or not. Should it throw an error for a developer to look at, or not?

[VincentLanglet]

If your docblock says it throws a second exception but there isn't anywhere in the method where the exception is being thrown, the sniff cant possible know if this docblock is actually correct. It might be correct (an exception thrown from a called method) or it might be a mistake.

I agree

At some point, the sniff has to decide if it trusts the docblock or not. Should it throw an error for a developer to look at, or not?

I prefer to not throw error. If the sniff throw the error, a developper can miss the fact the annotation is needed, and removing it. Plus, I don't want to see everywhere in my code some @phpcs-ignore comment. And finally, when a sniff throw an error when there is not, you lost the trust in this sniff.

[gsherwood]

Closing as the sniff is working as expected, even though it may not be considered useful in these cases.