Hotfix: fn arrow closure - Squiz.Scope.StaticThisUsage #2725
Conversation
That's the point of this sniff. Because it's a runtime error, the sniff tries to determine that the code is invalid during linting. So I think this change is a good one. |
| if ($next === false) { | ||
| continue; | ||
| } else if ($tokens[$next]['code'] === T_CLOSURE | ||
| || $tokens[$next]['code'] === T_FN | ||
| || $tokens[$next]['code'] === T_ANON_CLASS | ||
| ) { | ||
| $next = $tokens[$next]['scope_closer']; |
There was a problem hiding this comment.
T_FN tokens don't always have a scope closer, unlike closures and anon classes. This is because the T_FN token is a real token that exists, and then scope information is added later if the syntax looks valid. But T_CLOSURE and T_ANON_CLASS are custom tokens that only get applied by PHPCS if the syntax looks good. Otherwise they are normal T_FUNCTION and T_CLASS tokens that don't have scope information.
I think the error should be reported if method is static and class A {
public $name = 'NAME';
public static function m1() { // cannot be static as we are using $this in closure
return function () {
return $this->name;
};
}
public static function m2() { // cannot be static as we are using $this in closure
return fn () => $this->name;
}
}We are not getting any errors on that code, but if we try to call method and execute the result of the method - callback, then we get the error. Currently we are skipping that case for normal closure (T_CLOSURE), that's why I've added also the same for T_FN, but I believe we should report error in that case. I'll submit another commit soon and you'll decide what to do. |
| $next = $tokens[$next]['scope_closer']; | ||
| continue; | ||
| } else if (strtolower($tokens[$next]['content']) !== '$this') { | ||
| } else if ($tokens[$next]['content'] !== '$this') { |
There was a problem hiding this comment.
It was wrong, see:
https://3v4l.org/cvN2L
| } | ||
| public static function myFunc() { | ||
| $this->doSomething(); | ||
| } |
There was a problem hiding this comment.
changes tabs to spaces
| } | ||
|
|
||
| public static function anonClassUseThis() { | ||
| return new class($this) { |
There was a problem hiding this comment.
Currently, when we find anonymous class we are skipping till scope_closer, but we can use $this in params, so we really shouldn't skip it 'yet'.
The check should be more complicated here - please see also example in lines 98-110.
Errors should be reported only in line 84 and 99, not in 93 or 106.
I've implemented "similar" sniff here:
https://github.com/webimpress/coding-standard/blob/develop/src/WebimpressCodingStandard/Sniffs/PHP/StaticCallbackSniff.php
which checks if closure can be defined as static or not.
| @@ -31,9 +31,13 @@ public function getErrorList() | |||
| 9 => 1, | |||
| 14 => 1, | |||
| 20 => 1, | |||
| 41 => 1, | |||
There was a problem hiding this comment.
in my opinion it was invalid implementation before, so the error was not reported on that line - as noted in one of the previous comments - we shouldn't skip closures.
|
I'm going to remove this from the 3.5.4 milestone as it contains failing test cases that should be resolved before merging. |
Related squizlabs#2523 I am not sure about this change as this is actually invalid code. We cannot use `$this` even inside closure, as then closure is not gonna be usable. The same in case of normal closure. Maybe here we should have another fix then?
Recursion for anonymous classes as $this can be used in the constructor parameter. Fixes failing tests.
michalbundyra
force-pushed
the
hotfix/static-this-usage-array-closure
branch
from
692aeab to
10b6b2a
Compare
|
@gsherwood I've rebased and updated the code of detecting usage of Can it be added back to 3.5.4 release, please? Thanks! |
|
Thanks a lot for this. Sorry it's taken so long. |
Related #2523
I am not sure about this change as this is actually invalid code.
We cannot use
$thiseven inside closure, as then closure is not gonnabe usable. The same in case of normal closure.
Maybe here we should have another fix then?