Fix invalid file reported for non-overridden dead trait methods (#67)

shipmonk-rnd · Sep 5, 2024 · 780dc3f · 780dc3f
1 parent 5d4d631
commit 780dc3f
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 23 deletions.
diff --git a/src/Collector/MethodDefinitionCollector.php b/src/Collector/MethodDefinitionCollector.php
@@ -16,7 +16,7 @@
 use function strpos;
 
 /**
- * @implements Collector<InClassNode, list<array{line: int, definition: string, overriddenDefinitions: list<string>, traitOriginDefinition: ?string}>>
+ * @implements Collector<InClassNode, list<array{line: int, file: string, definition: string, overriddenDefinitions: list<string>, traitOriginDefinition: ?string}>>
  */
 class MethodDefinitionCollector implements Collector
 {
@@ -28,7 +28,7 @@ public function getNodeType(): string
 
     /**
      * @param InClassNode $node
-     * @return list<array{line: int, definition: string, overriddenDefinitions: list<string>, traitOriginDefinition: ?string}>|null
+     * @return list<array{line: int, file: string, definition: string, overriddenDefinitions: list<string>, traitOriginDefinition: ?string}>|null
      */
     public function processNode(
         Node $node,
@@ -51,16 +51,18 @@ public function processNode(
                 }
 
                 $traitLine = $traitMethod->getStartLine();
+                $traitFile = $traitMethod->getFileName();
                 $traitName = $trait->getName();
                 $traitMethodName = $traitMethod->getName();
                 $declaringTraitDefinition = $this->getDeclaringTraitDefinition($trait, $traitMethodName);
 
-                if ($traitLine === false) {
+                if ($traitLine === false || $traitFile === false) {
                     continue;
                 }
 
                 $result[] = [
                     'line' => $traitLine,
+                    'file' => $traitFile,
                     'definition' => (new MethodDefinition($traitName, $traitMethodName))->toString(),
                     'overriddenDefinitions' => [],
                     'traitOriginDefinition' => $declaringTraitDefinition !== null ? $declaringTraitDefinition->toString() : null,
@@ -105,6 +107,7 @@ public function processNode(
 
             $result[] = [
                 'line' => $line,
+                'file' => $scope->getFile(),
                 'definition' => $definition->toString(),
                 'overriddenDefinitions' => array_map(static fn (MethodDefinition $definition) => $definition->toString(), $overriddenDefinitions),
                 'traitOriginDefinition' => $declaringTraitDefinition !== null ? $declaringTraitDefinition->toString() : null,

diff --git a/src/Rule/DeadMethodRule.php b/src/Rule/DeadMethodRule.php
@@ -89,11 +89,12 @@ public function processNode(
 
         unset($classDeclarationData);
 
-        foreach ($methodDeclarationData as $file => $methodsInFile) {
+        foreach ($methodDeclarationData as $methodsInFile) {
             foreach ($methodsInFile as $declared) {
                 foreach ($declared as $serializedMethodDeclaration) {
                     [
                         'line' => $line,
+                        'file' => $file,
                         'definition' => $definition,
                         'overriddenDefinitions' => $overriddenDefinitions,
                         'traitOriginDefinition' => $declaringTraitMethodKey,
@@ -241,13 +242,14 @@ private function isEntryPoint(MethodDefinition $methodDefinition): bool
     }
 
     /**
-     * @param array{line: int, definition: string, overriddenDefinitions: list<string>, traitOriginDefinition: string|null} $serializedMethodDeclaration
-     * @return array{line: int, definition: MethodDefinition, overriddenDefinitions: list<MethodDefinition>, traitOriginDefinition: MethodDefinition|null}
+     * @param array{line: int, file: string, definition: string, overriddenDefinitions: list<string>, traitOriginDefinition: string|null} $serializedMethodDeclaration
+     * @return array{line: int, file: string, definition: MethodDefinition, overriddenDefinitions: list<MethodDefinition>, traitOriginDefinition: MethodDefinition|null}
      */
     private function deserializeMethodDeclaration(array $serializedMethodDeclaration): array
     {
         return [
             'line' => $serializedMethodDeclaration['line'],
+            'file' => $serializedMethodDeclaration['file'],
             'definition' => MethodDefinition::fromString($serializedMethodDeclaration['definition']),
             'overriddenDefinitions' => array_map(
                 static fn (string $definition) => MethodDefinition::fromString($definition),

diff --git a/tests/Rule/DeadMethodRuleTest.php b/tests/Rule/DeadMethodRuleTest.php
@@ -23,6 +23,7 @@
 use ShipMonk\PHPStan\DeadCode\Provider\SymfonyEntrypointProvider;
 use ShipMonk\PHPStan\DeadCode\Provider\VendorEntrypointProvider;
 use ShipMonk\PHPStan\DeadCode\Reflection\ClassHierarchy;
+use function is_array;
 use const PHP_VERSION_ID;
 
 /**
@@ -60,19 +61,20 @@ protected function getCollectors(): array
     }
 
     /**
+     * @param string|list<string> $files
      * @dataProvider provideFiles
      */
-    public function testDead(string $file, ?int $lowestPhpVersion = null): void
+    public function testDead($files, ?int $lowestPhpVersion = null): void
     {
         if ($lowestPhpVersion !== null && PHP_VERSION_ID < $lowestPhpVersion) {
             self::markTestSkipped('Requires PHP ' . $lowestPhpVersion);
         }
 
-        $this->analyseFile($file);
+        $this->analyseFiles(is_array($files) ? $files : [$files]);
     }
 
     /**
-     * @return array<string, array{0: string, 1?: int}>
+     * @return array<string, array{0: string|list<string>, 1?: int}>
      */
     public static function provideFiles(): iterable
     {
@@ -97,6 +99,7 @@ public static function provideFiles(): iterable
         yield 'trait-8' => [__DIR__ . '/data/DeadMethodRule/traits-8.php'];
         yield 'trait-9' => [__DIR__ . '/data/DeadMethodRule/traits-9.php'];
         yield 'trait-10' => [__DIR__ . '/data/DeadMethodRule/traits-10.php'];
+        yield 'trait-11' => [[__DIR__ . '/data/DeadMethodRule/traits-11-a.php', __DIR__ . '/data/DeadMethodRule/traits-11-b.php']];
         yield 'nullsafe' => [__DIR__ . '/data/DeadMethodRule/nullsafe.php'];
         yield 'dead-in-parent-1' => [__DIR__ . '/data/DeadMethodRule/dead-in-parent-1.php'];
         yield 'indirect-interface' => [__DIR__ . '/data/DeadMethodRule/indirect-interface.php'];

diff --git a/tests/Rule/RuleTestCase.php b/tests/Rule/RuleTestCase.php
@@ -6,6 +6,7 @@
 use PHPStan\Analyser\Error;
 use PHPStan\Rules\Rule;
 use PHPStan\Testing\RuleTestCase as OriginalRuleTestCase;
+use function array_diff;
 use function array_values;
 use function explode;
 use function file_get_contents;
@@ -15,6 +16,7 @@
 use function preg_match;
 use function preg_match_all;
 use function preg_replace;
+use function sort;
 use function sprintf;
 use function trim;
 use function uniqid;
@@ -26,27 +28,52 @@
 abstract class RuleTestCase extends OriginalRuleTestCase
 {
 
-    protected function analyseFile(string $file, bool $autofix = false): void
+    /**
+     * @param list<string> $files
+     */
+    protected function analyseFiles(array $files, bool $autofix = false): void
     {
-        $analyserErrors = $this->gatherAnalyserErrors([$file]);
+        sort($files);
+
+        $analyserErrors = $this->gatherAnalyserErrors($files);
 
         if ($autofix === true) {
-            $this->autofix($file, $analyserErrors);
-            self::fail("File $file was autofixed. This setup should never remain in the codebase.");
+            foreach ($files as $file) {
+                $this->autofix($file, $analyserErrors);
+            }
+
+            self::fail('Autofixed. This setup should never remain in the codebase.');
         }
 
-        $actualErrors = $this->processActualErrors($analyserErrors);
-        $expectedErrors = $this->parseExpectedErrors($file);
+        if ($analyserErrors === []) {
+            $this->expectNotToPerformAssertions();
+        }
+
+        $actualErrorsByFile = $this->processActualErrors($analyserErrors);
+
+        foreach ($actualErrorsByFile as $file => $actualErrors) {
+            $expectedErrors = $this->parseExpectedErrors($file);
+
+            $extraErrors = array_diff($expectedErrors, $actualErrors);
+            $missingErrors = array_diff($actualErrors, $expectedErrors);
 
-        self::assertSame(
-            implode("\n", $expectedErrors) . "\n",
-            implode("\n", $actualErrors) . "\n",
-        );
+            $extraErrorsString = $extraErrors === [] ? '' : "\n - Extra errors: " . implode("\n", $extraErrors);
+            $missingErrorsString = $missingErrors === [] ? '' : "\n - Missing errors: " . implode("\n", $missingErrors);
+
+            self::assertSame(
+                implode("\n", $expectedErrors) . "\n",
+                implode("\n", $actualErrors) . "\n",
+                sprintf(
+                    "Errors in file $file do not match. %s\n",
+                    $extraErrorsString . $missingErrorsString,
+                ),
+            );
+        }
     }
 
     /**
      * @param list<Error> $actualErrors
-     * @return list<string>
+     * @return array<string, list<string>>
      */
     protected function processActualErrors(array $actualErrors): array
     {
@@ -55,15 +82,22 @@ protected function processActualErrors(array $actualErrors): array
         foreach ($actualErrors as $error) {
             $usedLine = $error->getLine() ?? -1;
             $key = sprintf('%04d', $usedLine) . '-' . uniqid();
-            $resultToAssert[$key] = $this->formatErrorForAssert($error->getMessage(), $usedLine);
+            $resultToAssert[$error->getFile()][$key] = $this->formatErrorForAssert($error->getMessage(), $usedLine);
 
             self::assertNotNull($error->getIdentifier(), "Missing error identifier for error: {$error->getMessage()}");
             self::assertStringStartsWith('shipmonk.', $error->getIdentifier(), "Unexpected error identifier for: {$error->getMessage()}");
         }
 
-        ksort($resultToAssert);
+        $finalResult = [];
+
+        foreach ($resultToAssert as $file => $fileErrors) {
+            ksort($fileErrors);
+            $finalResult[$file] = array_values($fileErrors);
+        }
 
-        return array_values($resultToAssert);
+        ksort($finalResult);
+
+        return $finalResult;
     }
 
     /**
@@ -117,6 +151,10 @@ private function autofix(string $file, array $analyserErrors): void
                 throw new LogicException('Error without line number: ' . $analyserError->getMessage());
             }
 
+            if ($analyserError->getFile() !== $file) {
+                continue;
+            }
+
             $errorsByLines[$line] = $analyserError;
         }
 

diff --git a/tests/Rule/data/DeadMethodRule/traits-11-a.php b/tests/Rule/data/DeadMethodRule/traits-11-a.php
@@ -0,0 +1,9 @@
+<?php declare(strict_types = 1);
+
+namespace DeadTrait11;
+
+trait SomeTrait {
+    protected function foo(): void {} // error: Unused DeadTrait11\SomeTrait::foo
+}
+
+
diff --git a/tests/Rule/data/DeadMethodRule/traits-11-b.php b/tests/Rule/data/DeadMethodRule/traits-11-b.php
@@ -0,0 +1,7 @@
+<?php declare(strict_types = 1);
+
+namespace DeadTrait11;
+
+class User {
+    use SomeTrait;
+}