phpstan-exception-rules Inconsistent behavior detecting missing and useless @throws annotations

Update: Before you spend too much time reading all the details the below, see my comment in which I explain that I think I've found the cause of my issue and ask a couple of follow-up questions.

I'm getting inconsistent behavior detecting missing and useless @throws annotations. Under some circumstances it behaves correctly. But under others it tells you to fix a problem and then complains if you do. It's easiest to just demonstrate. I'll simplify the scenarios a bit for clarity, but you can view my entire codebase for full context at https://github.com/php-tuf/composer-stager/tree/4bc5893a34aafd042329cb1d9aa377f39f82c88b.

Scenario 1: Pass

Without any annotations, it correctly detects that two are missing.

namespace PhpTuf\ComposerStager\Domain;

use PhpTuf\ComposerStager\Exception\DirectoryNotFoundException;
use PhpTuf\ComposerStager\Exception\IOException;

class Cleaner
{
    public function clean(string $stagingDir): void
    {
        if (!file_exists($stagingDir)) {
            throw new DirectoryNotFoundException($stagingDir);
        }

        if (rmdir($stagingDir)) {
            throw new IOException($stagingDir);
        }
    }
}

$ ./vendor/bin/phpstan analyse src/Domain/Cleaner.php --no-progress
Note: Using configuration file phpstan.neon.
 ------ ---------------------------------------------------------------------------------------
  Line   Cleaner.php
 ------ ---------------------------------------------------------------------------------------
  13     Missing @throws PhpTuf\ComposerStager\Exception\DirectoryNotFoundException annotation
  17     Missing @throws PhpTuf\ComposerStager\Exception\IOException annotation
 ------ ---------------------------------------------------------------------------------------


 [ERROR] Found 2 errors

Scenario 2: Fail

...but add them both, and now it claims that one is useless.

namespace PhpTuf\ComposerStager\Domain;

use PhpTuf\ComposerStager\Exception\DirectoryNotFoundException;
use PhpTuf\ComposerStager\Exception\IOException;

class Cleaner
{
    /**
     * @throws \PhpTuf\ComposerStager\Exception\DirectoryNotFoundException
     * @throws \PhpTuf\ComposerStager\Exception\IOException
     */
    public function clean(string $stagingDir): void
    {
        if (!file_exists($stagingDir)) {
            throw new DirectoryNotFoundException($stagingDir);
        }

        if (rmdir($stagingDir)) {
            throw new IOException($stagingDir);
        }
    }
}

$ ./vendor/bin/phpstan analyse src/Domain/Cleaner.php --no-progress
Note: Using configuration file phpstan.neon.
 ------ ---------------------------------------------------------------------------------------
  Line   Cleaner.php
 ------ ---------------------------------------------------------------------------------------
  14     Useless @throws PhpTuf\ComposerStager\Exception\DirectoryNotFoundException annotation
 ------ ---------------------------------------------------------------------------------------


 [ERROR] Found 1 error

Scenario 3: Fail

Remove the supposedly useless one (DirectoryNotFoundException), and it's happy, even though there's an obvious error. (I currently resort to this option just to keep my builds passing.)

namespace PhpTuf\ComposerStager\Domain;

use PhpTuf\ComposerStager\Exception\DirectoryNotFoundException;
use PhpTuf\ComposerStager\Exception\IOException;

class Cleaner
{
    /**
     * @throws \PhpTuf\ComposerStager\Exception\IOException
     */
    public function clean(string $stagingDir): void
    {
        if (!file_exists($stagingDir)) {
            throw new DirectoryNotFoundException($stagingDir);
        }

        if (rmdir($stagingDir)) {
            throw new IOException($stagingDir);
        }
    }
}

$ ./vendor/bin/phpstan analyse src/Domain/Cleaner.php --no-progress
Note: Using configuration file phpstan.neon.


 [OK] No errors

Scenario 4: Pass

Remove the other one (IOException), and it correctly complains about its absence but no longer incorrectly complains that the first (DirectoryNotFoundException) is useless.

