Skip to content

Commit d6ba2c4

Browse files
janedbaljanedbal
authored
Solution to partial analysis false positives over inline ignores (#292)
1 parent 6c23c61 commit d6ba2c4

File tree

6 files changed

+238
-1
lines changed

6 files changed

+238
-1
lines changed

‎README.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -487,6 +487,20 @@ If you set up `editorUrl` [parameter](https://phpstan.org/user-guide/output-form
487487
- If you mark such methods with `@api` phpdoc, those will be considered entrypoints
488488
- You can also mark whole class or interface with `@api` to mark all its methods as entrypoints
489489

490+
## Running PHPStan partial analysis:
491+
- Dead code detection can be reliable executed only on full codebase, thus it gets autodisabled during partial analysis (when only files are passed to PHPStan analysis)
492+
- In such cases, PHPStan will report `No error with identifier shipmonk.deadMethod is reported on line X` [false positives](https://github.com/phpstan/phpstan/issues/12328) for every inline ignore (e.g. `// @phpstan-ignore shipmonk.deadMethod`) as those errors are no longer emitted
493+
- To eliminate those false positives, use built-in formatter that filters out those errors:
494+
```neon
495+
parameters:
496+
errorFormat: filterOutUnmatchedInlineIgnoresDuringPartialAnalysis
497+
498+
# optionally:
499+
shipmonkDeadCode:
500+
filterOutUnmatchedInlineIgnoresDuringPartialAnalysis:
501+
wrappedErrorFormatter: table
502+
```
503+
490504
## Future scope:
491505
- Dead class detection
492506
- Dead parameters detection

‎rules.neon‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@ services:
22
errorFormatter.removeDeadCode:
33
class: ShipMonk\PHPStan\DeadCode\Formatter\RemoveDeadCodeFormatter
44

5+
errorFormatter.filterOutUnmatchedInlineIgnoresDuringPartialAnalysis:
6+
class: ShipMonk\PHPStan\DeadCode\Formatter\FilterOutUnmatchedInlineIgnoresFormatter
7+
arguments:
8+
wrappedFormatter: %shipmonkDeadCode.filterOutUnmatchedInlineIgnoresDuringPartialAnalysis.wrappedErrorFormatter%
9+
identifiers: %shipmonkDeadCode.filterOutUnmatchedInlineIgnoresDuringPartialAnalysis.identifiers%
10+
511
-
612
class: ShipMonk\PHPStan\DeadCode\Hierarchy\ClassHierarchy
713
-
@@ -242,6 +248,14 @@ parameters:
242248
enabled: false
243249
debug:
244250
usagesOf: []
251+
filterOutUnmatchedInlineIgnoresDuringPartialAnalysis:
252+
wrappedErrorFormatter: table
253+
identifiers:
254+
- ShipMonk\PHPStan\DeadCode\Rule\DeadCodeRule::IDENTIFIER_METHOD
255+
- ShipMonk\PHPStan\DeadCode\Rule\DeadCodeRule::IDENTIFIER_CONSTANT
256+
- ShipMonk\PHPStan\DeadCode\Rule\DeadCodeRule::IDENTIFIER_ENUM_CASE
257+
- ShipMonk\PHPStan\DeadCode\Rule\DeadCodeRule::IDENTIFIER_PROPERTY_NEVER_READ
258+
- ShipMonk\PHPStan\DeadCode\Rule\DeadCodeRule::IDENTIFIER_PROPERTY_NEVER_WRITTEN
245259

246260
expandRelativePaths:
247261
- '[parameters][shipmonkDeadCode][usageProviders][symfony][configDir]'
@@ -317,4 +331,8 @@ parametersSchema:
317331
debug: structure([
318332
usagesOf: listOf(string())
319333
])
334+
filterOutUnmatchedInlineIgnoresDuringPartialAnalysis: structure([
335+
wrappedErrorFormatter: string()
336+
identifiers: listOf(string())
337+
])
320338
])

‎src/Formatter/FilterOutUnmatchedInlineIgnoresFormatter.php‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ShipMonk\PHPStan\DeadCode\Formatter;
4+
5+
use LogicException;
6+
use PHPStan\Analyser\Error;
7+
use PHPStan\Command\AnalysisResult;
8+
use PHPStan\Command\ErrorFormatter\ErrorFormatter;
9+
use PHPStan\Command\Output;
10+
use PHPStan\DependencyInjection\Container;
11+
use PHPStan\DependencyInjection\MissingServiceException;
12+
use function strpos;
13+
use function substr;
14+
15+
/**
16+
* This formatter solves the following issue https://github.com/phpstan/phpstan/issues/12328
17+
*/
18+
final class FilterOutUnmatchedInlineIgnoresFormatter implements ErrorFormatter
19+
{
20+
21+
private ErrorFormatter $originalFormatter;
22+
23+
/**
24+
* @var list<string>
25+
*/
26+
private array $identifiers;
27+
28+
/**
29+
* @param list<string> $identifiers
30+
*/
31+
public function __construct(
32+
Container $container,
33+
string $wrappedFormatter,
34+
array $identifiers
35+
)
36+
{
37+
try {
38+
/** @var ErrorFormatter $formatter */
39+
$formatter = $container->getService('errorFormatter.' . $wrappedFormatter);
40+
$this->originalFormatter = $formatter;
41+
} catch (MissingServiceException $e) {
42+
throw new LogicException('Invalid error formatter given: ' . $wrappedFormatter, 0, $e);
43+
}
44+
$this->identifiers = $identifiers;
45+
}
46+
47+
public function formatErrors(
48+
AnalysisResult $analysisResult,
49+
Output $output
50+
): int
51+
{
52+
if (!$this->isPartialAnalysis()) {
53+
return $this->originalFormatter->formatErrors($analysisResult, $output);
54+
}
55+
56+
$modifiedAnalysisResult = new AnalysisResult( // @phpstan-ignore phpstanApi.constructor
57+
$this->filterErrors($analysisResult->getFileSpecificErrors()),
58+
$analysisResult->getNotFileSpecificErrors(),
59+
$analysisResult->getInternalErrorObjects(),
60+
$analysisResult->getWarnings(),
61+
$analysisResult->getCollectedData(),
62+
$analysisResult->isDefaultLevelUsed(),
63+
$analysisResult->getProjectConfigFile(),
64+
$analysisResult->isResultCacheSaved(),
65+
$analysisResult->getPeakMemoryUsageBytes(),
66+
$analysisResult->isResultCacheUsed(),
67+
$analysisResult->getChangedProjectExtensionFilesOutsideOfAnalysedPaths(),
68+
);
69+
70+
return $this->originalFormatter->formatErrors($modifiedAnalysisResult, $output);
71+
}
72+
73+
/**
74+
* @param list<Error> $errors
75+
* @return list<Error>
76+
*/
77+
private function filterErrors(array $errors): array
78+
{
79+
$result = [];
80+
foreach ($errors as $error) {
81+
if (
82+
$error->getIdentifier() === 'ignore.unmatchedIdentifier'
83+
&& $this->isDeadCodeIdentifier($error->getMessage())
84+
) {
85+
continue;
86+
}
87+
88+
$result[] = $error;
89+
}
90+
91+
return $result;
92+
}
93+
94+
private function isDeadCodeIdentifier(string $message): bool
95+
{
96+
foreach ($this->identifiers as $identifier) {
97+
if (strpos($message, $identifier) !== false) {
98+
return true;
99+
}
100+
}
101+
return false;
102+
}
103+
104+
private function isPartialAnalysis(): bool
105+
{
106+
/** @var array<string> $argv */
107+
$argv = $_SERVER['argv'] ?? [];
108+
foreach ($argv as $arg) {
109+
if (substr($arg, -4) === '.php') {
110+
return true;
111+
}
112+
}
113+
114+
return false;
115+
}
116+
117+
}

‎tests/ParamsArePassedToServicesTest.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use PHPStan\Testing\PHPStanTestCase;
66
use function array_merge;
7+
use function array_values;
78
use function file_get_contents;
89
use function is_array;
910
use function strpos;
@@ -56,7 +57,15 @@ private function getArrayPaths(
5657
$newPathSegment = $currentPath === '' ? (string) $key : $currentPath . '.' . $key;
5758

5859
if (is_array($value)) {
59-
$resultPaths = array_merge($resultPaths, $this->getArrayPaths($value, $newPathSegment));
60+
if ($this->isList($value)) {
61+
// Non-empty lists are passed as a whole, add path without recursing
62+
// Empty lists are ignored (no useful value to pass)
63+
if ($value !== []) {
64+
$resultPaths[] = $newPathSegment;
65+
}
66+
} else {
67+
$resultPaths = array_merge($resultPaths, $this->getArrayPaths($value, $newPathSegment));
68+
}
6069
} else {
6170
$resultPaths[] = $newPathSegment;
6271
}
@@ -65,4 +74,12 @@ private function getArrayPaths(
6574
return $resultPaths;
6675
}
6776

77+
/**
78+
* @param array<mixed> $array
79+
*/
80+
private function isList(array $array): bool
81+
{
82+
return array_values($array) === $array;
83+
}
84+
6885
}

‎tests/Rule/DeadCodeRuleTest.php‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use PHPStan\Analyser\Scope;
1313
use PHPStan\Collectors\Collector;
1414
use PHPStan\Command\AnalysisResult;
15+
use PHPStan\Command\ErrorFormatter\ErrorFormatter;
1516
use PHPStan\Command\Output;
1617
use PHPStan\DependencyInjection\Container;
1718
use PHPStan\File\SimpleRelativePathHelper;
@@ -36,6 +37,7 @@
3637
use ShipMonk\PHPStan\DeadCode\Excluder\MemberUsageExcluder;
3738
use ShipMonk\PHPStan\DeadCode\Excluder\MixedUsageExcluder;
3839
use ShipMonk\PHPStan\DeadCode\Excluder\TestsUsageExcluder;
40+
use ShipMonk\PHPStan\DeadCode\Formatter\FilterOutUnmatchedInlineIgnoresFormatter;
3941
use ShipMonk\PHPStan\DeadCode\Formatter\RemoveDeadCodeFormatter;
4042
use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage;
4143
use ShipMonk\PHPStan\DeadCode\Hierarchy\ClassHierarchy;
@@ -737,6 +739,53 @@ function (string $file, string $content): void {
737739
self::assertSame(file_get_contents($expectedOutputFile), $this->trimFgColors($writtenOutput), "Output does not match expected: $expectedOutputFile");
738740
}
739741

742+
public function testFilterOutUnmatchedInlineIgnores(): void
743+
{
744+
$file = __DIR__ . '/data/partial-analysis/inline-ignore.php';
745+
746+
$originalArgv = $_SERVER['argv'] ?? [];
747+
748+
try {
749+
// simulate partial analysis (file path in argv)
750+
$_SERVER['argv'] = ['phpstan', 'analyse', $file];
751+
752+
$analyserErrors = $this->gatherAnalyserErrors([$file]);
753+
$receivedErrors = null;
754+
755+
$wrappedFormatter = $this->createMock(ErrorFormatter::class);
756+
$wrappedFormatter->expects(self::once())
757+
->method('formatErrors')
758+
->willReturnCallback(static function (AnalysisResult $result) use (&$receivedErrors): int {
759+
$receivedErrors = $result->getFileSpecificErrors();
760+
return 0;
761+
});
762+
763+
$container = $this->createMock(Container::class);
764+
$container->expects(self::once())
765+
->method('getService')
766+
->with('errorFormatter.table')
767+
->willReturn($wrappedFormatter);
768+
769+
$output = $this->createOutput();
770+
771+
$formatter = new FilterOutUnmatchedInlineIgnoresFormatter(
772+
$container,
773+
'table',
774+
[DeadCodeRule::IDENTIFIER_METHOD],
775+
);
776+
777+
$formatter->formatErrors($this->createAnalysisResult($analyserErrors), $output);
778+
779+
self::assertNotNull($receivedErrors);
780+
self::assertCount(2, $receivedErrors);
781+
self::assertSame('No error with identifier invalid.ignore is reported on line 5.', $receivedErrors[0]->getMessage());
782+
self::assertSame('Unused Filtering\Example::FOO2', $receivedErrors[1]->getMessage());
783+
784+
} finally {
785+
$_SERVER['argv'] = $originalArgv;
786+
}
787+
}
788+
740789
/**
741790
* @param list<Error> $errors
742791
*/

‎tests/Rule/data/partial-analysis/inline-ignore.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Filtering;
4+
5+
class Example // @phpstan-ignore invalid.ignore
6+
{
7+
8+
public const FOO1 = 1; // @phpstan-ignore shipmonk.deadConstant
9+
public const FOO2 = 2;
10+
11+
public function used(): void
12+
{
13+
$this->dead();
14+
}
15+
16+
public function dead(): void // @phpstan-ignore shipmonk.deadMethod (is filtered out)
17+
{
18+
}
19+
20+
}
21+
22+
(new Example())->used();

0 commit comments

Comments
 (0)