[Caching] Fix cross-file cache false negative - invalidate on ancestor change (#8493)

src/Application/ApplicationFileProcessor.php

@@ -8,6 +8,7 @@
 use PHPStan\Parser\ParserErrorsException;
 use Rector\Application\Provider\CurrentFileProvider;
 use Rector\Caching\Detector\ChangedFilesDetector;
+use Rector\Caching\Detector\FileDependencyResolver;
 use Rector\Configuration\Option;
 use Rector\Configuration\Parameter\SimpleParameterProvider;
 use Rector\FileSystem\FilesFinder;
@@ -53,6 +54,7 @@ public function __construct(
         private readonly ArrayParametersMerger $arrayParametersMerger,
         private readonly MissConfigurationReporter $missConfigurationReporter,
         private readonly Skipper $skipper,
+        private readonly FileDependencyResolver $fileDependencyResolver,
         // @experimental, see --lpt
         private readonly LptScheduleFactory $lptScheduleFactory,
         private readonly ExperimentalParallelFileProcessor $experimentalParallelFileProcessor,
@@ -201,7 +203,8 @@ private function processFile(File $file, Configuration $configuration): FileProc
             // gate on the actual content change, not on FileDiff: a FileDiff also carries reported line changes
             // that print identically, and such files would otherwise be re-processed on every dry run
             // selective runs are safe to cache now — the key is scoped to the rule selection
-            $this->changedFilesDetector->cacheFile($file->getFilePath());
+            $dependentFilePaths = $this->fileDependencyResolver->resolveDependentFilePaths($file);
+            $this->changedFilesDetector->cacheFile($file->getFilePath(), $dependentFilePaths);
         }
 
         return $fileProcessResult;

src/Caching/Detector/ChangedFilesDetector.php

@@ -46,7 +46,10 @@ public function setActiveScope(array $onlyRules, ?string $onlySuffix, array $fil
             : '|only:' . implode(',', $onlyRules) . '|suffix:' . ($onlySuffix ?? '') . '|filter:' . implode(',', $filters);
     }
 
-    public function cacheFile(string $filePath): void
+    /**
+     * @param string[] $dependentFilePaths ancestry files whose change must also invalidate this file
+     */
+    public function cacheFile(string $filePath, array $dependentFilePaths = []): void
     {
         $filePathCacheKey = $this->getFilePathCacheKey($filePath);
 
@@ -57,6 +60,7 @@ public function cacheFile(string $filePath): void
         $hash = $this->hashFile($filePath);
 
         $this->cache->save($filePathCacheKey, CacheKey::FILE_HASH_KEY, $hash);
+        $this->storeDependentFiles($filePath, $dependentFilePaths);
     }
 
     public function addCacheableFile(string $filePath): void
@@ -68,17 +72,24 @@ public function addCacheableFile(string $filePath): void
     public function hasFileChanged(string $filePath): bool
     {
         $cachedValue = $this->cache->load($this->getFilePathCacheKey($filePath), CacheKey::FILE_HASH_KEY);
+        $dependentFilesCacheKey = $this->getDependentFilesCacheKey($filePath);
 
         // a scoped (--only) run reuses the full-run cache: a file left clean by all rules stays
         // clean under a single rule too, and the content is still compared below
         if ($cachedValue === null && $this->scopeSuffix !== '') {
             $unscopedCacheKey = $this->fileHasher->hash($this->cacheKeyPath($filePath));
             $cachedValue = $this->cache->load($unscopedCacheKey, CacheKey::FILE_HASH_KEY);
+            $dependentFilesCacheKey = CacheKey::DEPENDENT_FILES_KEY . '_' . $unscopedCacheKey;
         }
 
         if ($cachedValue !== null) {
             $currentFileHash = $this->hashFile($filePath);
-            return $currentFileHash !== $cachedValue;
+            if ($currentFileHash !== $cachedValue) {
+                return true;
+            }
+
+            // the file itself is unchanged, but a parent/interface/trait it relies on might have changed
+            return $this->haveDependentFilesChanged($dependentFilesCacheKey);
         }
 
         // we don't have a value to compare against. Be defensive and assume its changed
@@ -89,6 +100,7 @@ public function invalidateFile(string $filePath): void
     {
         $fileInfoCacheKey = $this->getFilePathCacheKey($filePath);
         $this->cache->clean($fileInfoCacheKey);
+        $this->cache->clean($this->getDependentFilesCacheKey($filePath));
         unset($this->cacheableFiles[$fileInfoCacheKey]);
     }
 
@@ -107,6 +119,58 @@ public function setFirstResolvedConfigFileInfo(string $filePath): void
         $this->storeConfigurationDataHash($filePath, $configurationSnapshot);
     }
 
+    /**
+     * @param string[] $dependentFilePaths
+     */
+    private function storeDependentFiles(string $filePath, array $dependentFilePaths): void
+    {
+        $dependentFilesCacheKey = $this->getDependentFilesCacheKey($filePath);
+
+        // drop any stale dependency set from a previous run, e.g. a parent class was removed
+        if ($dependentFilePaths === []) {
+            $this->cache->clean($dependentFilesCacheKey);
+            return;
+        }
+
+        $dependentFileHashes = [];
+        foreach ($dependentFilePaths as $dependentFilePath) {
+            $dependentFileHashes[$dependentFilePath] = $this->hashFile($dependentFilePath);
+        }
+
+        $this->cache->save($dependentFilesCacheKey, CacheKey::DEPENDENT_FILES_KEY, $dependentFileHashes);
+    }
+
+    private function haveDependentFilesChanged(string $dependentFilesCacheKey): bool
+    {
+        $dependentFileHashes = $this->cache->load($dependentFilesCacheKey, CacheKey::DEPENDENT_FILES_KEY);
+        if (! is_array($dependentFileHashes)) {
+            return false;
+        }
+
+        foreach ($dependentFileHashes as $dependentFilePath => $dependentFileHash) {
+            if (! is_string($dependentFilePath) || ! is_string($dependentFileHash)) {
+                // unexpected shape, be defensive and treat as changed
+                return true;
+            }
+
+            // a dependency disappeared, the file must be reconsidered
+            if (! is_file($dependentFilePath)) {
+                return true;
+            }
+
+            if ($this->hashFile($dependentFilePath) !== $dependentFileHash) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    private function getDependentFilesCacheKey(string $filePath): string
+    {
+        return CacheKey::DEPENDENT_FILES_KEY . '_' . $this->getFilePathCacheKey($filePath);
+    }
+
     private function getFilePathCacheKey(string $filePath): string
     {
         return $this->fileHasher->hash($this->cacheKeyPath($filePath) . $this->scopeSuffix);

src/Caching/Detector/FileDependencyResolver.php

@@ -0,0 +1,95 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Caching\Detector;
+
+use PhpParser\Node\Stmt\ClassLike;
+use PHPStan\Reflection\ClassReflection;
+use PHPStan\Reflection\ReflectionProvider;
+use Rector\NodeNameResolver\NodeNameResolver;
+use Rector\PhpParser\Node\BetterNodeFinder;
+use Rector\ValueObject\Application\File;
+use Throwable;
+
+/**
+ * Resolves the class-ancestry files a file relies on (parents, interfaces, traits).
+ * A change in any of them can change this file's refactoring result, so they must
+ * invalidate its cache entry - see https://github.com/rectorphp/rector-src/issues/8493
+ *
+ * @see \Rector\Tests\Caching\Detector\FileDependencyResolverTest
+ */
+final readonly class FileDependencyResolver
+{
+    public function __construct(
+        private BetterNodeFinder $betterNodeFinder,
+        private NodeNameResolver $nodeNameResolver,
+        private ReflectionProvider $reflectionProvider,
+    ) {
+    }
+
+    /**
+     * @return string[] real paths of the ancestry files, excluding the file itself
+     */
+    public function resolveDependentFilePaths(File $file): array
+    {
+        $classLikes = $this->betterNodeFinder->findInstanceOf($file->getNewStmts(), ClassLike::class);
+        if ($classLikes === []) {
+            return [];
+        }
+
+        $dependentFilePaths = [];
+        foreach ($classLikes as $classLike) {
+            $className = $this->nodeNameResolver->getName($classLike);
+            if ($className === null) {
+                continue;
+            }
+
+            if (! $this->reflectionProvider->hasClass($className)) {
+                continue;
+            }
+
+            $classReflection = $this->reflectionProvider->getClass($className);
+
+            // best-effort: resolving ancestors forces full reflection (incl. docblock parsing) and may
+            // throw on unresolvable or malformed ancestors; a miss here only skips a cache optimisation
+            try {
+                $ancestorReflections = $this->resolveAncestors($classReflection);
+            } catch (Throwable) {
+                continue;
+            }
+
+            foreach ($ancestorReflections as $ancestorReflection) {
+                $fileName = $ancestorReflection->getFileName();
+                if ($fileName === null) {
+                    continue;
+                }
+
+                $dependentFilePaths[$this->normalizePath($fileName)] = true;
+            }
+        }
+
+        unset($dependentFilePaths[$this->normalizePath($file->getFilePath())]);
+
+        return array_keys($dependentFilePaths);
+    }
+
+    /**
+     * @return ClassReflection[]
+     */
+    private function resolveAncestors(ClassReflection $classReflection): array
+    {
+        return [
+            ...$classReflection->getParents(),
+            ...$classReflection->getInterfaces(),
+            ...$classReflection->getTraits(),
+        ];
+    }
+
+    private function normalizePath(string $filePath): string
+    {
+        $realPath = realpath($filePath);
+
+        return $realPath !== false ? $realPath : $filePath;
+    }
+}

src/Caching/Enum/CacheKey.php

@@ -12,4 +12,6 @@ final class CacheKey
     public const string CONFIGURATION_HASH_KEY = 'configuration_hash';
 
     public const string FILE_HASH_KEY = 'file_hash';
+
+    public const string DEPENDENT_FILES_KEY = 'dependent_files';
 }

tests/Application/ApplicationFileProcessor/CacheFileDependencyTest.php

@@ -0,0 +1,108 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Application\ApplicationFileProcessor;
+
+use Nette\Utils\FileSystem;
+use Rector\Application\ApplicationFileProcessor;
+use Rector\Caching\Detector\ChangedFilesDetector;
+use Rector\StaticReflection\DynamicSourceLocatorDecorator;
+use Rector\Testing\PHPUnit\AbstractLazyTestCase;
+use Rector\ValueObject\Configuration;
+
+/**
+ * Reproduces the cross-file cache false negative of https://github.com/rectorphp/rector-src/issues/8493
+ *
+ * A child's output depends on its parent, but the cache keys the child only by its own contents.
+ * When the parent changes and the child does not, the child's entry still looks valid, so the child
+ * is skipped and the transformation it now needs is silently missed.
+ */
+final class CacheFileDependencyTest extends AbstractLazyTestCase
+{
+    private ApplicationFileProcessor $applicationFileProcessor;
+
+    private ChangedFilesDetector $changedFilesDetector;
+
+    private string $temporaryDirectory;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        self::$rectorConfig = null;
+        $this->bootFromConfigFiles([__DIR__ . '/config-parent-return-type.php']);
+
+        $this->applicationFileProcessor = $this->make(ApplicationFileProcessor::class);
+        $this->changedFilesDetector = $this->make(ChangedFilesDetector::class);
+
+        $this->temporaryDirectory = sys_get_temp_dir() . '/_rector_cache_file_dependency_test_src';
+    }
+
+    protected function tearDown(): void
+    {
+        $this->changedFilesDetector->clear();
+        FileSystem::delete($this->temporaryDirectory);
+    }
+
+    public function testChildIsReconsideredWhenParentChanges(): void
+    {
+        $parentFilePath = $this->temporaryDirectory . '/ParentClass.php';
+        $childFilePath = $this->temporaryDirectory . '/ChildClass.php';
+
+        // parent method has no return type yet, so the child gets nothing to copy
+        FileSystem::write($parentFilePath, $this->createParentContents(''));
+        FileSystem::write($childFilePath, $this->createChildContents());
+
+        // register the files for static reflection, as the process command does for a real run
+        $this->make(DynamicSourceLocatorDecorator::class)
+            ->addPaths([$this->temporaryDirectory]);
+
+        $this->applicationFileProcessor->processFiles(
+            [$parentFilePath, $childFilePath],
+            new Configuration(isDryRun: true)
+        );
+
+        // both files are clean on this run and cached as such
+        $this->assertFalse($this->changedFilesDetector->hasFileChanged($childFilePath));
+
+        // the parent now declares a return type the child should inherit on the next run
+        FileSystem::write($parentFilePath, $this->createParentContents(': string'));
+
+        // the child's own contents are untouched, yet its output now depends on the changed parent,
+        // so it must be reconsidered rather than served from the stale cache entry (#8493)
+        $this->assertTrue($this->changedFilesDetector->hasFileChanged($childFilePath));
+    }
+
+    private function createParentContents(string $returnType): string
+    {
+        return <<<PHP
+            <?php
+
+            namespace RectorCacheDependency;
+
+            class ParentClass
+            {
+                public function run(){$returnType}
+                {
+                }
+            }
+            PHP;
+    }
+
+    private function createChildContents(): string
+    {
+        return <<<'PHP'
+            <?php
+
+            namespace RectorCacheDependency;
+
+            class ChildClass extends ParentClass
+            {
+                public function run()
+                {
+                }
+            }
+            PHP;
+    }
+}

tests/Application/ApplicationFileProcessor/config-parent-return-type.php

@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\Config\RectorConfig;
+use Rector\TypeDeclaration\Rector\ClassMethod\AddReturnTypeDeclarationBasedOnParentClassMethodRector;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->cacheDirectory(sys_get_temp_dir() . '/_rector_cache_file_dependency_test');
+    $rectorConfig->rule(AddReturnTypeDeclarationBasedOnParentClassMethodRector::class);
+};