Skip to content

Commit b478b96

Browse files
pepakrizpepakriz
authored
Report useless throws annotations (fixed #32) (#70)
* Report useless throws annotations (fixed #32) * Ported to latest master * Make useless exception check order-insensitive
1 parent c3be204 commit b478b96

File tree

6 files changed

+305
-0
lines changed

6 files changed

+305
-0
lines changed

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ This extension provides following rules and features:
1717
* JsonSerializable interface combinated with `json_encode()` function ([examples](https://github.com/pepakriz/phpstan-exception-rules/blob/master/tests/src/Rules/data/json-serializable.php))
1818
* Ignore caught checked exceptions ([examples](https://github.com/pepakriz/phpstan-exception-rules/blob/master/tests/src/Rules/data/try-catch.php))
1919
* Unnecessary `@throws` annotation detection ([examples](https://github.com/pepakriz/phpstan-exception-rules/blob/master/tests/src/Rules/data/unused-throws.php))
20+
* Useless `@throws` annotation detection ([examples](https://github.com/pepakriz/phpstan-exception-rules/blob/master/tests/src/Rules/data/useless-throws.php))
2021
* Optionally ignore descriptive `@throws` annotations ([examples](https://github.com/pepakriz/phpstan-exception-rules/blob/master/tests/src/Rules/data/unused-descriptive-throws.php))
2122
* `@throws` annotation variance validation ([examples](https://github.com/pepakriz/phpstan-exception-rules/blob/master/tests/src/Rules/data/throws-inheritance.php))
2223
* [Dynamic throw types based on arguments](#extensibility)

‎extension.neon‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,7 @@ services:
5858
-
5959
class: Pepakriz\PHPStanExceptionRules\Rules\UnreachableCatchRule
6060
tags: [phpstan.rules.rule]
61+
62+
-
63+
class: Pepakriz\PHPStanExceptionRules\Rules\UselessThrowsPhpDocRule
64+
tags: [phpstan.rules.rule]

‎src/Rules/UselessThrowsPhpDocRule.php‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Pepakriz\PHPStanExceptionRules\Rules;
4+
5+
use Pepakriz\PHPStanExceptionRules\ThrowsAnnotationReader;
6+
use PhpParser\Node;
7+
use PhpParser\Node\FunctionLike;
8+
use PhpParser\Node\Stmt\ClassMethod;
9+
use PhpParser\Node\Stmt\Function_;
10+
use PHPStan\Analyser\Scope;
11+
use PHPStan\Broker\Broker;
12+
use PHPStan\Broker\ClassNotFoundException;
13+
use PHPStan\Broker\FunctionNotFoundException;
14+
use PHPStan\Reflection\MissingMethodFromReflectionException;
15+
use PHPStan\Rules\Rule;
16+
use PHPStan\ShouldNotHappenException;
17+
use function array_keys;
18+
use function ltrim;
19+
use function sprintf;
20+
use function uksort;
21+
22+
class UselessThrowsPhpDocRule implements Rule
23+
{
24+
25+
/**
26+
* @var Broker
27+
*/
28+
private $broker;
29+
30+
/**
31+
* @var ThrowsAnnotationReader
32+
*/
33+
private $throwsAnnotationReader;
34+
35+
public function __construct(
36+
Broker $broker,
37+
ThrowsAnnotationReader $throwsAnnotationReader
38+
)
39+
{
40+
$this->broker = $broker;
41+
$this->throwsAnnotationReader = $throwsAnnotationReader;
42+
}
43+
44+
public function getNodeType(): string
45+
{
46+
return FunctionLike::class;
47+
}
48+
49+
/**
50+
* @return string[]
51+
*/
52+
public function processNode(Node $node, Scope $scope): array
53+
{
54+
$docComment = $node->getDocComment();
55+
if ($docComment === null) {
56+
return [];
57+
}
58+
59+
if ($node instanceof ClassMethod) {
60+
$classReflection = $scope->getClassReflection();
61+
if ($classReflection === null) {
62+
throw new ShouldNotHappenException();
63+
}
64+
65+
$methodName = $node->name->toString();
66+
try {
67+
$functionReflection = $classReflection->getMethod($methodName, $scope);
68+
} catch (MissingMethodFromReflectionException $e) {
69+
throw new ShouldNotHappenException();
70+
}
71+
72+
} elseif ($node instanceof Function_) {
73+
$functionName = ltrim($scope->getNamespace() . '\\' . $node->name->toString(), '\\');
74+
try {
75+
$functionReflection = $this->broker->getFunction(new Node\Name\FullyQualified($functionName), $scope);
76+
} catch (FunctionNotFoundException $e) {
77+
throw new ShouldNotHappenException();
78+
}
79+
80+
} else {
81+
return [];
82+
}
83+
84+
$throwsAnnotations = $this->throwsAnnotationReader->readByReflection($functionReflection, $scope);
85+
86+
try {
87+
return $this->checkUselessThrows($throwsAnnotations);
88+
} catch (ClassNotFoundException $exception) {
89+
return [];
90+
}
91+
}
92+
93+
/**
94+
* @param string[][] $throwsAnnotations
95+
* @return string[]
96+
*
97+
* @throws ClassNotFoundException
98+
*/
99+
private function checkUselessThrows(array $throwsAnnotations): array
100+
{
101+
/** @var string[] $errors */
102+
$errors = [];
103+
104+
$this->sortThrowsAnnotationsHierarchically($throwsAnnotations);
105+
106+
/** @var bool[] $usefulThrows */
107+
$usefulThrows = [];
108+
foreach ($throwsAnnotations as $exceptionClass => $descriptions) {
109+
foreach ($descriptions as $description) {
110+
if (
111+
isset($usefulThrows[$exceptionClass])
112+
|| (
113+
$description === '' && $this->isSubtypeOfUsefulThrows($exceptionClass, array_keys($usefulThrows))
114+
)
115+
) {
116+
$errors[] = sprintf('Useless @throws %s annotation', $exceptionClass);
117+
}
118+
119+
$usefulThrows[$exceptionClass] = true;
120+
}
121+
}
122+
123+
return $errors;
124+
}
125+
126+
/**
127+
* @param string[] $usefulThrows
128+
*
129+
* @throws ClassNotFoundException
130+
*/
131+
private function isSubtypeOfUsefulThrows(string $exceptionClass, array $usefulThrows): bool
132+
{
133+
$classReflection = $this->broker->getClass($exceptionClass);
134+
135+
foreach ($usefulThrows as $usefulThrow) {
136+
if ($classReflection->isSubclassOf($usefulThrow)) {
137+
return true;
138+
}
139+
}
140+
141+
return false;
142+
}
143+
144+
/**
145+
* @param string[][] $throwsAnnotations
146+
*
147+
* @throws ClassNotFoundException
148+
*/
149+
private function sortThrowsAnnotationsHierarchically(array &$throwsAnnotations): void
150+
{
151+
uksort($throwsAnnotations, function (string $leftClass, string $rightClass): int {
152+
$leftReflection = $this->broker->getClass($leftClass);
153+
$rightReflection = $this->broker->getClass($rightClass);
154+
155+
// Ensure canonical class names
156+
$leftClass = $leftReflection->getName();
157+
$rightClass = $rightReflection->getName();
158+
159+
if ($leftClass === $rightClass) {
160+
return 0;
161+
}
162+
163+
if ($leftReflection->isSubclassOf($rightClass)) {
164+
return 1;
165+
}
166+
167+
if ($rightReflection->isSubclassOf($leftClass)) {
168+
return -1;
169+
}
170+
171+
// Doesn't matter, sort consistently on classname
172+
return $leftClass <=> $rightClass;
173+
});
174+
}
175+
176+
}

‎src/ThrowsAnnotationReader.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,16 @@ public function read(Scope $scope): array
5656
return [];
5757
}
5858

59+
return $this->readByReflection($reflection, $scope);
60+
}
61+
62+
/**
63+
* @param \PHPStan\Reflection\FunctionReflection|\PHPStan\Reflection\MethodReflection $reflection
64+
*
65+
* @return string[][]
66+
*/
67+
public function readByReflection($reflection, Scope $scope): array
68+
{
5969
$namespace = $scope->getNamespace();
6070
$sourceFile = $scope->getFile();
6171

‎tests/src/Rules/UselessThrowsPhpDocRuleTest.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Pepakriz\PHPStanExceptionRules\Rules;
4+
5+
use Pepakriz\PHPStanExceptionRules\RuleTestCase;
6+
use PHPStan\Rules\Rule;
7+
8+
class UselessThrowsPhpDocRuleTest extends RuleTestCase
9+
{
10+
11+
protected function getRule(): Rule
12+
{
13+
return new UselessThrowsPhpDocRule(
14+
$this->createBroker([]),
15+
$this->createThrowsAnnotationReader()
16+
);
17+
}
18+
19+
public function testBasicUselessThrows(): void
20+
{
21+
$this->analyse(__DIR__ . '/data/useless-throws.php');
22+
}
23+
24+
}

‎tests/src/Rules/data/useless-throws.php‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Pepakriz\PHPStanExceptionRules\Rules\UselessThrows;
4+
5+
use RuntimeException;
6+
use LogicException;
7+
8+
class FooException extends RuntimeException {}
9+
10+
class BarException extends LogicException {}
11+
12+
/**
13+
* @throws RuntimeException
14+
* @throws RuntimeException
15+
*/
16+
function duplicatedAnnotation(): void // error: Useless @throws RuntimeException annotation
17+
{
18+
throw new FooException();
19+
}
20+
21+
trait UselessThrowsTrait
22+
{
23+
24+
/**
25+
* @throws RuntimeException
26+
* @throws RuntimeException
27+
*/
28+
public function duplicatedAnnotationInTrait(): void // error: Useless @throws RuntimeException annotation
29+
{
30+
throw new FooException();
31+
}
32+
33+
}
34+
35+
class UselessThrows
36+
{
37+
38+
use UselessThrowsTrait;
39+
40+
/**
41+
* @throws RuntimeException
42+
* @throws RuntimeException
43+
*/
44+
public function duplicatedAnnotation(): void // error: Useless @throws RuntimeException annotation
45+
{
46+
throw new FooException();
47+
}
48+
49+
/**
50+
* @throws RuntimeException
51+
* @throws RuntimeException Because of ...
52+
*/
53+
public function duplicatedWithDescription(): void // error: Useless @throws RuntimeException annotation
54+
{
55+
throw new FooException();
56+
}
57+
58+
/**
59+
* @throws RuntimeException
60+
* @throws FooException
61+
*/
62+
public function inheritedAnnotation(): void // error: Useless @throws Pepakriz\PHPStanExceptionRules\Rules\UselessThrows\FooException annotation
63+
{
64+
throw new FooException();
65+
}
66+
67+
/**
68+
* @throws FooException
69+
* @throws BarException
70+
* @throws RuntimeException
71+
*/
72+
public function unorderedExceptions(): void // error: Useless @throws Pepakriz\PHPStanExceptionRules\Rules\UselessThrows\FooException annotation
73+
{
74+
if (2 % 2 === 0) {
75+
throw new FooException();
76+
}
77+
78+
throw new BarException();
79+
}
80+
81+
/**
82+
* @throws RuntimeException
83+
* @throws FooException Because of ...
84+
*/
85+
public function inheritedWithDescription(): void
86+
{
87+
throw new FooException();
88+
}
89+
90+
}

0 commit comments

Comments
 (0)