Decline a bootstrap autoloader only when it would re-include a loaded file - #6265
Conversation
01f2943 to
c12c14e
Compare
|
@ondrejmirtes rebased onto Instead of guessing from the name it now probes the autoloaders under the file-read trap and declines only when they would re-include an already-loaded file, which is what #14988 was actually about. Diff is +218/-17 across the locator and its new test. Verified locally on the rebased head:
GitHub Actions is in a major outage right now (no runs repo-wide since 15:32Z), so the checks here will only populate once it recovers. One thing worth knowing for coverage: an |
… file The guard added for phpstan/phpstan#14988 declined this locator whenever a function of the class's name existed. Classes and functions live in separate symbol spaces, so that also blocked class names which merely coincide with a function - Laravel's facade aliases are exactly that shape, since Cache, File, Str and friends coincide with the global helpers cache(), file() and str(), and `use Cache;` started reporting class.notFound. The hazard was never the name: it was a catch-all autoloader resolving a class name to the function's own file and including it a second time. Probing the autoloaders under the file-read trap says which file they would read without executing it, so only that case declines. A loader that defines the class without reading a file - class_alias(), eval() - now runs as it did before. Closes phpstan/phpstan#15102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c12c14e to
720c7c3
Compare
|
phpstan/phpstan, where the compiled PHAR exists, also has e2e/ tests. Would be nice to reproduce it there with actual project, make sure it's red, and then make sure here it's green. |
An autoloader that resolves a short alias whose name is also a global function - Laravel's Cache, File, Str and Hash all are - is declined by the locator without the fix, so `use File;` reports class.notFound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Added as
I also checked it against the release phars on a copy of the same project, which shows the regression window:
The project is the reduced facade-alias shape: a prepended autoloader that resolves Gates on the pushed head: full suite 21157, self-analysis clean, On the real-project route you suggested: larastan is already in |
|
Thank you! |
|
Heads-up: the Laravel half of phpstan/phpstan#15102 is still red on the merged Your cache warning was the reason I caught my own bad measurement first: every run below has its own empty
Minimal reproduction (portable — no Laravel, no larastan)// composer.json
{
"require-dev": { "phpstan/phpstan": "2.2.x-dev" },
"autoload": { "psr-4": { "Repro\\": "src/" } }
}# phpstan.neon
parameters:
level: 0
paths: [src]
bootstrapFiles: [bootstrap.php]<?php // bootstrap.php - mirrors Illuminate\Foundation\AliasLoader::prependToLoaderStack()
spl_autoload_register(static function (string $class): void {
if ($class === 'File') {
class_alias(\Repro\Target::class, 'File');
}
}, true, true);<?php // src/Consumer.php (src/Target.php is any plain class)
namespace Repro;
use File;
final class Consumer
{
public function run(): File
{
return new File();
}
}Where it stopsThe phar extracts and Your #6281 split is doing the right thing here — the bootstrap closure lands in The trapped read is the alias target's file, not the function's file. For Laravel that precondition always holds: larastan's bootstrap boots the application, so Suggested narrowing: only the file that declares the function of that name if ($locatedFiles === []) {
return false;
}
+ // Only re-including the file that *declares the function of this name* redeclares it.
+ // A trapped read of any other file is not the #14988 hazard: an alias autoloader reads
+ // the file of the class it aliases to, and that file being loaded already is the normal
+ // case - class_alias() does not include anything, it names a class that is there.
+ $functionFile = (new ReflectionFunction($className))->getFileName();
+ if ($functionFile === false) {
+ return false;
+ }
// PHP canonicalises the path before it reaches a stream wrapper - a `/./` segment, a
// symlinked directory or an include-path-relative name all arrive resolved - so the
// trapped paths compare directly against get_included_files().
$includedFiles = get_included_files();
foreach ($locatedFiles as $locatedFile) {
+ if ($locatedFile !== $functionFile) {
+ continue;
+ }
if (in_array($locatedFile, $includedFiles, true)) {
return true;
}
}
return false;A builtin has no declaring file (
Red-team for the hazard the guard exists for — an autoloader that maps the class name to the file that declares the function, with that file already required at bootstrap: <?php // bootstrap.php
require __DIR__ . '/helpers.php'; // declares function sniffhelper()
spl_autoload_register(static function (string $class): void {
if ($class === 'sniffhelper') {
require __DIR__ . '/helpers.php'; // would fatally redeclare
}
}, true, true);Still declined with the patch — What I did not verify
Happy to open this as a PR with the change plus an |
|
You are right on every point, and thank you for the depth - the diagnosis, the portable repro and the red-team case all hold. Sent as #6288 with your condition, credited to you. Confirmed on the merged tip ( The part I should have caught myself: I ran the checks you flagged as decisive: Two things I did not fold in: the guard is only reachable from inside the |
|
FYI reverting this: #6292 phpstan/phpstan#15102 keeps working. |
3 participants
Fixes the second regression reported in phpstan/phpstan#15102 - the one @hoetaek reduced to a portable repro. This is not the ordering issue that opened that ticket; that one is #6069's heuristic and needs its own change.
What was wrong
#6185 added this to
AutoloadFunctionsSourceLocator:Classes and functions occupy separate symbol spaces, so a class name coinciding with a function name is legal and common. Laravel is the worst case: the facade aliases
Cache,File,Str,Hashcoincide with the global helperscache(),str()and with PHP's ownfile(),hash().Illuminate\Foundation\AliasLoadercreates those aliases lazily from a prepended autoloader, so with the guard in placeuse Cache;reportsclass.notFoundfor every larastan user. Verified against the release phars on @hoetaek's repro:File(collides withfile())Alias(no such function)What the hazard actually was
phpstan/phpstan#14988 was not about the name. A catch-all autoloader - PHP_CodeSniffer's, falling back to Composer's
findFile()- resolved a class name to a function's file and plain-included it a second time, fatally redeclaring the function.So the check now asks that question instead of guessing from the name: probe the autoloaders under
FileReadTrapStreamWrapper, which reports which file they would read without executing it, and decline only when that file is already inget_included_files(). An autoloader that defines the class without reading a file -class_alias(),eval()- runs exactly as it did in 2.2.8. The probe only runs for names that actually collide with a function, and not at all when the bucket holds no autoloaders - so a project without bootstrap autoloaders never reaches it: analysingsrc/Rulestriggers 0 probes. Where it does run,get_included_files()held ~35 entries, so the scan is nothing.One subtlety the trap forces: it intercepts file reads, not execution, so the probe really does run the autoloaders. An autoloader that defines the class without reading a file has therefore already done its work by the time the probe returns, and calling it again would redeclare what it defined -
class_alias()warns that the name is already in use. So when the class exists after the probe, the locator reflects it directly instead of looping over the autoloaders again; the test asserts the autoloader runs exactly once. For the same reason the probe stops at the first autoloader that defines the name, the wayspl_autoload_call()does - otherwise a later catch-all autoloader in the same bucket could still resolve that name to a loaded file and veto a class the first one had already defined.Verification
AutoloadFunctionsSourceLocatorTestpins two cases, both verified failing without the change: the alias case (on2.2.xthe locator declines and returns null), and a defining autoloader followed by a catch-all one in the same bucket. It also asserts the autoloader is invoked exactly once - the outcome assertion alone passes either way, only the count (2 vs 1) separates them.e2e/bug-14988still exits 0 - the redeclare fatal does not come back, it is prevented by the trap rather than by the name.e2e/bug-12972b,e2e/bug-12972c(Consult bootstrap-registered custom autoloaders only after the static source locators #6069) andCollectNewAutoloadFunctionsTestunchanged and green.2.2.x.Result cache E2E bug-11826, thephpstan-doctrinelane (Ask the node-callback scope directly instead of toMutatingScope() phpstan-doctrine#789 fixes it once 2.2.10 is out) and the integration tests, all red on every PR.Coverage:
e2e/bug-15102bcovers it - red without the fix (2xclass.notFound), green with it. An earlier version of this description claimed only a phar exhibits this; that was wrong, and came from a result cache shared between source states (same2.2.x-devcache key). Each measurement now uses a freshtmpDir.