Process reordered named arguments in call-site order in processArgs#5725
Open
phpstan-bot wants to merge 1 commit into
Open
Process reordered named arguments in call-site order in processArgs#5725phpstan-bot wants to merge 1 commit into
processArgs#5725phpstan-bot wants to merge 1 commit into
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.
Member
|
This needs more thought I think, the issue is really crazy and breaks a lot of assumptions. |
Member
|
Oh sorry, never mind, I thought this was about phpstan/phpstan#14661 |
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
3 participants
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
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