Repository navigation
Conversation
The bind-scope factory captured the call as written and read $newThis and $newScope from getArgs()[1] and [2]. processArgs() already walks the normalized call, so the closure gets the factory in any argument order, but with named arguments the factory picked the wrong arguments: $this became mixed, object or a class-name string, and the class scope was lost, so accessing the scope class's private and protected members was reported. Normalize the call inside the factory as well. The reordered Args keep the same value expressions, so their stored results are still found. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ClosureBindArgVisitor marked the first argument of any Closure::bind() call with more than one argument as the bound closure. With named arguments the first argument can be $newThis - Closure::bind(newThis: $c, closure: ...) - and when it was a variable with a @param-closure-this type, ParametersAcceptorSelector narrowed the $newThis parameter to that type and reported the variable passed for it. Find the closure and $newThis arguments by position or by name, and mark the closure argument only when both are passed. A duplicated named argument does not replace the one already found, like in ArgumentsNormalizer::reorderArgs(). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…amed ParametersAcceptorSelector narrowed the $newThis parameter of Closure::bind() to a closure variable's @param-closure-this type only when the closure was the first argument. With named arguments the closure can be anywhere - Closure::bind(newThis: $o, closure: $c) - and ClosureBindArgVisitor marks it wherever it is, so look the marked argument up when the first one is named. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
This makes
Closure::bind()analysed the same however its arguments are written: positional, named in any order, or mixed. The bound$thisand class scope were only right when the closure,$newThisand$newScopesat at positions 0, 1 and 2. The@param-closure-thischeck only applied when the closure was at position 0 and a second argument was present. This PR is split out of #4081 so it can be reviewed and merged on its own; it contains nothing aboutself/parent/staticresolution.Changes
StaticCallHandler: the bind-scope factory normalizes the call before reading$newThisand$newScope.ClosureBindArgVisitor: finds the closure and$newThisarguments by position or by name, and marks the closure argument only when both are passed. A call without$newThis, such asClosure::bind($c, newScope: X::class), is no longer marked. A duplicated argument now resolves like inArgumentsNormalizer::reorderArgs(): the first named one wins.ParametersAcceptorSelector: when the first argument is named, looks up the argument the visitor marked instead of reading$args[0]. Only this rule-side path, which sees the call's original arguments, needed the lookup.Cause
The factory captured the call as written and read
getArgs()[1]and[2].processArgs()already walks the normalized call, so the closure got the factory in any order. With named arguments, though, the factory read the wrong arguments:$thisbecamemixed,objector a class-name string, and the scope was lost. Members of the scope class were then reported as inaccessible, while a closure bound to another scope could slip through unreported. Separately, the visitor marked the first argument of anyClosure::bind()call with more than one argument as the bound closure. InClosure::bind(newThis: $c, closure: ...), where$chas a@param-closure-thistype,$citself was then reported as the wrong type for$newThis. AndParametersAcceptorSelectoronly checked$args[0], so the override never applied when the closure was passed by name after another argument.When normalization fails (a missing
$newThis, or an unknown name before a skipped parameter), the factory still reads the call as written, as before; those calls are invalid, and the rules reportargument.missing/argument.unknownfor them.Tests
closure-bind-named-arguments.php:$this(PHPDoc and native) and a private property inside the closure, for every argument order.ClassConstantRuleTest,AccessPropertiesRuleTest,CallMethodsRuleTest: protected and private members of the scope class are accessible in every order. A closure bound to another scope, with the closure written before$newThis, still gets the access reported; the base missed it.CallStaticMethodsRuleTest: the@param-closure-thisoverride with named arguments, and no false positive when the closure-this variable is passed as$newThis.ParametersAcceptorSelector::selectFromArgs()over namedClosure::bind()calls.