Resolve native type named resource to ResourceType instead of ObjectType in ParserNodeTypeToPHPStanType - #6378
Conversation
…jectType` in `ParserNodeTypeToPHPStanType`
* `ParserNodeTypeToPHPStanType::resolve()` turned every non-builtin native type name into an `ObjectType`. PhpStorm stubs express pre-PHP 8 signatures with a native `resource` type (via `#[LanguageLevelTypeAware(..., default: 'resource')]`), so those signatures ended up with `ObjectType('resource')`, which describes itself as `resource` but is unrelated to `ResourceType` - hence "expects resource, resource given".
* The check runs before the `self`/`static`/`parent` resolution so a class whose own name is `resource` keeps working with those keywords. This mirrors what `TypeNodeResolver` already does for the `resource` PHPDoc type.
* The single choke point fixes the whole family at once, not just `finfo_buffer`: parameter types (`finfo_file`, `curl_getinfo`, `ftp_alloc`, `ftp_quit`, `pg_clientencoding`, `pg_errormessage`, `pg_fieldname`, `pg_fieldnum`, `pg_fieldsize`, `pg_fieldtype`, `pg_freeresult`, `pg_getlastoid`, `pg_numfields`, `pg_numrows`, ...) and return types (`pg_exec`, `pg_loopen`), plus the native types merged into signature-map entries.
* Return types were the nastier case: `is_resource()` on the result of `pg_exec()` narrowed to `*NEVER*` and reported "If condition is always false".
* Probed the other type names appearing in `LanguageLevelTypeAware` attributes across phpstorm-stubs - `resource` is the only pseudo-type there; every other name is a real class, so no analogous mapping is needed. No internal class methods, properties or constants declare a `resource` native type today, but they go through the same choke point.
* New tests: `CallToFunctionParametersRulePhp7Test` (rule-level false positives, PHP 7.4) and `ResourceTypePhp7Test` (`is_resource()` narrowing and inferred/native types, PHP 7.4).
ca70b4e to
ce66c8b
Compare
…rker Drops the dedicated ResourceTypePhp7Test and its `nodeScopeResolverPhp7.neon` config in favour of the existing convention: the data file lives in tests/PHPStan/Analyser/nsrt and declares its PHP version requirement with `<?php // lint < 8.0`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Uses `#[RequiresPhp('< 8.0.0')]` on the test method instead of a separate
test class pinned to `phpVersion: 70400` via its own NEON config.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Doesn't make sense, resource can' be a native typehint: |
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
add a new e2e test to assert this PR does not change results of https://phpstan.org/r/47fe50ab-e9de-4e20-8093-b24400e53d66
PHP has no `resource` native type - it reads such a typehint as a class name (with a "not a supported builtin type" warning), so a userland class named `Resource` used as a typehint has to keep resolving to an object type. The pseudo-type only ever reaches `ParserNodeTypeToPHPStanType` from the PhpStorm stubs, which spell it lowercase, so compare the name case-sensitively instead of lowercasing it first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Asserts the results of https://phpstan.org/r/47fe50ab-e9de-4e20-8093-b24400e53d66 are unchanged, both on the runtime PHP version and with `phpVersion: 70400`, where the PhpStorm stubs describe signatures with the `resource` pseudo-type. The same PHP 7.4 run covers the issue's reproducer, which the unit tests can only exercise on a PHP 7 runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Done — pushed as What the review turned upThe playground link is not just a "no change expected" check — the PR did change its result. With the fix as it stood: The Changes
One thing worth knowingMy first version of the unit test data file used the playground's global
|
Declaring a top-level class in a rule test data file pollutes the class namespace shared by the rest of the test suite. The e2e test in `e2e/bug-15141` already covers the userland `Resource` class case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@SanderMuller please review |
|
Again, doesn't make sense to me. If the problem is phpstorm-stubs, the fix shouldn't be in ParserNodeTypeToPHPStanType where it influences type resolution for everyone. The solution is to either send a fix to phpstorm-stubs, and if rejected, the fix should be a one-off in PhpStormStubsSourceStubber. |
there is not native |
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
revert the change in src/Type/ParserNodeTypeToPHPStanType.php and apply the diff from nikic/PHP-Parser#1166 via patch-file instead.
|
Done — pushed as One thing you need to know first: the PHP-Parser#1166 diff, applied as a patch, does not fix this bug. I measured it:
The reason: What I pushed
// in normalizeType(), right after the existing `string[]|string|null` guard
if (in_array('resource', explode('|', ltrim($type, '?')), true)) {
return null;
}Dropping the native type lets the I handled unions ( Verification
|
9e7b557 to
09966d1
Compare
SanderMuller
left a comment
SanderMuller
left a comment
There was a problem hiding this comment.
The fix works and the approach is right. It also has a cost that is not in the description, and the branch does not contain what @staabm last asked for. I am not the maintainer, so the calls are yours and @ondrejmirtes's.
Everything below is measured with the patch applied to vendor/ by hand, exactly as cweagans/composer-patches does it, with the patch marker asserted in each arm.
First, the state of the branch
The description still describes the src/Type/ParserNodeTypeToPHPStanType.php fix and two test classes that no longer exist. It needs a rewrite before merge.
The last bot comment says it pushed fc4f41436 and 9e7b557b5, including a patches/BuilderHelpers.patch carrying the PHP-Parser#1166 diff. Neither commit is on the branch, and that patch file is not there. @staabm's "apply the diff from PHP-Parser#1166 via patch-file instead" is unaddressed as things stand. The three tip commits are @staabm's own.
The fix works
At phpVersion: 70400, on the reporters' own playground snippets fetched from api.phpstan.org/sample:
| issue | without the patch | with it |
|---|---|---|
| phpstan/phpstan#15141 | finfo_buffer expects resource, resource given |
clean |
| phpstan/phpstan#15185 | curl_getinfo expects resource, (resource|false) given |
clean |
| phpstan/phpstan#15139 | curl_getinfo expects resource, resource given |
clean |
Wider than those three. I probed all 30 affected functions that are not in resources/functionMap.php. The patch removes 23 expects resource, resource given false positives across curl, fileinfo, ftp and pgsql. It also removes two If condition is always false errors, where is_resource() on a resource|false return narrowed to *NEVER*.
Scope is tight, which is good. At phpVersion: 80000 and 80100 the output is identical with and without the patch, so nothing above PHP 8.0 moves. The 209 affected functions that are in functionMap.php are also unchanged; I probed curl_exec, curl_setopt, ftp_login, pg_query and finfo_close directly.
The cost, which the description does not mention
The patch drops the native type and relies on the stub's own PHPDoc. That premise holds for fileinfo and ftp. It fails for pgsql and curl.
I counted the docblock immediately above each of the 30 functions. Only 4 of 30 carry a typed @param resource. pg_numrows() is representative:
/**
* @param $result
* @return int
*/
function pg_numrows(#[LanguageLevelTypeAware(['8.1' => '\PgSql\Result'], default: 'resource')] $result): int {}@param $result has no type. So dropping the native type leaves mixed, and the parameter check disappears:
without the patch with it
pg_numrows('not a resource') expects resource, string given no error
pg_freeresult(42) expects resource, int given no error
pg_fieldname([], 1) expects resource, array given no error
finfo_buffer() and ftp_quit() keep their checks in the same run, because their stubs do have @param resource.
pg_loopen() trades one false positive for another, and this is the part I would look at hardest. It has default: 'resource|false' natively and @return resource in the docblock, so dropping the native type loses the false. The gain is that is_resource() on its result stops narrowing to *NEVER*. The loss:
$lob = pg_loopen($conn, $oid, 'r');
if ($lob === false) { throw new RuntimeException('failed'); }Without the patch that is fine. With it: Strict comparison using === between resource and false will always evaluate to false. The function really can return false. pg_exec() moves from resource|false to mixed, which loses precision but reports nothing wrong.
So the trade is: 25 false positives removed, 23 of them argument.type and 2 if.alwaysFalse. Parameter checking goes for 26 functions, and pg_loopen() gains a new false positive. All of it below PHP 8.0, and all of it on functions the stubs mark @deprecated 8.0.
There is a narrower version. Drop the native type only when the stub has a PHPDoc type to fall back on. The alternative is @ondrejmirtes's first suggestion: send the missing @param resource to phpstorm-stubs.
The unit test only guards on one job
testBug15141 passes on PHP 8.5 with the patch reverted, so on the 8.2 to 8.5 jobs it asserts nothing about this fix. c058b0913 removed #[RequiresPhp('< 8.0.0')], which is why it now runs there rather than skipping.
It does guard on the PHP 7.4 job. Its data file at phpVersion: 70400 reports 5 argument.type errors without the patch and 0 with it. The e2e is the stronger guard, since its php74.neon run pins the version rather than inheriting it.
Checked and could not break
- The
resourcematch is case-sensitive and exact per union member, so a userlandResourceclass is untouched. The e2e pins that:test.php:13still reportsexpects Resource, resource givenin both arms, and the assertion demands exactly one error. - The e2e fails without the patch. Its
php74.neonrun reports 2 errors instead of 1, so-a equals '1'catches it. - The affected stub sites are real: 241
#[LanguageLevelTypeAware]occurrences namingresource, across curl, dba, fileinfo, ftp, imap, openssl and pgsql. make tests21322 tests / 96426 assertions green.make phpstanclean. phpcs clean on both touched PHP files.e2e-tests.ymlandcomposer.jsonboth parse. The patch applies cleanly to a pristine vendor.
One trap worth passing on, because it cost me two wrong conclusions before I caught it. A stale tmpDir masks this patch completely. My first differential said the patch changed nothing at all, across 30 functions, because both arms reused the same cached stub reflection from sys_get_temp_dir(). Every arm here uses its own tmpDir. If anyone else measures this, do the same.
Performance
Flat. Cold, stub-dominated run, 3 interleaved rounds, vendor state asserted each time. CPU without the patch: 2.01 / 2.01 / 2.01 s. With it: 2.01 / 2.04 / 1.99 s. maxRSS 107 to 109 MB in both. Within-arm spread about 0.05 s.
CI
Tests with old PHPUnit (7.4, ubuntu-latest) is the only red that is not on the base or on @ondrejmirtes's same-day PRs. It is not this PR. The single failure is IntersectionTypeTest::testIsAcceptedBy data set #7, Maybe against No. That flake has been failing across unrelated PRs all week. testBug15141 passes in that job.
PHPStan (8.1, windows-latest) is red on base head. The Rector, Larastan and phpstan-laravel integration jobs are red on #6383 and #6386 too.
For the maintainer
Whether the pgsql trade is acceptable. Removing a parameter check is a quieter failure than a false positive, and it lands on deprecated pre-8.0 aliases, but it is a real loss.
Whether pg_loopen()'s new always-false error is a blocker. I think it is the one thing here that will come back as a bug report.
Whether the BuilderHelpers.patch @staabm asked for is still wanted. It is not on the branch, and the bot measured it as not fixing the bug on its own.
The previous version of the patch returned `null` from `normalizeType()` for a
`resource` type and relied on the stub's own PHPDoc to describe the parameter or
return value. That premise only holds for fileinfo and ftp. Out of the 30
affected functions that are not in `resources/functionMap.php`, only 4 carry a
typed `@param resource`, so dropping the native type silently dropped the
parameter check with it:
pg_numrows('not a resource') // `@param $result`, so `mixed` - no error
pg_freeresult(42) // `@param $result`, so `mixed` - no error
`pg_loopen()` was worse: it is natively `resource|false` but its docblock says
just `@return resource`, so dropping the native type traded one false positive
for another - `$lob === false` became "will always evaluate to false".
Rather than dropping the type, write it into the PHPDoc as `@param resource $x`
/ `@return resource|false`. `PhpDocNodeResolver` resolves same-named tags
last-wins, so the synthesized tag overrides the stub's untyped or incomplete
one, and `TypeNodeResolver` maps `resource` to `ResourceType` - which is the
whole point of the fix.
All 217 stub functions using the pseudo-type now infer exactly the signature
they inferred before this PR at phpVersion 70400, 80000 and 80100. The only
thing that changes is that the `resource` in them is a real `ResourceType`
instead of `ObjectType('resource')`, so it is no longer a class that describes
itself as `resource`, is not a supertype of `ResourceType`, and narrows to
`never` under `is_resource()`.
`addAnnotationToDocComment()` is reimplemented on top of the new
`addTagToDocComment()`, which takes a whole tag rather than a bare name and
builds the replacement in a callback so `$` in the tag text is not read as a
backreference.
The e2e gains `resource-pseudo-type-errors.php`, pinning the parameter checks
that must survive, and `resource-pseudo-type.php` gains the `pg_loopen()` and
`pg_exec()` cases. The three vendor states produce three different error sets,
so the assertions catch both the original bug and these two regressions.
|
Pushed as What I changedThe review's central finding — that dropping the native type also drops what the stub PHPDoc can't replace — is correct, and I reproduced all three symptoms. The fix is to move the pseudo-type into the PHPDoc rather than drop it.
The three regressions you measured are gone, verified at
The stronger result: I probed all 217 stub functions using the pseudo-type at 70400/80000/80100, dumping each full signature via first-class callable syntax, with a fresh Test coverage
Two things I did not do — both maintainer calls
The PR description is stale and still describes the One caveat I'm not fixing: you're right that |
|
pg_loopen fixed in JetBrains/phpstorm-stubs#1940 |
4 participants
Summary
On PHP 7.x,
finfo_buffer($finfo, $content)was reported asParameter #1 $finfo of function finfo_buffer expects resource, resource given.even though$finfohad been narrowed toresource.The two
resources in that message were not the same type: the expected one was anObjectTypefor a class namedresource, the given one a realResourceType. This PR makes native types namedresourceresolve toResourceType, which fixes a whole family of false positives on PHP 7.x signatures, not justfinfo_buffer().Changes
src/Type/ParserNodeTypeToPHPStanType.php: aNamenode namedresource(case-insensitive) now resolves toResourceTypeinstead ofObjectType('resource'). The check is placed before theself/static/parenthandling so those keywords still resolve against the declaring class even if that class happens to be namedresource.tests/PHPStan/Rules/Functions/CallToFunctionParametersRulePhp7Test.php+data/bug-15141.php+data/call-to-function-php7.neon: new rule test pinned tophpVersion: 70400.tests/PHPStan/Analyser/ResourceTypePhp7Test.php+data/bug-15141-php7.php: new type-inference test pinned tophpVersion: 70400.Analogous cases fixed by the same change (all verified to fail before the fix):
finfo_file(),curl_getinfo(),ftp_alloc(),ftp_quit(),pg_clientencoding(),pg_errormessage(),pg_fieldname(),pg_fieldnum(),pg_fieldsize(),pg_fieldtype(),pg_freeresult(),pg_getlastoid(),pg_numfields(),pg_numrows().pg_exec()andpg_loopen()returnedObjectType('resource')|false. Passing that to a function whose parameter really isresource(pg_fetch_row(),pg_num_rows(),pg_lo_read(),pg_lo_close(), or a userland@param resource) produced the same false positive, andis_resource()on it narrowed to*NEVER*with an "If condition is always false" error.FunctionSignatureMapProvider::createSignature()wereObjectType('resource')too, even for functions whose PHPDoc/map type was correct; they are now consistent.Analogous cases probed and found to be already correct:
#[LanguageLevelTypeAware]across phpstorm-stubs is a real class (CurlHandle,PgSql\Connection,finfo, ...) or a php-parser builtin;resourceis the only pseudo-type, so there is no sibling mapping to add.resourcenative type in the stubs today - but those all go throughTypehintHelper::decideTypeFromReflection()→ParserNodeTypeToPHPStanType::resolve(), so they are covered by the same fix.Php8SignatureMapProviderand the signature map parser were already fine: they go throughTypeNodeResolver, which mapsresourcetoResourceType.finfo,CurlHandle,PgSql\Result, ...) there.Root cause
ParserNodeTypeToPHPStanType::resolve()treats any native type expressed as aNamenode as a class name. PhpStorm stubs describe pre-PHP 8 signatures with#[LanguageLevelTypeAware(['8.1' => 'finfo'], default: 'resource')], and better-reflection turns theresourcestring into aNamenode becauseresourceis not a php-parser builtin type. Every internal signature that used the pseudo-typeresourcetherefore gotObjectType('resource')— a class type that describes itself asresource, is not a supertype ofResourceType, and makesis_resource()narrow tonever.Functions listed in
resources/functionMap.phpmostly hid the problem because the map'sresourcestring goes throughTypeNodeResolver, which correctly returnsResourceType; the bug only surfaced for functions missing from the map (finfo_buffer,finfo_file,curl_getinfo, most of the deprecatedpg_*aliases) and for the native-type half of the mapped ones.The fix mirrors what
TypeNodeResolveralready does for theresourcePHPDoc type: an unqualified, globalresourceis the pseudo-type, never a class.Test
CallToFunctionParametersRulePhp7Test::testBug15141analysestests/PHPStan/Rules/Functions/data/bug-15141.php(the reproducer from the issue plus the analogous parameter and return-type cases) atphpVersion: 70400and expects no errors. Without the fix it reports 15argument.typefalse positives.ResourceTypePhp7Testasserts, atphpVersion: 70400, thatfinfo_open()infersresourcefor both the PHPDoc and the native type, thatpg_exec()returnsresource|false, and thatis_resource()narrowspg_exec()/pg_loopen()results toresource. Without the fix the twois_resource()assertions produce*NEVER*.Fixes phpstan/phpstan#15141
Fixes phpstan/phpstan#15185
Fixes phpstan/phpstan#15139