Bugfix: Autoloaders require source, not the file

.github/workflows/ci.yml

@@ -56,6 +56,11 @@ jobs:
       - name: Static code analysis
         run: composer analyze
 
+      - name: Autoloader smoke test
+        env:
+          REQUIRE_TURBO: '1'
+        run: composer smoke-test
+
       - name: Unittests
         if: matrix.coveralls == false
         run: composer test

CHANGELOG.md

@@ -2,6 +2,24 @@
 
 All notable changes to this project will be documented in this file, in reverse chronological order by release.
 
+## Unreleased
+
+### Added
+
+- A CI smoke test (`composer smoke-test`) that runs a real analysis over a generated fixture project and fails when the generated-class autoloaders report an internal error.
+
+### Deprecated
+
+- Nothing.
+
+### Removed
+
+- `bitExpert\PHPStan\Magento\Autoload\Cache\FileCacheStorage`, replaced by `bitExpert\PHPStan\Magento\Autoload\Cache\GeneratedFileCache`. The autoloaders now take that class in place of `PHPStan\Cache\Cache`, and the generated classes move from `%tmpDir%/cache/PHPStan` to `%tmpDir%/cache/bitExpert/PHPStanMagento` so that they no longer share a directory with PHPStan's own cache.
+
+### Fixed
+
+- Fix "Internal error: Failed opening required '<?php ...'" for every generated Factory, Proxy, Extension and ExtensionInterface class on PHPStan 2.2.10 and PHP 8.3 or later. `FileCacheStorage` stored the generated source but returned a file path from `load()`, which a `PHPStan\Cache\CacheStorage` must not do. Once PHPStan served cache entries from its shared memory arena, `Cache::load()` returned the source code and the autoloaders required that in place of the file.
+
 ## 0.43.0
 
 ### Added

bin/ci_autoloader_smoke

