Skip to content

Commit 783fd93

Browse files
janedbaljanedbal
committed
Properly track transitive calls from unsupported methods
1 parent cab8dab commit 783fd93

File tree

11 files changed

+167
-85
lines changed

11 files changed

+167
-85
lines changed

‎src/Collector/ClassDefinitionCollector.php

Lines changed: 4 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
use PhpParser\Node\Name;
88
use PhpParser\Node\Stmt\Class_;
99
use PhpParser\Node\Stmt\ClassLike;
10-
use PhpParser\Node\Stmt\ClassMethod;
1110
use PhpParser\Node\Stmt\Enum_;
1211
use PhpParser\Node\Stmt\Interface_;
1312
use PhpParser\Node\Stmt\Trait_;
@@ -16,15 +15,15 @@
1615
use PHPStan\Analyser\Scope;
1716
use PHPStan\Collectors\Collector;
1817
use ShipMonk\PHPStan\DeadCode\Crate\Kind;
19-
use ShipMonk\PHPStan\DeadCode\Crate\Method;
18+
use ShipMonk\PHPStan\DeadCode\Crate\Visibility;
2019
use function array_fill_keys;
2120
use function array_map;
2221

2322
/**
2423
* @implements Collector<ClassLike, array{
2524
* kind: string,
2625
* name: string,
27-
* methods: array<string, array{line: int, abstract: bool}>,
26+
* methods: array<string, array{line: int, abstract: bool, visibility: int-mask-of<Visibility::*>}>,
2827
* parents: array<string, null>,
2928
* traits: array<string, array{excluded?: list<string>, aliases?: array<string, string>}>,
3029
* interfaces: array<string, null>,
@@ -43,7 +42,7 @@ public function getNodeType(): string
4342
* @return array{
4443
* kind: string,
4544
* name: string,
46-
* methods: array<string, array{line: int, abstract: bool}>,
45+
* methods: array<string, array{line: int, abstract: bool, visibility: int-mask-of<Visibility::*>}>,
4746
* parents: array<string, null>,
4847
* traits: array<string, array{excluded?: list<string>, aliases?: array<string, string>}>,
4948
* interfaces: array<string, null>,
@@ -64,13 +63,10 @@ public function processNode(
6463
$methods = [];
6564

6665
foreach ($node->getMethods() as $method) {
67-
if ($this->isUnsupportedMethod($node, $method)) {
68-
continue;
69-
}
70-
7166
$methods[$method->name->toString()] = [
7267
'line' => $method->getStartLine(),
7368
'abstract' => $method->isAbstract() || $node instanceof Interface_,
69+
'visibility' => $method->flags & (Visibility::PUBLIC | Visibility::PROTECTED | Visibility::PRIVATE),
7470
];
7571
}
7672

@@ -163,27 +159,6 @@ private function getTraits(ClassLike $node): array
163159
return $traits;
164160
}
165161

166-
private function isUnsupportedMethod(ClassLike $class, ClassMethod $method): bool
167-
{
168-
$methodName = $method->name->toString();
169-
170-
if (Method::isUnsupported($methodName)) {
171-
return true;
172-
}
173-
174-
if ($methodName === '__construct' && $method->isPrivate()) { // e.g. classes with "denied" instantiation
175-
return true;
176-
}
177-
178-
// abstract methods in traits make sense (not dead) only when called within the trait itself, but that is hard to detect for now, so lets ignore them completely
179-
// the difference from interface methods (or abstract methods) is that those methods can be called over the interface, but you cannot call method over trait
180-
if ($class instanceof Trait_ && $method->isAbstract()) {
181-
return true;
182-
}
183-
184-
return false;
185-
}
186-
187162
private function getKind(ClassLike $node): string
188163
{
189164
if ($node instanceof Class_) {

‎src/Crate/Method.php

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22

33
namespace ShipMonk\PHPStan\DeadCode\Crate;
44

5-
use function strpos;
6-
75
/**
86
* @immutable
97
*/
@@ -28,21 +26,4 @@ public function toString(): string
2826
return $this->className . '::' . $this->methodName;
2927
}
3028

31-
public static function isUnsupported(string $methodName): bool
32-
{
33-
if ($methodName === '__destruct') {
34-
return true;
35-
}
36-
37-
if (
38-
strpos($methodName, '__') === 0
39-
&& $methodName !== '__construct'
40-
&& $methodName !== '__clone'
41-
) {
42-
return true; // magic methods like __toString, __get, __set etc
43-
}
44-
45-
return false;
46-
}
47-
4829
}

