Squiz/StaticThisUsage: examine the body of static closures

As noted in the previous commit, closures can be declared as `static`, in which case they do not have access to `$this`.

That case was so far not considered by this sniff.

This commit changes the sniff to:
1. Also examine `T_CLOSURE` tokens.
2. Examine these when nested inside OO methods, as well as when found completely outside of OO structures.

Note: in a future iteration it should be considered to get rid of the implementation via the `AbstractScopeSniff` as that implementation actually makes the sniff _more complicated_ instead of _less complicated_.

Includes unit tests.

Author: jrfnl

src/Standards/Squiz/Sniffs/Scope/StaticThisUsageSniff.php

@@ -23,7 +23,7 @@ class StaticThisUsageSniff extends AbstractScopeSniff
      */
     public function __construct()
     {
-        parent::__construct([T_CLASS, T_TRAIT, T_ENUM, T_ANON_CLASS], [T_FUNCTION]);
+        parent::__construct([T_CLASS, T_TRAIT, T_ENUM, T_ANON_CLASS], [T_FUNCTION, T_CLOSURE], true);
     }
 
 
@@ -42,22 +42,28 @@ public function processTokenWithinScope(File $phpcsFile, int $stackPtr, int $cur
         $tokens = $phpcsFile->getTokens();
 
         // Determine if this is a function which needs to be examined.
-        $conditions = $tokens[$stackPtr]['conditions'];
-        end($conditions);
-        $deepestScope = key($conditions);
-        if ($deepestScope !== $currScope) {
-            return;
-        }
+        if ($tokens[$stackPtr]['code'] === T_FUNCTION) {
+            $conditions = $tokens[$stackPtr]['conditions'];
+            end($conditions);
+            $deepestScope = key($conditions);
+            if ($deepestScope !== $currScope) {
+                return;
+            }
 
-        // Ignore abstract functions.
-        if (isset($tokens[$stackPtr]['scope_closer']) === false) {
-            return;
-        }
+            // Ignore abstract functions.
+            if (isset($tokens[$stackPtr]['scope_closer']) === false) {
+                return;
+            }
 
-        $next = $phpcsFile->findNext(Tokens::EMPTY_TOKENS, ($stackPtr + 1), null, true);
-        if ($next === false || $tokens[$next]['code'] !== T_STRING) {
-            // Not a function declaration, or incomplete.
-            return;
+            $next = $phpcsFile->findNext(Tokens::EMPTY_TOKENS, ($stackPtr + 1), null, true);
+            if ($next === false || $tokens[$next]['code'] !== T_STRING) {
+                // Not a function declaration, or incomplete.
+                return;
+            }
+
+            $type = 'methods';
+        } else {
+            $type = 'closures';
         }
 
         $methodProps = $phpcsFile->getMethodProperties($stackPtr);
@@ -68,7 +74,7 @@ public function processTokenWithinScope(File $phpcsFile, int $stackPtr, int $cur
         $next = $stackPtr;
         $end  = $tokens[$stackPtr]['scope_closer'];
 
-        $this->checkThisUsage($phpcsFile, $next, $end);
+        $this->checkThisUsage($phpcsFile, $next, $end, $type);
     }
 
 
@@ -78,10 +84,11 @@ public function processTokenWithinScope(File $phpcsFile, int $stackPtr, int $cur
      * @param \PHP_CodeSniffer\Files\File $phpcsFile The current file being scanned.
      * @param int                         $next      The position of the next token to check.
      * @param int                         $end       The position of the last token to check.
+     * @param string                      $type      Either 'method' or 'closure'.
      *
      * @return void
      */
-    private function checkThisUsage(File $phpcsFile, int $next, int $end)
+    private function checkThisUsage(File $phpcsFile, int $next, int $end, string $type)
     {
         $tokens = $phpcsFile->getTokens();
 
@@ -95,7 +102,7 @@ private function checkThisUsage(File $phpcsFile, int $next, int $end)
                 || $tokens[$next]['code'] === T_CLOSURE)
                 && isset($tokens[$next]['scope_opener']) === true
             ) {
-                $this->checkThisUsage($phpcsFile, $next, $tokens[$next]['scope_opener']);
+                $this->checkThisUsage($phpcsFile, $next, $tokens[$next]['scope_opener'], $type);
                 $next = $tokens[$next]['scope_closer'];
                 continue;
             }
@@ -104,8 +111,10 @@ private function checkThisUsage(File $phpcsFile, int $next, int $end)
                 continue;
             }
 
-            $error = 'Usage of "$this" in static methods will cause runtime errors';
-            $phpcsFile->addError($error, $next, 'Found');
+            $error = 'Usage of "$this" in static %s will cause runtime errors';
+            $data  = [$type];
+
+            $phpcsFile->addError($error, $next, 'Found', $data);
         } while ($next !== false);
     }
 