@@ -0,0 +1,161 @@
+#!/usr/bin/env php
+<?php
+
+/*
+ * This file is part of the phpstan-magento package.
+ *
+ * (c) bitExpert AG
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+declare(strict_types=1);
+
+/*
+ * Runs a real PHPStan analysis against a generated fixture project and fails when the autoloaders
+ * report an internal error.
+ *
+ * The fixture only asks for factories, which is enough: all four generated-class autoloaders
+ * share the store-then-require mechanism this guards.
+ *
+ * The autoloaders generate a class, store it and then require the stored file. Whenever PHPStan
+ * changes how it hands cached values back, that round trip can start to return the source code in
+ * place of the path, which turns every generated class into
+ * "Internal error: Failed opening required '<?php ...'". Unit tests cannot see this: it only shows
+ * with a cold cache, more than one analysis process, and PHPStan's turbo extension active.
+ *
+ * Set REQUIRE_TURBO=1 (the CI job does) to also fail when PHPStan's shared memory arena is not
+ * usable, so that the guard cannot pass without exercising the code path it protects.
+ */
+
+$repoRoot = dirname(__DIR__);
+$fixtureRoot = sys_get_temp_dir() . '/phpstan-magento-smoke-' . uniqid();
+$numberOfFiles = 100;
+
+function removeDirectory(string $directory): void
+{
+    if (!is_dir($directory)) {
+        return;
+    }
+
+    $entries = new RecursiveIteratorIterator(
+        new RecursiveDirectoryIterator($directory, FilesystemIterator::SKIP_DOTS),
+        RecursiveIteratorIterator::CHILD_FIRST
+    );
+
+    foreach ($entries as $entry) {
+        /** @var SplFileInfo $entry */
+        $entry->isDir() ? rmdir($entry->getPathname()) : unlink($entry->getPathname());
+    }
+
+    rmdir($directory);
+}
+
+function run(string $repoRoot, string $command, string $config): array
+{
+    $commandLine = sprintf(
+        '%s %s %s --configuration=%s 2>&1',
+        escapeshellarg(PHP_BINARY),
+        escapeshellarg($repoRoot . '/vendor/bin/phpstan'),
+        $command,
+        escapeshellarg($config)
+    );
+
+    $output = [];
+    $exitCode = 0;
+    exec($commandLine, $output, $exitCode);
+
+    return [implode(PHP_EOL, $output), $exitCode];
+}
+
+/*
+ * The arena is what makes the cache round trip observable, and the extension being loaded does not
+ * mean the arena works: on macOS the extension loads but never hands out an arena. Probe it the way
+ * PHPStan does, with the binary PHPStan itself reports.
+ */
+function arenaIsUsable(string $diagnoseOutput): bool
+{
+    if (preg_match('#^Turbo worker binary: (\S+)#m', $diagnoseOutput, $matches) !== 1) {
+        return false;
+    }
+
+    $probe = sprintf(
+        '%s -d %s -r %s 2>&1',
+        escapeshellarg(PHP_BINARY),
+        escapeshellarg('extension=' . $matches[1]),
+        escapeshellarg('echo \PHPStanTurbo\ArenaCache::create("probe") === null ? "no" : "yes";')
+    );
+
+    $output = [];
+    $exitCode = 0;
+    exec($probe, $output, $exitCode);
+
+    return $exitCode === 0 && in_array('yes', $output, true);
+}
+
+mkdir($fixtureRoot . '/src', 0777, true);
+
+for ($i = 1; $i <= $numberOfFiles; $i++) {
+    file_put_contents(
+        sprintf('%s/src/Consumer%d.php', $fixtureRoot, $i),
+        <<<PHP
+        <?php declare(strict_types=1);
+
+        namespace Smoke;
+
+        class Consumer{$i}
+        {
+            public function useFactory(\\Smoke\\Thing{$i}Factory \$factory): void
+            {
+                \$factory->create();
+            }
+        }
+
+        PHP
+    );
+}
+
+// A cold tmpDir matters: with a warm cache the autoloaders find the generated file before they
+// ever ask PHPStan to store anything, which hides the very problem this guard looks for.
+file_put_contents($fixtureRoot . '/phpstan.neon', <<<NEON
+includes:
+    - {$repoRoot}/extension.neon
+
+parameters:
+    tmpDir: {$fixtureRoot}/tmp
+    level: 2
+    paths:
+        - {$fixtureRoot}/src
+    magento:
+        magentoRoot: {$fixtureRoot}
+    parallel:
+        jobSize: 5
+        minimumNumberOfJobsPerProcess: 1
+        maximumNumberOfProcesses: 4
+
+NEON);
+
+[$diagnoseOutput] = run($repoRoot, 'diagnose', $fixtureRoot . '/phpstan.neon');
+$arenaIsUsable = strpos($diagnoseOutput, 'Turbo extension: enabled (') !== false
+    && arenaIsUsable($diagnoseOutput);
+
+echo 'PHPStan shared memory arena usable: ' . ($arenaIsUsable ? 'yes' : 'no') . PHP_EOL;
+
+if (!$arenaIsUsable && getenv('REQUIRE_TURBO') === '1') {
+    fwrite(STDERR, PHP_EOL . 'The shared memory arena is not usable, thus this guard would pass' . PHP_EOL);
+    fwrite(STDERR, 'without exercising the code path it protects. PHPStan reported:' . PHP_EOL . PHP_EOL);
+    fwrite(STDERR, $diagnoseOutput . PHP_EOL);
+    removeDirectory($fixtureRoot);
+    exit(1);
+}
+
+[$analyseOutput, $exitCode] = run($repoRoot, 'analyse --no-progress', $fixtureRoot . '/phpstan.neon');
+removeDirectory($fixtureRoot);
+
+if (strpos($analyseOutput, 'Internal error') !== false || $exitCode !== 0) {
+    fwrite(STDERR, PHP_EOL . 'The generated-class autoloaders did not survive a real analysis run:' . PHP_EOL . PHP_EOL);
+    fwrite(STDERR, $analyseOutput . PHP_EOL);
+    exit(1);
+}
+
+echo 'Autoloader smoke test passed.' . PHP_EOL;

composer.json

