Skip to content

Commit b61c37d

Browse files
authored
Merge pull request #35 from moufmouf/must_rethrow
Base exceptions must be rethrown if caught.
2 parents 470fec0 + 596f378 commit b61c37d

File tree

5 files changed

+163
-0
lines changed

5 files changed

+163
-0
lines changed

README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ They are more "strict" than the default PHPStan rules and some may be controvers
2222
- You should not throw the "Exception" base class directly [but throw a sub-class instead](http://bestpractices.thecodingmachine.com/php/error_handling.html#subtyping-exceptions).
2323
- You should not have empty catch statements
2424
- When throwing an exception inside a catch block, [you should pass the catched exception as the "previous" exception](http://bestpractices.thecodingmachine.com/php/error_handling.html#wrapping-an-exception-do-not-lose-the-previous-exception)
25+
- If you catch a `Throwable`, an `Exception` or a `RuntimeException`, you must rethrow the exception.
2526

2627
### Type-hinting related rules
2728

phpstan-strict-rules.neon

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ services:
1111
class: TheCodingMachine\PHPStan\Rules\Exceptions\EmptyExceptionRule
1212
tags:
1313
- phpstan.rules.rule
14+
-
15+
class: TheCodingMachine\PHPStan\Rules\Exceptions\MustRethrowRule
16+
tags:
17+
- phpstan.rules.rule
1418
-
1519
class: TheCodingMachine\PHPStan\Rules\TypeHints\MissingTypeHintInFunctionRule
1620
tags:
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
<?php
2+
3+
4+
namespace TheCodingMachine\PHPStan\Rules\Exceptions;
5+
6+
use Exception;
7+
use function in_array;
8+
use PhpParser\Node;
9+
use PhpParser\Node\Stmt\Catch_;
10+
use PhpParser\NodeTraverser;
11+
use PhpParser\NodeVisitorAbstract;
12+
use PHPStan\Analyser\Scope;
13+
use PHPStan\Broker\Broker;
14+
use PHPStan\Rules\Rule;
15+
use RuntimeException;
16+
use TheCodingMachine\PHPStan\Utils\PrefixGenerator;
17+
use Throwable;
18+
19+
/**
20+
* When catching \Exception, \RuntimeException or \Throwable, the exception MUST be thrown again
21+
* (unless you are developing an exception handler...)
22+
*/
23+
class MustRethrowRule implements Rule
24+
{
25+
public function getNodeType(): string
26+
{
27+
return Catch_::class;
28+
}
29+
30+
/**
31+
* @param Catch_ $node
32+
* @param \PHPStan\Analyser\Scope $scope
33+
* @return string[]
34+
*/
35+
public function processNode(Node $node, Scope $scope): array
36+
{
37+
// Let's only apply the filter to \Exception, \RuntimeException or \Throwable
38+
$exceptionType = null;
39+
foreach ($node->types as $type) {
40+
if (in_array((string)$type, [Exception::class, RuntimeException::class, Throwable::class], true)) {
41+
$exceptionType = (string)$type;
42+
break;
43+
}
44+
}
45+
46+
if ($exceptionType === null) {
47+
return [];
48+
}
49+
50+
// Let's visit and find a throw.
51+
$visitor = new class() extends NodeVisitorAbstract {
52+
private $throwFound = false;
53+
54+
public function leaveNode(Node $node)
55+
{
56+
if ($node instanceof Node\Stmt\Throw_) {
57+
$this->throwFound = true;
58+
}
59+
}
60+
61+
/**
62+
* @return bool
63+
*/
64+
public function isThrowFound(): bool
65+
{
66+
return $this->throwFound;
67+
}
68+
};
69+
70+
$traverser = new NodeTraverser();
71+
72+
$traverser->addVisitor($visitor);
73+
74+
$traverser->traverse($node->stmts);
75+
76+
$errors = [];
77+
78+
if (!$visitor->isThrowFound()) {
79+
$errors[] = sprintf('%scaught "%s" must be rethrown. Either catch a more specific exception or add a "throw" clause in the "catch" block to propagate the exception.', PrefixGenerator::generatePrefix($scope), $exceptionType);
80+
}
81+
82+
return $errors;
83+
}
84+
}
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace PHPStan\Rules\Exceptions;
4+
5+
use PHPStan\Testing\RuleTestCase;
6+
use TheCodingMachine\PHPStan\Rules\Exceptions\MustRethrowRule;
7+
use TheCodingMachine\PHPStan\Rules\Exceptions\ThrowMustBundlePreviousExceptionRule;
8+
9+
class MustRethrowRuleTest extends RuleTestCase
10+
{
11+
protected function getRule(): \PHPStan\Rules\Rule
12+
{
13+
return new MustRethrowRule();
14+
}
15+
16+
public function testCheckCatchedException()
17+
{
18+
$this->analyse([__DIR__ . '/data/must_rethrow.php'], [
19+
[
20+
'caught "Exception" must be rethrown. Either catch a more specific exception or add a "throw" clause in the "catch" block to propagate the exception.',
21+
18,
22+
],
23+
[
24+
'caught "Throwable" must be rethrown. Either catch a more specific exception or add a "throw" clause in the "catch" block to propagate the exception.',
25+
24,
26+
],
27+
[
28+
'In function "TestCatch\foo", caught "RuntimeException" must be rethrown. Either catch a more specific exception or add a "throw" clause in the "catch" block to propagate the exception.',
29+
31,
30+
],
31+
]);
32+
}
33+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?php
2+
3+
namespace TestCatch;
4+
5+
class FooCatch
6+
{
7+
}
8+
9+
class MyCatchException extends \Exception
10+
{
11+
}
12+
13+
try {
14+
} catch (\TestCatch\MyCatchException $e) {
15+
}
16+
17+
try {
18+
} catch (\Exception $e) {
19+
// Do something
20+
$foo = 42;
21+
}
22+
23+
try {
24+
} catch (\Throwable $e) {
25+
// Do something
26+
$foo = 42;
27+
}
28+
29+
function foo() {
30+
try {
31+
} catch (\RuntimeException $e) {
32+
// Do something
33+
$foo = 42;
34+
}
35+
}
36+
37+
try {
38+
} catch (\Exception $e) {
39+
// Do something
40+
throw $e;
41+
}

0 commit comments

Comments
 (0)