Repository navigation
Ask Composer's autoloaders for the file instead of letting them include it - #6431
theodorejb wants to merge 1 commit into
Conversation
2eb5402 to
3f90b11
Compare
3f90b11 to
1f51101
Compare
1f51101 to
5ea1f21
Compare
5ea1f21 to
80d5c78
Compare
|
This pull request has been marked as ready for review. |
|
The fix works from source, but in the phar the new fast path never runs, so the fatal from phpstan/phpstan#15184 is still there. php-scoper prefixes the new import. The phar that CI built for this PR (artifact The project's autoloader is an unprefixed Repro on macOS, PHP 8.5.8: a project with
On the PR head from source:
All five red checks are also red on #6564, another 2.2.x PR. I did not run the Windows setup from the issue, and I did not measure performance. |
80d5c78 to
fb40c09
Compare
|
@SanderMuller Thanks for finding the phar issue. I rebased and had Claude update the commit to add In regards to performance, two A/B runs each of PHPStan's own |
fb40c09 to
bed3f0c
Compare
SanderMuller
left a comment
There was a problem hiding this comment.
Thanks, that fixes it. With the phar CI built from bed3f0c, the php-standard-library/type repro now reports class.notFound instead of the fatal, with and without OPcache. The new build test fails without the entry, and the phar test fails without the :// guard.
On a real project of about 4,500 files the reported errors are identical. CPU did not go up: median 140.2s before and 131.4s after, over 3 cold runs each. The full suite and make phpstan pass locally. The red Checksum PHAR job is expected here: the only file that differs between the base and PR checksum phars is AutoloadSourceLocator.php.
|
@ondrejmirtes Should I rebase this against the 2.3.x branch? I don't know if there's another approach to fixing this you would prefer, but I really hope something can be done as I'm still stuck on PHPStan 2.2.12 in order to use the PSL Type library and plugin. |
bed3f0c to
34f08f3
Compare
34f08f3 to
ed07ba0
Compare
…de it AutoloadSourceLocator finds which file declares a class by running the registered autoloaders behind FileReadTrapStreamWrapper, which records the path an include reached for and serves an empty script in its place. That only shadows the real file while the compiler asks the wrapper for the contents. With OPcache already holding the script it does not ask, and the file runs a second time - fatal for one declaring a function, which is the function-per-file layout of php-standard-library and azjezz/psl: their files-autoload bootstrap has already loaded every path their PSR-4 prefix also resolves to. ClassLoader::findFile() answers with the same path loadClass() would include, without running anything, so ask it rather than arranging for the include to be harmless. Nothing is compiled and no cache is consulted, which is what makes this hold wherever PHP runs. Autoloaders still run in registration order: every non-Composer one runs inside the trap as before, one at a time, since an autoloader ahead of Composer's may claim a name Composer would resolve elsewhere. Running them one at a time also stops hoa/compiler's autoloader, registered ahead of the analysed project's loader, from forcing the whole probe down the include path. findFile() concatenates the mapped prefix with the rest of the name, so its answer can carry ../ segments and mixed separators, where PHP resolves an include path before the trap ever sees it. It is resolved here to match, which locateIdentifier() relies on when it compares the located path against ReflectionClass::getFileName() to tell two same-named classes in one file apart. In the phar, php-scoper prefixes the Composer\Autoload\ClassLoader import, but Box leaves the class itself unprefixed, so the prefixed name matches no autoloader at all. scoper.inc.php already strips that prefix back off in a fixed list of files; AutoloadSourceLocator.php joins it, and the list moves into scoper-namespaces.php so that ScoperComposerClassLoaderTest can fail when a file in src/ or bin/phpstan refers to the class without being listed. The phar's own autoloader is therefore a ClassLoader too, and it maps PHPStan\ to phar://.../src. realpath() cannot resolve a path behind a stream wrapper, so a ClassLoader that answers with one is left to the trap, which has always recorded such paths - otherwise PHPStan's own classes that are not loaded yet, PHPStan\TrinaryLogic among them, go missing when analysing code that uses them. The OPcache hazard is about files on disk. The OPcache test drives the real locator in a subprocess under the worker's own OPcache flags: a name whose PSR-4 prefix resolves to a file the process already ran, which must not run it again, and one whose file nothing has loaded, which must still resolve without executing. It runs from source, so the scoping above is left to ScoperComposerClassLoaderTest. AutoloadSourceLocatorTest covers a class inside a phar behind a ClassLoader, using a tar archive that PharData can write with phar.readonly on. Closes phpstan/phpstan#15184 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ed07ba0 to
b452f74
Compare
AutoloadSourceLocatorfinds which file declares a class by running the registered autoloaders behindFileReadTrapStreamWrapper, which records the path anincludereached for and serves an empty script in its place. That only shadows the real file while the compiler asks the wrapper for the contents. When OPcache already holds the script, it does not ask, and the file runs a second time:That is fatal for any file that declares a function, which is how
php-standard-libraryandazjezz/psllay out their code: one function per file. Theirfiles-autoload bootstrap has already loaded every path that their PSR-4 prefix also resolves to.2.2.13 exposed this by starting spawned workers with OPcache on (
ProcessHelper::resolveWorkerIniEntries()), and it still reproduces on 2.3.0. The existing code already handles the opposite direction:servesParseError()and theopcache_invalidate()loop stop the trap's empty script from being cached and shadowing the real file. A cache hit was the case that was missed.In the reported stack trace, the probe comes from
ScopeOps::containsExpressionToInvalidate(). When$thisis invalidated, it passes everyNamein stored expressions toClassReflection::is(), including function-call names, and each one becomes ahasClass()lookup. Skipping non-class names there would be a worthwhile cleanup (with the matching change inScopeOps.cpp), but it belongs in a separate PR. It also would not replace this fix, because any other path that looks up such a name as a class reaches the same trap.Fix
ClassLoader::findFile()answers with the same pathloadClass()would include, without running anything. Asking it, rather than arranging for the include to be harmless, means nothing is compiled and no cache is consulted. That is why the fix holds wherever PHP runs.FileReadTrapStreamWrapperis unchanged.Ordering
Autoloaders still run in registration order. Every non-Composer autoloader still runs inside the trap, one at a time, because an autoloader ahead of Composer's may claim a name that Composer would resolve elsewhere. Running them one at a time also matters in practice:
hoa/compilerregisters an autoloader ahead of the analysed project's loader, so stopping at the first non-Composer autoloader would almost never reach Composer's.Path resolution
findFile()joins the mapped prefix to the rest of the name, so its answer can contain../segments and mixed separators. PHP resolves an include path before the trap sees it, so the answer is resolved withrealpath()here to match.locateIdentifier()depends on this when it compares the located path againstReflectionClass::getFileName()to tell two same-named classes in one file apart, and the existingAutoloadSourceLocatorTestcatches the difference.In the phar
php-scoper prefixes the new
use Composer\Autoload\ClassLoader;, but Box leaves the class itself unprefixed. In the phar, the prefixed name would match no autoloader, and every probe would still go through the trap.compiler/build/scoper.inc.phpalready strips that prefix back off in a fixed list of files, andAutoloadSourceLocator.phpis now on that list. The list moves from the patcher closure intocompiler/build/scoper-namespaces.php, next to the scoper's other lists, so that a test can read it.ScoperComposerClassLoaderTestfails when a file insrc/orbin/phpstanrefers toComposer\Autoload\ClassLoaderwithout being listed, the same wayScoperClassNameStringsTestguards class-name strings.Checksum PHAR is expected to fail. It runs when
compiler/changes and requires the phar to be byte-identical to the base's, and this PR deliberately changes how one file is scoped. Moving the list intoscoper-namespaces.phpdoes not change the phar by itself; I can split that move out if you'd like it verified separately.Paths behind a stream wrapper
The phar's own autoloader is also an unprefixed
ClassLoader, and it mapsPHPStan\tophar://.../src.realpath()cannot resolve a path behind a stream wrapper, so when aClassLoaderanswers with such a path, the probe falls back to the trap, which has always recorded those paths. Only plain local paths take the new route. The OPcache hazard is about files on disk, so the fix still covers the case that matters. Without this fallback, PHPStan's own classes that are not loaded yet go missing when running from the phar (Call to method yes() on an unknown class PHPStan\TrinaryLogic.).Alternatives considered
Keeping the include and making it safe would mean either dropping the OPcache entry with
opcache_invalidate(), or refusing to open a file that is already inget_included_files(). Both depend on OPcache consulting the stream wrapper at all, which PHP does not promise, and both failed the Linux integration job while passing on Windows. AskingfindFile()depends on nothing like that: if the autoloader is never called, the file cannot be included.Tests
FileReadTrapStreamWrapperTest::testTrapSurvivesOpcacheCacheHit()runs the realAutoloadSourceLocatorin a subprocess with the same OPcache flags as a worker, because the failure is a fatal error and OPcache is only on in spawned processes. It checks that:Without the fix, it reproduces the reported stack:
ClassLoader->loadClass()insidewithStreamWrapperOverride(). It is in theexecgroup, whichphpunit.xmlskips by default, so run it with:AutoloadSourceLocatorTest::testClassInsidePharBehindComposerClassLoader()covers the stream-wrapper fallback. A tar archive stands in for the phar, becausePharDatacan write one even withphar.readonlyon.ScoperComposerClassLoaderTestcovers the scoping, which the other tests cannot see because they run from source. It fails without the new list entry.Checked by hand on Windows with PHP 8.5, against a project on
php-standard-library/type6.2.1 at level 5 that analysesreturn $o instanceof \Psl\Type\optional;with the worker's OPcache flags:Cannot redeclare function Psl\Type\optional()class.notFoundon theinstanceofline, no fatalI could not build a full phar on Windows, so the phar that CI builds for this PR is what confirms the end-to-end result.
Not covered
An autoloader that is not a
Composer\Autoload\ClassLoaderstill goes through the trap and is still exposed to the same OPcache behaviour. That includes wrapped loaders: Symfony'sDebugClassLoaderdelegates to aClassLoaderbut is not one. This exposure already existed before this PR. Closing it means solving the original problem, keeping anincludefrom running a file that is already in the opcode cache, without relying on the engine to consult the stream wrapper. Happy to look at that separately.Closes phpstan/phpstan#15184
🤖 Generated with Claude Code