Resolve resource native type declarations in internal stubs as ResourceType - #6379
Closed
phpstan-bot wants to merge 1 commit into
Closed
Resolve resource native type declarations in internal stubs as ResourceType#6379phpstan-bot wants to merge 1 commit into
resource native type declarations in internal stubs as ResourceType#6379phpstan-bot wants to merge 1 commit into
Conversation
…ourceType`
- `ParserNodeTypeToPHPStanType::resolve()` and `TypehintHelper::decideTypeFromReflection()` take a new optional `$isBuiltin` flag. When it is set, a `Name` type node spelled `resource` resolves to `ResourceType` instead of `ObjectType('resource')`. The flag is propagated through nullable, union and intersection type nodes.
- The flag is passed from every reflection path that can read a type declaration of a PHP-internal (stubbed) symbol: `PhpFunctionReflection` (parameters + return), `PhpMethodReflection` (parameters, return, prototype tentative return), `PhpParameterReflection` (new constructor argument), `PhpClassReflectionExtension` (property native type, method native return), `ClassReflection` (enum backing type, class constant native types), `InitializerExprTypeResolver`, `NativeMethodReflection`, `FunctionSignatureMapProvider` and `Php8SignatureMapProvider`.
- User-written `resource` type declarations keep resolving to `ObjectType`, so PHPStan still reports `class.notFound` for them - that matches what PHP itself does with such a declaration.
- Swept the whole family: a scan of every function, method and property signature coming from phpstorm-stubs on PHP 7.2/7.4/8.0/8.4 shows no `ObjectType('resource')` left. Besides curl, the same bug affected fileinfo, xml, pgsql, ftp, imap and openssl signatures; only functions were affected in practice, but methods, properties, class constants and enum backing types are plumbed too. The same scan for other non-existent class names in internal signatures only turns up upstream phpstorm-stubs typos (`java_*`, `zend_*`, `Socket`/`GdImage` on PHP 7.4), which are not PHPStan bugs.
Contributor
|
Do we have similar problems with other type added in newer php versions? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On PHP 7.x,
curl_getinfo($handle)was reported asParameter #1 $handle of function curl_getinfo expects resource, (resource|false) given.— and even a definitely-narrowed resource produced the nonsensicalexpects resource, resource given.The two
resources in that message were not the same type: the expected one wasObjectType('resource')(a class namedresource), the given one was PHPStan'sResourceTypepseudo-type.Changes
src/Type/ParserNodeTypeToPHPStanType.php:resolve()gained an optionalbool $isBuiltinparameter. When it istrue, aNametype node whose lowercased name isresourceresolves toResourceType. The flag is threaded through the nullable, union and intersection branches.src/Type/TypehintHelper.php:decideTypeFromReflection()gained the same optionalbool $isBuiltinparameter and forwards it (also through its union/intersection recursion) toParserNodeTypeToPHPStanType::resolve().truefrom every reflection path that can read a type declaration belonging to a PHP-internal symbol:src/Reflection/Php/PhpFunctionReflection.php— parameters and (native) return typesrc/Reflection/Php/PhpParameterReflection.php— new constructor argument, used for bothgetType()andgetNativeType()src/Reflection/Php/PhpMethodReflection.php— parameters, return type, native return type and the prototype's tentative return typesrc/Reflection/Native/NativeMethodReflection.php— prototype tentative return typesrc/Reflection/Php/PhpClassReflectionExtension.php— property native type and method native return typesrc/Reflection/ClassReflection.php— enum backing type and class-constant native typessrc/Reflection/InitializerExprTypeResolver.php— class-constant native typesrc/Reflection/SignatureMap/FunctionSignatureMapProvider.phpandsrc/Reflection/SignatureMap/Php8SignatureMapProvider.php— always builtinfunction f(resource $r)still resolves toObjectType('resource')and still reportsclass.notFound, which is correct — PHP warns"resource" is not a supported builtin type and will be interpreted as a class nameand throws aTypeErrorwhen an actual resource is passed.Analogous cases
resources/functionMap.phpwere affected across fileinfo (finfo_file,finfo_buffer), xml (xml_parse_into_struct), pgsql (pg_exec,pg_numrows,pg_loopen,pg_loread, …), ftp (ftp_alloc,ftp_quit), imap and openssl. All are fixed by the same change.resourceon them, so the family cannot drift apart later.ObjectTypes referring to non-existent classes in all internal signatures on PHP 7.2/7.4/8.0/8.4 turns up only upstream phpstorm-stubs typos where a parameter name was written in the type position (java_reload,zend_get_id,register_event_handler, …) plusSocket/GdImage/OCI-Lobmisdeclarations.resourcewas the only real pseudo-type leak.CurlHandleetc., so nothing changed there; the same scan reports zero hits.Root cause
resourcehas no native PHP type declaration, so phpstorm-stubs express it with#[LanguageLevelTypeAware(['8.0' => 'CurlHandle'], default: 'resource')].BuilderHelpers::normalizeType('resource')is not in php-parser's builtin-type list, so it produces aPhpParser\Node\Name— a class name — andParserNodeTypeToPHPStanType::resolve()faithfully turned that intoObjectType('resource').Nothing is a subtype of a class that does not exist, so every argument of a genuine
resourcetype was rejected, and the error message was unreadable becauseObjectType('resource')andResourceTypeboth describe themselves asresource.Most of these signatures used to be shadowed by
resources/functionMap.php, which resolves types throughTypeNodeResolverwhereresourcecorrectly maps toResourceType. Commit 6a59780 ("Clean out redundant stub overwrites") dropped entries such ascurl_getinfo, which exposed the underlying resolution bug.The fix makes the resolution context-aware instead of adding another data patch: inside a PHP-internal symbol,
resourcemeans the pseudo-type; in user code it keeps meaning a class name, exactly as PHP reads it.Test
tests/PHPStan/Rules/Functions/CallToFunctionParametersPhp7RuleTest.php+tests/PHPStan/Rules/Functions/data/bug-15185.php— runsCallToFunctionParametersRulewithphpVersion: 70400. It contains the reporter's playground snippet plus narrowed-resource, fileinfo, xml and pgsql cases. Without the fix it reports 7 errors (expects resource, (resource|false) givenandexpects resource, resource givenacross curl, xml, fileinfo and pgsql); with the fix it reports none.tests/PHPStan/Analyser/Bug15185Test.php+tests/PHPStan/Analyser/data/bug-15185.php— aphpVersion: 70400type-inference test covering the return-type half of the bug.assertType('resource', $result)afteris_resource(pg_exec(...))yields*NEVER*without the fix, becauseObjectType('resource')andResourceTypeare disjoint.Fixes phpstan/phpstan#15185