From aaf902277f2889fddbdb37046ae02b9965e2cf0f Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 8 Sep 2022 14:35:53 -0400
Subject: [PATCH] Fix function call detection to exclude non-function tokens
 (#279)

* Add test for static var inside anon function

* Do not consider non-function names to be function names

* Fix small for loop test

* Add test for unused init var in for loop

* Add test for static var inside anonymous func inside closure

* Do not consider variables in closures as part of func args
---
 Tests/VariableAnalysisSniff/ForLoopTest.php   |  4 ++++
 .../fixtures/FunctionWithClosureFixture.php   | 15 +++++++++++++
 .../fixtures/FunctionWithForLoopFixture.php   | 17 +++++++++++++-
 VariableAnalysis/Lib/Helpers.php              | 22 ++++++++++++++++++-
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/Tests/VariableAnalysisSniff/ForLoopTest.php b/Tests/VariableAnalysisSniff/ForLoopTest.php
index de10478..778e997 100644
--- a/Tests/VariableAnalysisSniff/ForLoopTest.php
+++ b/Tests/VariableAnalysisSniff/ForLoopTest.php
@@ -23,6 +23,8 @@ public function testFunctionWithForLoopWarningsReportsCorrectLines()
 			118,
 			123,
 			129,
+			143,
+			145,
 		];
 		$this->assertSame($expectedWarnings, $lines);
 	}
@@ -43,5 +45,7 @@ public function testFunctionWithForLoopWarningsHasCorrectSniffCodes()
 		$this->assertSame(self::UNUSED_ERROR_CODE, $warnings[118][5][0]['source']);
 		$this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[123][5][0]['source']);
 		$this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[129][14][0]['source']);
+		$this->assertSame(self::UNUSED_ERROR_CODE, $warnings[143][5][0]['source']);
+		$this->assertSame(self::UNUSED_ERROR_CODE, $warnings[145][3][0]['source']);
 	}
 }
diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithClosureFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithClosureFixture.php
index d5cb237..40cb0c4 100644
--- a/Tests/VariableAnalysisSniff/fixtures/FunctionWithClosureFixture.php
+++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithClosureFixture.php
@@ -101,3 +101,18 @@ function function_with_static_closure() {
         echo $inner_param;
     }, $params);
 }
+
+function function_with_static_variable_inside_anonymous_function() {
+    $anon = (function() {
+      static $test;
+      echo $test;
+    });
+    $anon();
+}
+
+function function_with_static_variable_inside_anonymous_function_inside_arguments() {
+    add_action('test', function () {
+      static $providerId;
+      echo $providerId;
+    });
+}
diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php
index 0f48f45..0c2ce6f 100644
--- a/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php
+++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php
@@ -138,8 +138,23 @@ function veryBoringForLoop() {
   }
 }
 
+function reallySmallForLoopWithUnusedInitVar() {
+  for ($i = 1,
+    $j = 0; // Unused variable $j
+  $i <= 10;
+  $j += $i, // Unused variable $j
+    print $i,
+    $i++);
+}
+
 function reallySmallForLoop() {
-  for ($i = 1, $j = 0; $i <= 10; $j += $i, print $i, $i++);
+  for ($i = 1,
+    $j = 0;
+  $i <= 10;
+  $j += $i,
+    print $i +
+    $j,
+    $i++);
 }
 
 function colonSyntaxForLoop() {
diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php
index 2e47390..30f7735 100644
--- a/VariableAnalysis/Lib/Helpers.php
+++ b/VariableAnalysis/Lib/Helpers.php
@@ -1092,12 +1092,32 @@ public static function getFunctionIndexForFunctionCallArgument(File $phpcsFile,
 		if (! is_int($functionPtr) || ! isset($tokens[$functionPtr]['code'])) {
 			return null;
 		}
-		if ($tokens[$functionPtr]['content'] === 'function' || ($tokens[$functionPtr]['content'] === 'fn' && self::isArrowFunction($phpcsFile, $functionPtr))) {
+		if (
+			$tokens[$functionPtr]['content'] === 'function'
+			|| ($tokens[$functionPtr]['content'] === 'fn' && self::isArrowFunction($phpcsFile, $functionPtr))
+		) {
+			// If there is a function/fn keyword before the beginning of the parens,
+			// this is a function definition and not a function call.
 			return null;
 		}
 		if (! empty($tokens[$functionPtr]['scope_opener'])) {
+			// If the alleged function name has a scope, this is not a function call.
+			return null;
+		}
+
+		$functionNameType = $tokens[$functionPtr]['code'];
+		if (! in_array($functionNameType, Tokens::$functionNameTokens, true)) {
+			// If the alleged function name is not a variable or a string, this is
+			// not a function call.
 			return null;
 		}
+
+		if ($tokens[$functionPtr]['level'] !== $tokens[$stackPtr]['level']) {
+			// If the variable is inside a different scope than the function name,
+			// the function call doesn't apply to the variable.
+			return null;
+		}
+
 		return $functionPtr;
 	}