Detect polyfills by their conditional declaration instead of a polyfill substring in the file path - #6231
Conversation
…ill` substring in the file path - `NativeFunctionReflectionProvider::findFunctionReflection()` no longer drops the native signature just because a userland file declares the function; it keeps it when the declaration is conditional (the polyfill idiom) and phpstorm-stubs says PHP provides the function at the configured PHP version. - New `ConditionallyDeclaredSymbolDetector` parses the declaring file and reports which functions, classes and `define()`d constants are declared inside a conditional block (`function_exists()`, `class_exists()`, `defined()`, `PHP_VERSION_ID` comparisons, ...). - `SkipPolyfillSourceLocator` applies the same rule to the analogous cases: conditionally declared classes/interfaces/traits/enums and `define()`d constants that PHP provides natively are hidden so the internal stub wins, next to the existing symfony/polyfill path list. - Functions are deliberately not hidden at the source-locator level - that would also hide their existence on PHP versions without them natively. - An unconditional userland declaration of a native-sounding name (the `swf_actiongotoframe` case from #13556) keeps winning over the signature map.
staabm
left a comment
There was a problem hiding this comment.
which recent change introduced the regression?
|
the declarations are wrong, see phpstan/phpstan#15086 (comment) |
|
No code change was needed for this review — it's a question. I investigated and here's the answer, with evidence. Answer: no phpstan-src commit did — mockery 1.6.13 didThe trigger is external. nextras/orm requires
The last green run and the first red one bracket the mockery release, and no commit landed on The latent phpstan-src change it exposedThe behaviour it woke up came from 326c6ec "Improve function signatures with functionMap only for built-in symbols" (2025-09-22, first released in 2.1.29, fixing phpstan/phpstan#13556), which added to if (!$reflectionFunctionAdapter->isInternal() && !str_contains(strtolower($fileName), 'polyfill')) {
return null;
}Returning I verified this empirically rather than by reading alone, using the PR's e2e fixture at the pre-fix commit
So: |
Summary
str_contains(),str_starts_with()andstr_ends_with()started being reported asexpecting
non-empty-stringin the nextras/orm CI job:The culprit is
mockery/mockery'slibrary/helpers.php, which is a Composerautoload.filesentry containingThat declaration is dead code on PHP 8 - PHP provides
str_contains()natively - butPHPStan reflected it anyway and let its (wrong) PHPDoc replace the native signature.
PHPStan only recognised polyfills by looking for the substring
polyfillin thedeclaring file's path, which
library/helpers.phpdoes not contain.The fix recognises polyfills by what actually makes them polyfills: the declaration is
guarded by a conditional, so it can never run when PHP provides the symbol natively.
Changes
src/Reflection/ConditionallyDeclaredSymbolDetector.php(new): parses a file with@php8Parserand reports which functions, classes/interfaces/traits/enums anddefine()d constants are declared inside a conditional block. Results are cached perfile (
FILE_CACHE_LIMIT); an unreadable or unparseable file simply yields nothing.src/Reflection/SignatureMap/NativeFunctionReflectionProvider.php: replacedstr_contains(strtolower($fileName), 'polyfill')withisPolyfill(), which requiresboth that the declaration is conditional and that
PhpStormStubsSourceStubber::isPresentFunction()confirms PHP provides the function atthe configured PHP version.
src/Reflection/BetterReflection/SourceLocator/SkipPolyfillSourceLocator.php: thesame rule now also hides conditionally declared classes/interfaces/traits/enums
(gated by
isPresentClass()) anddefine()d constants (gated bygenerateConstantStub()), so the internal stub is reflected instead of the polyfill'sapproximation. The existing
symfony/polyfill-php8xpath list is kept - it also coverssymbols those packages declare unconditionally.
src/Reflection/BetterReflection/BetterReflectionSourceLocatorFactory.phpandsrc/Testing/TestCaseSourceLocatorFactory.php: pass the new detector and the stubberinto
SkipPolyfillSourceLocator.Analogous cases
str_contains/str_starts_with/str_ends_with, positional and named arguments -all covered by the same fix and exercised by the e2e fixture.
class_exists()/interface_exists()/PHP_VERSION_IDguard - probed, found broken (a polyfilledValueError/Stringableshadowed the native one), fixed inSkipPolyfillSourceLocator.define()- probed, found broken(
JSON_THROW_ON_ERRORresolved to the polyfill's value), fixed in the same place.constinside a conditional - not possible in PHP (parse error), so no handling isneeded for that form.
(
swf_actiongotoframe, False positive parameter count error for static method named "add" phpstan#13556) - probed, still correctly resolved to theuser's own signature, since
isPolyfill()requires the conditional guard.would also hide their existence when the analysed PHP version does not have them
natively (and would make
getallheaders()unknown); preferring the native signature inNativeFunctionReflectionProviderachieves the fix without that side effect.Root cause
NativeFunctionReflectionProvider::findFunctionReflection()returnednull- handing thefunction over to userland reflection - as soon as the function had a declaring file that
was not internal, unless the path contained the literal substring
polyfill. That pathheuristic was introduced to stop the signature map from being applied to a user's own
function that happens to share a name with an ancient PHP function
(phpstan/phpstan#13556), but it cannot tell those two situations apart:
function swf_actiongotoframe($string) {}- declared unconditionally,because nothing else declares it, and
if (!function_exists('str_contains')) { function str_contains(...) {} }-necessarily declared conditionally, because PHP would fatal on the redeclaration.
The conditional guard is the discriminator, and it applies verbatim to the other symbol
kinds:
class_exists()guards for classes anddefined()guards for constants.SkipPolyfillSourceLocatorhad exactly the same path-based blind spot for those.Test
e2e/shadowed-native-function/- amockery-shapedhelpers.phpwith all threeguarded
str_*polyfills carrying@param non-empty-string, registered in.github/workflows/e2e-tests.yml. Without the fix it reports the threeargument.typeerrors from the issue (including the named-argument call); with the fixit reports none.
tests/PHPStan/Analyser/nsrt/shadowed-native-function.php-getallheaders()fromralouphie/getallheaders(already in PHPStan's own vendor, guarded byfunction_exists()and carrying an invalid@return string[string]). Inferred as*ERROR*before the fix,arrayafter.tests/PHPStan/Reflection/BetterReflection/SourceLocator/SkipPolyfillSourceLocatorTest.php-polyfilled
ValueError,StringableandJSON_THROW_ON_ERRORare skipped, while aconditionally declared non-native class, an unconditional class and a non-native
constant are kept.
tests/PHPStan/Reflection/ConditionallyDeclaredSymbolDetectorTest.php- unit coveragefor the detector:
function_exists()andPHP_VERSION_IDguards,elseif/elsebranches, namespaced and global declarations, case-insensitivity for functions and
classes vs. case-sensitivity for constants, unconditional declarations and unreadable
files.
make tests,make phpstanandmake csare green.make name-collisionfails on thismachine for a pre-existing, unrelated reason:
tests/PHPStan/Reflection/data/attribute-const-reflection.phpand
tests/PHPStan/Analyser/nsrt/pipe-operator.phpneed PHP 8.5 to parse and the runnerhas PHP 8.4.
Fixes phpstan/phpstan#15086