‎src/Crate/Visibility.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ShipMonk\PHPStan\DeadCode\Crate;
4+
5+
interface Visibility
6+
{
7+
8+
public const PUBLIC = 1;
9+
public const PROTECTED = 2;
10+
public const PRIVATE = 4;
11+
12+
}

‎src/Rule/DeadMethodRule.php

Lines changed: 75 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,13 @@
1212
use ShipMonk\PHPStan\DeadCode\Collector\EntrypointCollector;
1313
use ShipMonk\PHPStan\DeadCode\Collector\MethodCallCollector;
1414
use ShipMonk\PHPStan\DeadCode\Crate\Call;
15-
use ShipMonk\PHPStan\DeadCode\Crate\Method;
15+
use ShipMonk\PHPStan\DeadCode\Crate\Kind;
16+
use ShipMonk\PHPStan\DeadCode\Crate\Visibility;
1617
use ShipMonk\PHPStan\DeadCode\Hierarchy\ClassHierarchy;
1718
use function array_key_exists;
1819
use function array_keys;
1920
use function array_merge;
21+
use function explode;
2022
use function in_array;
2123
use function strpos;
2224

@@ -26,6 +28,24 @@
2628
class DeadMethodRule implements Rule
2729
{
2830

31+
private const UNSUPPORTED_MAGIC_METHODS = [
32+
'__invoke' => null,
33+
'__toString' => null,
34+
'__destruct' => null,
35+
'__call' => null,
36+
'__callStatic' => null,
37+
'__get' => null,
38+
'__set' => null,
39+
'__isset' => null,
40+
'__unset' => null,
41+
'__sleep' => null,
42+
'__wakeup' => null,
43+
'__serialize' => null,
44+
'__unserialize' => null,
45+
'__set_state' => null,
46+
'__debugInfo' => null,
47+
];
48+
2949
private ClassHierarchy $classHierarchy;
3050

3151
/**
@@ -35,7 +55,7 @@ class DeadMethodRule implements Rule
3555
* kind: string,
3656
* name: string,
3757
* file: string,
38-
* methods: array<string, array{line: int, abstract: bool}>,
58+
* methods: array<string, array{line: int, abstract: bool, visibility: int-mask-of<Visibility::*>}>,
3959
* parents: array<string, null>,
4060
* traits: array<string, array{excluded?: list<string>, aliases?: array<string, string>}>,
4161
* interfaces: array<string, null>
@@ -53,7 +73,7 @@ class DeadMethodRule implements Rule
5373
/**
5474
* @var array<string, array{string, int}> methodKey => [file, line]
5575
*/
56-
private array $deadMethods = [];
76+
private array $blackMethods = [];
5777

5878
/**
5979
* @var array<string, list<string>> caller => callee[]
@@ -117,7 +137,7 @@ public function processNode(
117137

118138
foreach ($methods as $methodName => $methodData) {
119139
$definition = $this->getMethodKey($typeName, $methodName);
120-
$this->deadMethods[$definition] = [$file, $methodData['line']];
140+
$this->blackMethods[$definition] = [$file, $methodData['line']];
121141
}
122142
}
123143

@@ -133,9 +153,7 @@ public function processNode(
133153
$callerKey = $call->caller === null || $this->isAnonymousClass($call->caller->className)
134154
? ''
135155
: $call->caller->toString();
136-
$isWhite = $call->caller === null
137-
|| $this->isAnonymousClass($call->caller->className)
138-
|| Method::isUnsupported($call->caller->methodName);
156+
$isWhite = $this->isConsideredWhite($call);
139157

140158
foreach ($this->getAlternativeCalleeKeys($call) as $possibleCalleeKey) {
141159
$this->callGraph[$callerKey][] = $possibleCalleeKey;
@@ -160,18 +178,24 @@ public function processNode(
160178
$call = Call::fromString($entrypoint);
161179

162180
foreach ($this->getAlternativeCalleeKeys($call) as $methodDefinition) {
163-
unset($this->deadMethods[$methodDefinition]);
181+
unset($this->blackMethods[$methodDefinition]);
164182
}
165183

166184
$this->markTransitiveCallsWhite($call->callee->toString());
167185
}
168186
}
169187
}
170188

189+
foreach ($this->blackMethods as $blackMethodKey => $_) {
190+
if ($this->isNeverReportedAsDead($blackMethodKey)) {
191+
unset($this->blackMethods[$blackMethodKey]);
192+
}
193+
}
194+
171195
$errors = [];
172196

173197
if ($this->reportTransitivelyDeadMethodAsSeparateError) {
174-
foreach ($this->deadMethods as $deadMethodKey => [$file, $line]) {
198+
foreach ($this->blackMethods as $deadMethodKey => [$file, $line]) {
175199
$errors[] = $this->buildError($deadMethodKey, [], $file, $line);
176200
}
177201

@@ -181,11 +205,11 @@ public function processNode(
181205
$deadGroups = $this->groupDeadMethods();
182206

183207
foreach ($deadGroups as $deadGroupKey => $deadSubgroupKeys) {
184-
[$file, $line] = $this->deadMethods[$deadGroupKey]; // @phpstan-ignore offsetAccess.notFound
208+
[$file, $line] = $this->blackMethods[$deadGroupKey]; // @phpstan-ignore offsetAccess.notFound
185209
$subGroupMap = [];
186210

187211
foreach ($deadSubgroupKeys as $deadSubgroupKey) {
188-
$subGroupMap[$deadSubgroupKey] = $this->deadMethods[$deadSubgroupKey]; // @phpstan-ignore offsetAccess.notFound
212+
$subGroupMap[$deadSubgroupKey] = $this->blackMethods[$deadSubgroupKey]; // @phpstan-ignore offsetAccess.notFound
189213
}
190214

191215
$errors[] = $this->buildError($deadGroupKey, $subGroupMap, $file, $line);
@@ -290,14 +314,14 @@ private function markTransitiveCallsWhite(string $callerKey, array $visitedKeys
290314
$visitedKeys = $visitedKeys === [] ? [$callerKey => null] : $visitedKeys;
291315
$calleeKeys = $this->callGraph[$callerKey] ?? [];
292316

293-
unset($this->deadMethods[$callerKey]);
317+
unset($this->blackMethods[$callerKey]);
294318

295319
foreach ($calleeKeys as $calleeKey) {
296320
if (array_key_exists($calleeKey, $visitedKeys)) {
297321
continue;
298322
}
299323

300-
if (!isset($this->deadMethods[$calleeKey])) {
324+
if (!isset($this->blackMethods[$calleeKey])) {
301325
continue;
302326
}
303327

@@ -321,7 +345,7 @@ private function getTransitiveDeadCalls(string $callerKey, array $visitedKeys =
321345
continue;
322346
}
323347

324-
if (!isset($this->deadMethods[$calleeKey])) {
348+
if (!isset($this->blackMethods[$calleeKey])) {
325349
continue;
326350
}
327351

@@ -346,20 +370,20 @@ private function groupDeadMethods(): array
346370
$deadMethodsWithCaller = [];
347371

348372
foreach ($this->callGraph as $caller => $callees) {
349-
if (!array_key_exists($caller, $this->deadMethods)) {
373+
if (!array_key_exists($caller, $this->blackMethods)) {
350374
continue;
351375
}
352376

353377
foreach ($callees as $callee) {
354-
if (array_key_exists($callee, $this->deadMethods)) {
378+
if (array_key_exists($callee, $this->blackMethods)) {
355379
$deadMethodsWithCaller[$callee] = true;
356380
}
357381
}
358382
}
359383

360384
$methodsGrouped = [];
361385

362-
foreach ($this->deadMethods as $deadMethodKey => $_) {
386+
foreach ($this->blackMethods as $deadMethodKey => $_) {
363387
if (isset($methodsGrouped[$deadMethodKey])) {
364388
continue;
365389
}
@@ -380,7 +404,7 @@ private function groupDeadMethods(): array
380404
}
381405

382406
// now only cycles remain, lets pick group representatives based on first occurrence
383-
foreach ($this->deadMethods as $deadMethodKey => $_) {
407+
foreach ($this->blackMethods as $deadMethodKey => $_) {
384408
if (isset($methodsGrouped[$deadMethodKey])) {
385409
continue;
386410
}
@@ -463,4 +487,37 @@ private function getMethodKey(string $typeName, string $methodName): string
463487
return $typeName . '::' . $methodName;
464488
}
465489

490+
private function isConsideredWhite(Call $call): bool
491+
{
492+
return $call->caller === null
493+
|| $this->isAnonymousClass($call->caller->className)
494+
|| array_key_exists($call->caller->methodName, self::UNSUPPORTED_MAGIC_METHODS);
495+
}
496+
497+
private function isNeverReportedAsDead(string $methodKey): bool
498+
{
499+
[$typeName, $methodName] = explode('::', $methodKey); // @phpstan-ignore offsetAccess.notFound
500+
501+
$kind = $this->typeDefinitions[$typeName]['kind'] ?? null;
502+
$abstract = $this->typeDefinitions[$typeName]['methods'][$methodName]['abstract'] ?? false;
503+
$visibility = $this->typeDefinitions[$typeName]['methods'][$methodName]['visibility'] ?? 0;
504+
505+
if ($kind === Kind::TRAIT && $abstract) {
506+
// abstract methods in traits make sense (not dead) only when called within the trait itself, but that is hard to detect for now, so lets ignore them completely
507+
// the difference from interface methods (or abstract methods) is that those methods can be called over the interface, but you cannot call method over trait
508+
return true;
509+
}
510+
511+
if ($methodName === '__construct' && ($visibility & Visibility::PRIVATE) !== 0) {
512+
// private constructors are often used to deny instantiation
513+
return true;
514+
}
515+
516+
if (array_key_exists($methodName, self::UNSUPPORTED_MAGIC_METHODS)) {
517+
return true;
518+
}
519+
520+
return false;
521+
}
522+
466523
}

‎tests/AllServicesInConfigTest.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use ShipMonk\PHPStan\DeadCode\Crate\Call;
1111
use ShipMonk\PHPStan\DeadCode\Crate\Kind;
1212
use ShipMonk\PHPStan\DeadCode\Crate\Method;
13+
use ShipMonk\PHPStan\DeadCode\Crate\Visibility;
1314
use ShipMonk\PHPStan\DeadCode\Provider\MethodEntrypointProvider;
1415
use ShipMonk\PHPStan\DeadCode\Provider\SimpleMethodEntrypointProvider;
1516
use function array_keys;
@@ -45,6 +46,7 @@ public function test(): void
4546
Call::class,
4647
Method::class,
4748
Kind::class,
49+
Visibility::class,
4850
MethodEntrypointProvider::class,
4951
SimpleMethodEntrypointProvider::class,
5052
];

‎tests/Rule/DeadMethodRuleTest.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,8 @@ public static function provideFiles(): iterable
132132
yield 'code' => [__DIR__ . '/data/DeadMethodRule/basic.php'];
133133
yield 'ctor' => [__DIR__ . '/data/DeadMethodRule/ctor.php'];
134134
yield 'ctor-interface' => [__DIR__ . '/data/DeadMethodRule/ctor-interface.php'];
135+
yield 'ctor-private' => [__DIR__ . '/data/DeadMethodRule/ctor-private.php'];
136+
yield 'ctor-denied' => [__DIR__ . '/data/DeadMethodRule/ctor-denied.php'];
135137
yield 'cycles' => [__DIR__ . '/data/DeadMethodRule/cycles.php'];
136138
yield 'abstract-1' => [__DIR__ . '/data/DeadMethodRule/abstract-1.php'];
137139
yield 'entrypoint' => [__DIR__ . '/data/DeadMethodRule/entrypoint.php'];
@@ -143,6 +145,7 @@ public static function provideFiles(): iterable
143145
yield 'overwriting-3' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-3.php'];
144146
yield 'overwriting-4' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-4.php'];
145147
yield 'overwriting-5' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-5.php'];
148+
yield 'trait-abstract' => [__DIR__ . '/data/DeadMethodRule/traits-abstract-method.php'];
146149
yield 'trait-1' => [__DIR__ . '/data/DeadMethodRule/traits-1.php'];
147150
yield 'trait-2' => [__DIR__ . '/data/DeadMethodRule/traits-2.php'];
148151
yield 'trait-3' => [__DIR__ . '/data/DeadMethodRule/traits-3.php'];

‎tests/Rule/data/DeadMethodRule/basic.php

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,4 @@ public function traitMethodUnused(): void // error: Unused DeadBasic\TestTrait::
106106
}
107107
}
108108

109-
class StaticClass {
110-
111-
private function __construct()
112-
{
113-
}
114-
}
115-
116109
new TestChild();

‎tests/Rule/data/DeadMethodRule/ctor-denied.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace CtorDenied;
4+
5+
final class Utility
6+
{
7+
private function __construct()
8+
{
9+
}
10+
}
11+

0 commit comments

Comments
 (0)