From 780dc3f6d6a1551703c65bbf127dd63f94cc2166 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Thu, 5 Sep 2024 13:28:14 +0200 Subject: [PATCH] Fix invalid file reported for non-overridden dead trait methods (#67) --- src/Collector/MethodDefinitionCollector.php | 9 ++- src/Rule/DeadMethodRule.php | 8 ++- tests/Rule/DeadMethodRuleTest.php | 9 ++- tests/Rule/RuleTestCase.php | 66 +++++++++++++++---- .../Rule/data/DeadMethodRule/traits-11-a.php | 9 +++ .../Rule/data/DeadMethodRule/traits-11-b.php | 7 ++ 6 files changed, 85 insertions(+), 23 deletions(-) create mode 100644 tests/Rule/data/DeadMethodRule/traits-11-a.php create mode 100644 tests/Rule/data/DeadMethodRule/traits-11-b.php diff --git a/src/Collector/MethodDefinitionCollector.php b/src/Collector/MethodDefinitionCollector.php index 95c83f4..ca41e16 100644 --- a/src/Collector/MethodDefinitionCollector.php +++ b/src/Collector/MethodDefinitionCollector.php @@ -16,7 +16,7 @@ use function strpos; /** - * @implements Collector, traitOriginDefinition: ?string}>> + * @implements Collector, traitOriginDefinition: ?string}>> */ class MethodDefinitionCollector implements Collector { @@ -28,7 +28,7 @@ public function getNodeType(): string /** * @param InClassNode $node - * @return list, traitOriginDefinition: ?string}>|null + * @return list, 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 index 9280094..6d1dc3a 100644 --- 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, traitOriginDefinition: string|null} $serializedMethodDeclaration - * @return array{line: int, definition: MethodDefinition, overriddenDefinitions: list, traitOriginDefinition: MethodDefinition|null} + * @param array{line: int, file: string, definition: string, overriddenDefinitions: list, traitOriginDefinition: string|null} $serializedMethodDeclaration + * @return array{line: int, file: string, definition: MethodDefinition, overriddenDefinitions: list, 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 index 27a0d30..02d1544 100644 --- 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 $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 + * @return array, 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 index d1d06c8..bf65517 100644 --- 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 $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 $actualErrors - * @return list + * @return array> */ 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 new file mode 100644 index 0000000..ab76678 --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/traits-11-a.php @@ -0,0 +1,9 @@ +