@@ -63,6 +63,7 @@
     "cs-fix": "vendor/bin/phpcbf",
     "check-license": "vendor/bin/license-checker check",
     "analyze": "vendor/bin/phpstan analyze",
+    "smoke-test": "@php bin/ci_autoloader_smoke",
     "test": "vendor/bin/phpunit",
     "coverage": "vendor/bin/phpunit --coverage-clover clover.xml"
   },

extension.neon

@@ -61,16 +61,11 @@ services:
 		class: bitExpert\PHPStan\Magento\Rules\ResourceModelsShouldBeUsedDirectlyRule
 	-
 		class: bitExpert\PHPStan\Magento\Rules\SetTemplateDisallowedForBlockRule
-	fileCacheStorage:
-		class: bitExpert\PHPStan\Magento\Autoload\Cache\FileCacheStorage
+	generatedFileCache:
+		class: bitExpert\PHPStan\Magento\Autoload\Cache\GeneratedFileCache
 		arguments:
-			directory: %tmpDir%/cache/PHPStan
+			directory: %tmpDir%/cache/bitExpert/PHPStanMagento
 			magentoRoot: %magento.magentoRoot%
-	autoloaderCache:
-		autowired: false
-		class: PHPStan\Cache\Cache
-		arguments:
-			storage: @fileCacheStorage
 	extensionAttributeDataProvider:
 		class: bitExpert\PHPStan\Magento\Autoload\DataProvider\ExtensionAttributeDataProvider
 		arguments:
@@ -92,29 +87,29 @@ services:
 	factoryAutoloader:
 		class: bitExpert\PHPStan\Magento\Autoload\FactoryAutoloader
 		arguments:
-			cache: @autoloaderCache
+			cache: @generatedFileCache
 			classLoaderProvider: @classLoaderProvider
 		tags:
 			- phpstan.magento.autoloader
 	proxyAutoloader:
 		class: bitExpert\PHPStan\Magento\Autoload\ProxyAutoloader
 		arguments:
-			cache: @autoloaderCache
+			cache: @generatedFileCache
 			classLoaderProvider: @classLoaderProvider
 		tags:
 			- phpstan.magento.autoloader
 	extensionInterfaceAutoloader:
 		class: bitExpert\PHPStan\Magento\Autoload\ExtensionInterfaceAutoloader
 		arguments:
-			cache: @autoloaderCache
+			cache: @generatedFileCache
 			classLoaderProvider: @classLoaderProvider
 			attributeDataProvider: @extensionAttributeDataProvider
 		tags:
 			- phpstan.magento.autoloader
 	extensionAutoloader:
 		class: bitExpert\PHPStan\Magento\Autoload\ExtensionAutoloader
 		arguments:
-			cache: @autoloaderCache
+			cache: @generatedFileCache
 			classLoaderProvider: @classLoaderProvider
 			attributeDataProvider: @extensionAttributeDataProvider
 		tags:

phpstan.neon

@@ -7,21 +7,6 @@ parameters:
         - src/bitExpert
         - tests/bitExpert
     ignoreErrors:
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: src/bitExpert/PHPStan/Magento/Autoload/Cache/FileCacheStorage.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: src/bitExpert/PHPStan/Magento/Autoload/FactoryAutoloader.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: src/bitExpert/PHPStan/Magento/Autoload/ProxyAutoloader.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: src/bitExpert/PHPStan/Magento/Autoload/ExtensionAutoloader.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: src/bitExpert/PHPStan/Magento/Autoload/ExtensionInterfaceAutoloader.php
         -
             message: '~is not covered by backward compatibility promise.~'
             path: src/bitExpert/PHPStan/Magento/Reflection/Framework/Session/SessionManagerMagicMethodReflectionExtension.php
