Run bootstrap-registered autoloaders inside the file-read trap - #6185
Run bootstrap-registered autoloaders inside the file-read trap#6185SanderMuller wants to merge 1 commit into
Conversation
|
Please double check whether this also fixes phpstan/phpstan#14976 |
50ebc81 to
5312948
Compare
|
Checked, and no, it does not. I ran this PR's build against the #14976 repro (the They are separate problems:
So they compose: #6069 for #14976, this for #14988. Happy to fold the reorder in here if you would rather one PR close both, but I did not want to step on #6069. |
|
Thanks for checking. Lets keep it separate |
| // If the name is already a defined function, do not run the autoloaders to find a class of | ||
| // that name. A catch-all bootstrap autoloader (e.g. PHP_CodeSniffer's, which falls back to | ||
| // Composer's findFile()) would resolve it to the function's own file and plain-include it a | ||
| // second time - it was loaded once already, e.g. by a package that ships one function per | ||
| // PSR-4 path and requires it from its bootstrap - fatally redeclaring the function. The | ||
| // class being probed for is not in that file anyway. See https://github.com/phpstan/phpstan/issues/14988 | ||
| if (function_exists($className)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
does this make sense? php cannot autoload functions and there might exist a same named class and function without problems at the same time, see https://3v4l.org/QkbUH#veol
There was a problem hiding this comment.
Good point that a class and a function can share a name. It does make sense: returning null here is the SourceLocator "I decline, try the next locator" signal, not "no such class", so the class is still resolved by the later locators in the chain.
Your exact example (class Abc {} and function abc() {}, both defined) never reaches this line. The class_exists($className, false) check just above returns null first, so the class is found by runtime reflection as before.
The only state this guard is reached in is: the function is defined but the class is not (they cannot both be in the same already-loaded file, or class_exists would be true). If a class of that name genuinely exists in another file, it is still located by the downstream AutoloadSourceLocator. I checked with a function loaded eagerly plus a class of the same FQN in a separate autoloadable file, and PHPStan resolves new \Coexist\Thing() to Coexist\Thing and its property to int.
What the guard prevents is only this locator running the bootstrap autoloaders untrapped for a name that is already a function, which is what re-includes the function's file and fatals. I reworded the comment to make the "decline, not deny" intent explicit.
AutoloadFunctionsSourceLocator runs the autoloaders collected from bootstrap files to find a class, without first checking whether the name is already a defined function. A catch-all bootstrap autoloader then resolves a name like VeeWee\Xml\Dom\Builder\value to the file that defines the function and plain-includes it. That file was already loaded once - veewee/xml ships one function per PSR-4 path and requires it from its files-autoload bootstrap - so the second include fatally redeclares the function and the worker dies with "Cannot redeclare function ...". The common real-world autoloader here is PHP_CodeSniffer's, loaded via bootstrapFiles because the package ships no Composer autoload metadata; it falls back to Composer's findFile() for any name and includes with a plain include. The name is probed as a class and the file holds a function, so running the autoloader cannot find a class there; it only re-includes the file. Return early when the name is already a defined function, the same way the locator already returns early for an existing class, interface or trait. A class that genuinely exists under that name in another file is still found by the later source locators in the chain. Wrapping the autoloader call in FileReadTrapStreamWrapper (as AutoloadSourceLocator does) was considered but is not viable here: these bootstrap autoloaders run for real, and some read a file for its return value (a loader delegating via "$loader = include ...; $loader->loadClass()") or write cache files, which the trap breaks. e2e/bug-14988 reproduces it: an unguarded one-function-per-file, a bootstrap that requires it, and a catch-all autoloader shaped like PHP_CodeSniffer's. Before the change the worker fatals with the redeclare; after it, analysis completes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5312948 to
49852e5
Compare
AutoloadFunctionsSourceLocatorruns the autoloaders collected from bootstrap files to find a class, without first checking whether the name is already a defined function. When a catch-all bootstrap autoloader is present, it resolves a name likeVeeWee\Xml\Dom\Builder\valueto the file that defines the function and includes it. That file was already loaded once (veewee/xmlships one function per PSR-4 path and requires it from its files-autoload bootstrap, andvalue.phpis a barefunction value()), so the second include fatally redeclares it and the worker dies withCannot redeclare function VeeWee\Xml\Dom\Builder\value().The real-world trigger, from @LucasHantz, is PHP_CodeSniffer's autoloader, loaded through
bootstrapFiles:This is the documented way to make PHPCS's classes resolvable, since the package ships no Composer autoload metadata, so it is reached by any project that lints its own sniffs or reports. It reaches the crash because it registers as a string callable (
Foo\Autoload::load), whichspl_autoload_functions()returns as['Foo\Autoload', 'load']so it survives theis_object($fn[0]) && get_class($fn[0]) === Composer\Autoload\ClassLoader::classexclusion inbin/phpstan; it is catch-all (it falls back to Composer'sfindFile()); and it uses a plaininclude.Fix
The name is probed as a class and the file holds a function, so running the autoloader cannot find a class there, it only re-includes the file. Return early when the name is already a defined function, the same way the locator already returns early for an existing class, interface or trait. A class that genuinely exists under that name in another file is still found by the later source locators in the chain.
I first tried wrapping the autoloader call in
FileReadTrapStreamWrapper, asAutoloadSourceLocatordoes. That stops the redeclare, but it is not viable here: these bootstrap autoloaders run for real, and some read a file for its return value (a loader delegating via$loader = include ...; $loader->loadClass(), e.g. Rector's) or write cache files (RobotLoader), which the trap breaks. The e2e suite caught both, so the trap approach is out.Test
e2e/bug-14988reproduces it self-contained (so it cannot silently stop reproducing when PHPCS changes): an unguarded one-function-per-file, a bootstrap that requires it, and a catch-all autoloader shaped like PHP_CodeSniffer's (string callable, Composer-findFilefallback, plaininclude). Before the change the worker fatals with the redeclare; after it, analysis completes.Follow-up (out of scope here)
The
ClassLoaderexclusion exists only inbin/phpstan. The second place that collects autoloaders, thebootstrapFilespass inCommandHelper, has no such exclusion, so a bootstrap file registering a genuineComposer\Autoload\ClassLoaderis collected too. That is a separate tidy-up and is not needed for this fix.Thanks to @LucasHantz for the precise diagnosis and the PHPCS reproduction.
Closes phpstan/phpstan#14988