Skip to content

Decline only the file that declares the function of that name - #6288

Closed
SanderMuller wants to merge 1 commit into
phpstan:2.2.xfrom
SanderMuller:narrow-redeclare-guard
Closed

Decline only the file that declares the function of that name#6288
SanderMuller wants to merge 1 commit into
phpstan:2.2.xfrom
SanderMuller:narrow-redeclare-guard

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

Fixes the Laravel half of phpstan/phpstan#15102, which is still red on the merged tip - reported, diagnosed and patched by @hoetaek in #6265; I verified it and ran the gates he flagged as decisive.

What my #6265 guard got wrong

wouldReIncludeALoadedFile() declined on any trapped read of an already-loaded file. But an aliasing autoloader reads the file of the class it aliases to: class_alias(\Repro\Target::class, 'File') autoloads Repro\Target, and that file already being loaded is the normal case - and the precondition for success, since class_alias() includes nothing. So the guard declined exactly the case it was written to keep working, and use File; still reported class.notFound.

Reproduced on the merged tip (c10897b1f) with @hoetaek's portable repro - prepended class_alias autoloader, File colliding with the built-in file(), no Laravel involved - each run with its own empty tmpDir:

build result
2.2.8 phar No errors
tip without this change 2x class.notFound
tip with this change No errors

The narrowing

Only re-including the file that declares the function of that name can redeclare it, so the trapped read is compared against that file. A built-in has no declaring file, so there is nothing it could redeclare.

The new ReflectionFunction is deliberate runtime reflection - the hazard is a runtime redeclare - and it gets a baseline entry next to the two AutoloadSourceLocator already has for the same rule.

Why the e2e project did not catch it

e2e/bug-15102b (mine, from #6265) require_once'd its alias target in the bootstrap, so class_alias() read no file and the guard never fired. It now autoloads the target through Composer the way AliasLoader does. Verified both ways:

e2e/bug-15102b
without this change 2 errors
with this change No errors

Verification

  • @hoetaek's red-team case - an autoloader that maps a class name to the file declaring a userland function of that name, with the file already required at bootstrap - is still declined, no fatal redeclare.
  • e2e/bug-14988 (the redeclare fatal the guard exists for), bug-15102, bug-15102b, bug-12972b, bug-12972c: all exit 0.
  • Full suite 21159 green, self-analysis clean, phpcs clean over the whole repo.

Closes the Laravel half of phpstan/phpstan#15102

wouldReIncludeALoadedFile() declined on any trapped read of an
already-loaded file, but an aliasing autoloader reads the file of the
class it aliases *to*: class_alias(Target::class, 'File') autoloads
Target, and Target already being loaded is the normal case - class_alias()
includes nothing. So the guard declined the very case it was written to
keep working, and `use File;` still reported class.notFound with Laravel's
AliasLoader.

Only re-including the file that declares the function of that name can
redeclare it, so compare the trapped read against that file. A built-in
has no declaring file and nothing to redeclare.

e2e/bug-15102b required its alias target upfront, which is why it passed:
class_alias() then reads no file and the guard never fires. It now
autoloads the target like AliasLoader does, and fails without this change.

Reported and diagnosed by @hoetaek, including the suggested condition and
a red-team case for the redeclare hazard.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@SanderMuller

Copy link
Copy Markdown
Contributor Author

Red-teamed my own change, and it does narrow the guarantee - worth stating explicitly rather than leaving for someone to find.

What the narrowed guard still protects (verified, not assumed): stubbing wouldReIncludeALoadedFile() out on this branch makes e2e/bug-14988 fatal with Cannot redeclare function Redeclare\Builder\thing(); with the guard active it is [OK] No errors. So the reported #14988 hazard - a class name resolved to the file that declares the function of that name - is still declined.

What it gives up. An autoloader that maps the name to a different already-loaded file now re-includes it:

// bootstrap.php
require __DIR__ . '/helpers.php';   // declares zzcollide()
require __DIR__ . '/other.php';     // declares otherfn(), now loaded
spl_autoload_register(static function (string $class): void {
    if ($class === 'zzcollide') {
        require __DIR__ . '/other.php';   // different loaded file -> redeclares otherfn()
    }
}, true, true);
build this case
2.2.8 phar (no guard at all) Cannot redeclare function
2.2.9 phar (the over-broad guard) declined, 1 error, no fatal
this branch Cannot redeclare function

So 2.2.9's protection here was collateral - a side effect of declining on the name alone, which is the same over-breadth that broke every facade alias. This branch restores 2.2.8 behaviour for that contrived shape while fixing the reported one.

I could not find a rule that covers both cheaply. Anything keyed on "the located file is already included" re-breaks the alias case, because the alias target's file is already included - that is precisely why the guard fired. Parsing the located file for symbols that already exist fails the same way: the target's class exists, so it would decline again. The only signal that separates the two is whether the file declares the function with this exact name, which is what ReflectionFunction::getFileName() answers.

If you would rather keep the broader decline and accept aliases staying broken until a better signal exists, say so and I will close this - but the Laravel and TYPO3 reports are real users, and the case this gives up needs an autoloader that re-includes an unrelated loaded file, which is what require_once exists to prevent.

@ondrejmirtes

Copy link
Copy Markdown
Member

Compare this with #6287

What's better?

@SanderMuller

Copy link
Copy Markdown
Contributor Author

Yours is better, on three counts I could measure - closing this one.

I ran every case I had against the merged tip (598faaade), each with its own empty tmpDir:

case #6288 (this PR) #6287 (merged)
@hoetaek's repro - alias to an unloaded target No errors No errors
e2e/bug-14988 - the redeclare fatal No errors No errors
autoloader re-including a different loaded file Cannot redeclare function declined, class.notFound, no fatal
bug-15102, bug-15102b, bug-15102c, bug-12972b/c pass (no 15102c) all pass
  1. It keeps the protection mine gave up. That third row is the case I red-teamed above: mine narrows the guard to the file declaring the same-named function, so an autoloader re-including an unrelated loaded file fatals again. Judging the statically parsed symbols of the located files decides the same question without that trade.

  2. It found the deeper bug. I missed that a trapped include still registers in get_included_files(), so my probe poisoned its own evidence and any earlier probe poisoned later lookups for the rest of the process. That explains failures my narrowing would not have fixed, and it is the reason "include-list bookkeeping cannot be made sound" - the premise my guard was built on.

  3. It executes less. Locating the class in the files the autoloaders asked for resolves plain include-based autoloaders with no execution at all; mine ran them for real on every collision.

One thing worth knowing about the fixtures: bug-15102b (mine) require_once's its alias target, so it only exercises "the autoloader defines the class without reading a file" - still a valid path, but it cannot fail on the unloaded-target shape. bug-15102c is the one that covers it, so coverage is complete now; I had a fixture fix in this PR for that and it is redundant.

Thanks for taking it further rather than patching my premise - and @hoetaek's diagnosis stands, it was the trapped read of the alias target all along, just with a deeper cause than either of us attributed it to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants