Merge pull request #1377 from PHPCSStandards/feature/1357/squiz-staticthisusage-fix-false-positive-nested-closure

Squiz/StaticThisUsage: improve handling of anonymous functions

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 = 'method';
+        } else {
+            $type = 'closure';
         }
 
         $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,21 +84,25 @@ 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      Type of context being checked. 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();
 
         do {
-            $next = $phpcsFile->findNext([T_VARIABLE, T_ANON_CLASS], ($next + 1), $end);
+            $next = $phpcsFile->findNext([T_VARIABLE, T_CLOSURE, T_ANON_CLASS], ($next + 1), $end);
             if ($next === false) {
                 continue;
             }
 
-            if ($tokens[$next]['code'] === T_ANON_CLASS) {
-                $this->checkThisUsage($phpcsFile, $next, $tokens[$next]['scope_opener']);
+            if (($tokens[$next]['code'] === T_ANON_CLASS
+                || $tokens[$next]['code'] === T_CLOSURE)
+                && isset($tokens[$next]['scope_opener']) === true
+            ) {
+                $this->checkThisUsage($phpcsFile, $next, $tokens[$next]['scope_opener'], $type);
                 $next = $tokens[$next]['scope_closer'];
                 continue;
             }
@@ -101,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 a static %s will cause runtime errors';
+            $data  = [$type];
+
+            $phpcsFile->addError($error, $next, 'Found', $data);
         } while ($next !== false);
     }
 
@@ -119,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'], 'closure');
     }
 }

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

@@ -1,6 +1,6 @@
 <?php
-class MyClass
-{
+abstract class MyClass {
+    abstract public static function ignoreMe($a, $b);
     public static function func1()
     {
         $value = 'hello';
@@ -37,7 +37,7 @@ class MyClass
     }
 
     public static function func1() {
-        return function() {
+        return static function() {
             echo $this->name;
         };
     }
@@ -125,3 +125,54 @@ enum MyEnum {
         $this->doSomething();
     }
 }
+
+class NestedScopesShouldBeSkipped
+{
+    public static function examineMe() {
+        $this->shouldBeFlagged();
+
+        function nestedFunction() {
+            $this->nestedFunctionsDoNotHaveAccessToThis();
+        }
+
+        \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 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

@@ -45,6 +45,12 @@ public function getErrorList()
             84  => 1,
             99  => 1,
             125 => 1,
+            132 => 1,
+            135 => 1,
+            139 => 1,
+            147 => 1,
+            163 => 1,
+            177 => 1,
         ];
     }