namespace PhpTuf\ComposerStager\Domain;

use PhpTuf\ComposerStager\Exception\DirectoryNotFoundException;
use PhpTuf\ComposerStager\Exception\IOException;

class Cleaner
{
    /**
     * @throws \PhpTuf\ComposerStager\Exception\DirectoryNotFoundException
     */
    public function clean(string $stagingDir): void
    {
        if (!file_exists($stagingDir)) {
            throw new DirectoryNotFoundException($stagingDir);
        }

        if (rmdir($stagingDir)) {
            throw new IOException($stagingDir);
        }
    }
}

$ ./vendor/bin/phpstan analyse src/Domain/Cleaner.php --no-progress
Note: Using configuration file phpstan.neon.
 ------ ------------------------------------------------------------------------
  Line   Cleaner.php
 ------ ------------------------------------------------------------------------
  20     Missing @throws PhpTuf\ComposerStager\Exception\IOException annotation
 ------ ------------------------------------------------------------------------


 [ERROR] Found 1 error

May 26 '21 16:05 TravisCarden

Oh... I think I've figured out the reason for the behavior. My DirectoryNotFoundException extends IOException, so @throws IOException must be considered to cover them both by reason of inheritance. (If I remove the extends relationship, suddenly PHPStan starts complaining about missing @throws DirectoryNotFoundException all over the place.)

So assuming my technical assessment is correct, it leads me to two questions:

Is there a way to prevent other people from experiencing the same confusion I did, via some sort of help text or error message? I raise this merely as something for the maintainers to consider. I have no stake in the matter.
How should I think about the implications of this behavior? For calling code, the difference between a general IOException and a DirectoryNotFoundException in particular may be significant. Relying on docblock comments to communicate the nuance would be liable to introduce the same inconsistency this project is designed to prevent. The best solution I can think of is to redesign my exception hierarchy so that I never throw a general exception (e.g., IOException) but only specific ones (e.g., DirectoryNotFoundException)--i.e., only leaves, no branches. That way I retain the expressive distinction in my @throws annotations without losing the benefit of enforceable rules. If anyone has any thoughts on this, I would welcome them--links to patterns or articles even more-so.

May 26 '21 17:05 TravisCarden

I agree that the error message in the first scenario is a little confusing. It should mean Exception PhpTuf\ComposerStager\Exception\DirectoryNotFoundException is not covered by @throws annotation. IOException covers both exceptions thanks to inheritance.

Is there a way to prevent other people from experiencing the same confusion I did, via some sort of help text or error message? I raise this merely as something for the maintainers to consider. I have no stake in the matter.

Yeah, we can improve error messages :)

How should I think about the implications of this behavior? For calling code, the difference between a general IOException and a DirectoryNotFoundException in particular may be significant. Relying on docblock comments to communicate the nuance would be liable to introduce the same inconsistency this project is designed to prevent. The best solution I can think of is to redesign my exception hierarchy so that I never throw a general exception (e.g., IOException) but only specific ones (e.g., DirectoryNotFoundException)--i.e., only leaves, no branches. That way I retain the expressive distinction in my @throws annotations without losing the benefit of enforceable rules. If anyone has any thoughts on this, I would welcome them--links to patterns or articles even more-so.

As you say. My recommendation is to use the same abstract exception for all "business/checked" exceptions. You can recognize them easily then.

By the way, I'm not sure if I should continue to do something with this project, because all functionality is being implemented to phpstan core. So in the near future, I'll mark this package as obsolete.

May 26 '21 19:05 pepakriz

My recommendation is to use the same abstract exception for all "business/checked" exceptions. You can recognize them easily then.

Oh! I've never thought of abstract exceptions. That's a great idea. Thanks!

all functionality is being implemented to phpstan core

Oh, nice! I assume you're happy about that--congratulations! And thanks for the great contribution. I've only known about it for a short while, but it's been very valuable. 👍

May 27 '21 14:05 TravisCarden