@@ -122,5 +131,18 @@ private function checkThisUsage(File $phpcsFile, int $next, int $end)
      */
     protected function processTokenOutsideScope(File $phpcsFile, int $stackPtr)
     {
+        $tokens = $phpcsFile->getTokens();
+
+        if ($tokens[$stackPtr]['code'] !== T_CLOSURE) {
+            // We're only interested in closures when looking outside of OO.
+            return;
+        }
+
+        $methodProps = $phpcsFile->getMethodProperties($stackPtr);
+        if ($methodProps['is_static'] === false) {
+            return;
+        }
+
+        $this->checkThisUsage($phpcsFile, $stackPtr, $tokens[$stackPtr]['scope_closer'], 'closures');
     }
 }

src/Standards/Squiz/Tests/Scope/StaticThisUsageUnitTest.inc

@@ -37,7 +37,7 @@ class MyClass
     }
 
     public static function func1() {
-        return function() {
+        return static function() {
             echo $this->name;
         };
     }
@@ -137,10 +137,42 @@ class NestedScopesShouldBeSkipped
 
         \Closure::bind(
             function ($this) { // Cannot use $this as a param, so this should still be flagged.
-                $this->x = 1; // This is fine. $this related to the Closure class, not to the NestedScopesShouldBeSkipped class.
+                $this->x = 1; // This is fine. $this related to the Target class, not to the NestedScopesShouldBeSkipped class.
             },
             new Target(),
             Target::class,
         )();
+
+        $callable = static function ($a, $b) {
+            return $a * $b * $this->multiplier; // This should be flagged. The closure is declared as static.
+        };
+    }
+
+    public function dontExamineMe() {
+        $this->shouldNotBeFlagged();
+
+        \Closure::bind(
+            function () {
+                $this->x = 1; // This is fine. $this related to the Target class, not to the NestedScopesShouldBeSkipped class.
+            },
+            new Target(),
+            Target::class,
+        )();
+
+        $callable = static function ($a, $b) {
+            return $a * $b * $this->multiplier; // This should be flagged. The closure is declared as static.
+        };
     }
 }
+
+function ignoreMe() {
+    $this->notAllowedButNotTheSameIssueAsThisSniffIsLookingFor();
+}
+
+$callable = function ($a, $b) {
+    return $a * $b * $this->multiplier; // This should NOT be flagged. The closure might be bound to an object.
+};
+
+$callable = static function ($a, $b) {
+    return $a * $b * $this->multiplier; // This should be flagged. The closure is declared as static.
+};

src/Standards/Squiz/Tests/Scope/StaticThisUsageUnitTest.php

@@ -37,6 +37,7 @@ public function getErrorList()
             9   => 1,
             14  => 1,
             20  => 1,
+            41  => 1,
             61  => 1,
             69  => 1,
             76  => 1,
@@ -47,6 +48,9 @@ public function getErrorList()
             132 => 1,
             135 => 1,
             139 => 1,
+            147 => 1,
+            163 => 1,
+            177 => 1,
         ];
     }