Process reordered named arguments in call-site order in processArgs#5725
Conversation
- `ArgumentsNormalizer` reorders named arguments into parameter-declaration order for type checking. `processArgs` then processed them in this reordered order, causing variable assignments in earlier call-site arguments to be invisible to later ones. - Sort the processing order in `processArgs` by the original argument's source position (via `ORIGINAL_ARG_ATTRIBUTE`) so scope mutations (variable assignments) happen in call-site evaluation order. - Parameter matching still uses the reordered index, so type checking is unaffected. - Affects all call types: `new`, function calls, method calls, and static method calls.
|
This needs more thought I think, the issue is really crazy and breaks a lot of assumptions. |
|
Oh sorry, never mind, I thought this was about phpstan/phpstan#14661 |
So this PR is ok to you @ondrejmirtes ? |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Merge the nsrt and rule test fixture file to use only one file
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
|
Yes, this is alright. |
| $processingOrder = array_keys($args); | ||
| $hasReorderedArgs = false; | ||
| foreach ($args as $arg) { | ||
| if ($arg->getAttribute(ArgumentsNormalizer::ORIGINAL_ARG_ATTRIBUTE) !== null) { |
There was a problem hiding this comment.
use $arg->hasAttribute instead
There was a problem hiding this comment.
Done. Changed $arg->getAttribute(ArgumentsNormalizer::ORIGINAL_ARG_ATTRIBUTE) !== null to $arg->hasAttribute(ArgumentsNormalizer::ORIGINAL_ARG_ATTRIBUTE) as requested by staabm. Tests and static analysis both pass, commit pushed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 participants
Summary
When named arguments are passed in a different order than the parameter declaration,
ArgumentsNormalizerreorders them to match declaration order for type checking. However,processArgsthen processed arguments in this reordered order, so variable assignments in earlier call-site arguments were not visible in later ones. This caused false "Variable might not be defined" errors.Changes
NodeScopeResolver::processArgs()insrc/Analyser/NodeScopeResolver.phpto sort the argument processing order by the original argument's source position (using theORIGINAL_ARG_ATTRIBUTEstored byArgumentsNormalizer). Default-value fillers (without original args) are processed last since they have no side effects.$i) still refers to the reordered position, so parameter type checking,$closureBindScopehandling, and by-reference detection remain correct.Root cause
ArgumentsNormalizer::reorderArgs()reorders named arguments from call-site order to parameter-declaration order and stores the originalArgon each reordered arg viaORIGINAL_ARG_ATTRIBUTE.processArgs()then iterated over these reordered args sequentially, meaning scope mutations (variable assignments) happened in declaration order instead of evaluation order. When a later-declared parameter's argument referenced a variable assigned in an earlier call-site argument, the variable was not yet in scope.All four call types (
new, function call, method call, static method call) were affected equally because they all normalize arguments before passing them toprocessArgs.Test
tests/PHPStan/Rules/Variables/data/bug-9392.phpcovering the original reproducer (constructor with reordered named args) plus analogous cases for function calls, method calls, static method calls, and mixed positional/named arguments.tests/PHPStan/Analyser/nsrt/bug-9392.phpto verify the inferred types of variables assigned in named arguments.Fixes phpstan/phpstan#9392