@@ -37,24 +22,9 @@ parameters:
         -
             message: '~Function is_subclass_of\(\) is a runtime reflection concept that might not work in PHPStan~'
             path: 'src/bitExpert/PHPStan/Magento/Type/TestFrameworkObjectManagerDynamicReturnTypeExtension.php'
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: tests/bitExpert/PHPStan/Magento/Autoload/FactoryAutoloaderUnitTest.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: tests/bitExpert/PHPStan/Magento/Autoload/ProxyAutoloaderUnitTest.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: tests/bitExpert/PHPStan/Magento/Autoload/RegistrationUnitTest.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionAutoloaderUnitTest.php
         -
             message: '~Parameter #1 \$argument of class ReflectionClass constructor expects class-string<MyUncachedExtension>|MyUncachedExtension, string given~'
             path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionAutoloaderUnitTest.php
-        -
-            message: '~is not covered by backward compatibility promise.~'
-            path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionInterfaceAutoloaderUnitTest.php
         -
             message: '~Parameter #1 \$argument of class ReflectionClass constructor expects class-string<UncachedExtensionInterface>|UncachedExtensionInterface, string given~'
             path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionInterfaceAutoloaderUnitTest.php

src/bitExpert/PHPStan/Magento/Autoload/Cache/FileCacheStorage.php → src/bitExpert/PHPStan/Magento/Autoload/Cache/GeneratedFileCache.php

@@ -12,9 +12,15 @@
 
 namespace bitExpert\PHPStan\Magento\Autoload\Cache;
 
-use PHPStan\Cache\CacheStorage;
-
-class FileCacheStorage implements CacheStorage
+/**
+ * Stores the source code the autoloaders generate and hands back the path of the file holding it.
+ *
+ * This deliberately is not a PHPStan\Cache\CacheStorage: the autoloaders need the path of the
+ * generated file to require it, while a CacheStorage has to return what was handed to save().
+ * Mixing the two made the autoloaders require the source code itself once PHPStan started to
+ * serve cache entries from its shared memory arena.
+ */
+class GeneratedFileCache
 {
     /**
      * @var string
@@ -27,9 +33,10 @@ class FileCacheStorage implements CacheStorage
     private $magentoRoot;
 
     /**
-     * FileCacheStorage constructor.
+     * GeneratedFileCache constructor.
      *
      * @param string $directory
+     * @param string $magentoRoot
      */
     public function __construct(string $directory, string $magentoRoot)
     {
@@ -38,39 +45,41 @@ public function __construct(string $directory, string $magentoRoot)
     }
 
     /**
+     * Returns the path of the file generated for the given key, null when nothing was generated yet.
+     *
      * @param string $key
-     * @param string $variableKey
-     * @return mixed
+     * @return string|null
      */
-    public function load(string $key, string $variableKey)
+    public function getFile(string $key): ?string
     {
-        return (function (string $key): ?string {
-            $cacheDir = $this->getCacheDir($key);
-            $cacheFile = $this->getCacheFile($key);
-            if (!is_file($cacheDir . '/' . $cacheFile)) {
-                return null;
-            }
+        $cacheFile = $this->getCacheDir($key) . '/' . $this->getCacheFile($key);
+        if (!is_file($cacheFile)) {
+            return null;
+        }
 
-            return $cacheDir . '/' . $cacheFile;
-        })($key);
+        return $cacheFile;
     }
 
     /**
+     * Writes the given source code and returns the path of the file holding it.
+     *
      * @param string $key
-     * @param string $variableKey
-     * @param mixed $data
+     * @param string $contents
+     * @return string
      */
-    public function save(string $key, string $variableKey, $data): void
+    public function putFile(string $key, string $contents): string
     {
         $cacheDir = $this->getCacheDir($key);
-        $cacheFile = $this->getCacheFile($key);
+        $cacheFile = $cacheDir . '/' . $this->getCacheFile($key);
         $this->makeDir($cacheDir);
-        $tmpSuccess = @file_put_contents($cacheDir . '/' . $cacheFile, $data);
-        if ($tmpSuccess === false) {
+        $written = @file_put_contents($cacheFile, $contents);
+        if ($written === false) {
             throw new \InvalidArgumentException(
-                sprintf('Could not write data to cache file %s.', $cacheDir . '/' . $cacheFile)
+                sprintf('Could not write data to cache file %s.', $cacheFile)
             );
         }
+
+        return $cacheFile;
     }
 
     /**