From 99e30498ce74faef4a650d682af4362dfcd007a5 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 30 Nov 2024 13:38:54 -0500 Subject: [PATCH 1/6] Add test for compact used within arrow function --- Tests/VariableAnalysisSniff/VariableAnalysisTest.php | 4 ++++ .../VariableAnalysisSniff/fixtures/CompactFixture.php | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/Tests/VariableAnalysisSniff/VariableAnalysisTest.php b/Tests/VariableAnalysisSniff/VariableAnalysisTest.php index 4fe7516..ed11f48 100644 --- a/Tests/VariableAnalysisSniff/VariableAnalysisTest.php +++ b/Tests/VariableAnalysisSniff/VariableAnalysisTest.php @@ -522,6 +522,8 @@ public function testCompactWarnings() 23, 26, 36, + 41, + 42, ]; $this->assertSame($expectedWarnings, $lines); } @@ -543,6 +545,8 @@ public function testCompactWarningsHaveCorrectSniffCodes() $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[26][66][0]['source']); $this->assertSame(self::UNUSED_ERROR_CODE, $warnings[36][5][0]['source']); $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[36][23][0]['source']); + $this->assertSame(self::UNUSED_ERROR_CODE, $warnings[41][22][0]['source']); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[42][39][0]['source']); } public function testTraitAllowsThis() diff --git a/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php b/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php index d259b1b..e4da908 100644 --- a/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php @@ -35,3 +35,14 @@ function foo() { $a = 'Hello'; $c = compact( $a, $b ); // Unused variable c and undefined variable b } + +function function_with_arrow_function_and_compact() { + $make_array = fn ($arg) => compact('arg'); + $make_nothing = fn ($arg) => []; // Unused variable $arg + $make_no_variable = fn () => compact('arg') // Undefined variable $arg + echo $make_array('hello'); + echo $make_nothing('hello'); + echo $make_no_variable(); + $make_array_multiple = fn ($arg1, $arg2, $arg3) => compact('arg1', 'arg2', 'arg3'); + echo $make_array_multiple(); +} From 78f099add7fd6b38210f791fda750819e0c36582 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sun, 1 Dec 2024 14:19:47 -0500 Subject: [PATCH 2/6] Process every variable in compact separately --- VariableAnalysis/Lib/Helpers.php | 58 +++++++++++++++ .../CodeAnalysis/VariableAnalysisSniff.php | 70 +++---------------- 2 files changed, 68 insertions(+), 60 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 6af1b22..50b4ce1 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -445,6 +445,64 @@ public static function findVariableScope(File $phpcsFile, $stackPtr, $varName = return self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr); } + /** + * Return the variable names of each variable targetted by a `compact()` call. + * + * @param File $phpcsFile + * @param int $stackPtr + * @param array> $arguments The stack pointers of each argument; see findFunctionCallArguments + * + * @return string[] + */ + public static function getVariableNamesFromCompact(File $phpcsFile, $stackPtr, $arguments) + { + $tokens = $phpcsFile->getTokens(); + $variableNames = []; + + foreach ($arguments as $argumentPtrs) { + $argumentPtrs = array_values(array_filter($argumentPtrs, function ($argumentPtr) use ($tokens) { + return isset(Tokens::$emptyTokens[$tokens[$argumentPtr]['code']]) === false; + })); + if (empty($argumentPtrs)) { + continue; + } + if (!isset($tokens[$argumentPtrs[0]])) { + continue; + } + $argumentFirstToken = $tokens[$argumentPtrs[0]]; + if ($argumentFirstToken['code'] === T_ARRAY) { + // It's an array argument, recurse. + $arrayArguments = Helpers::findFunctionCallArguments($phpcsFile, $argumentPtrs[0]); + $variableNames = array_merge($variableNames, self::getVariableNamesFromCompact($phpcsFile, $stackPtr, $arrayArguments)); + continue; + } + if (count($argumentPtrs) > 1) { + // Complex argument, we can't handle it, ignore. + continue; + } + if ($argumentFirstToken['code'] === T_CONSTANT_ENCAPSED_STRING) { + // Single-quoted string literal, ie compact('whatever'). + // Substr is to strip the enclosing single-quotes. + $varName = substr($argumentFirstToken['content'], 1, -1); + $variableNames[] = $varName; + continue; + } + if ($argumentFirstToken['code'] === T_DOUBLE_QUOTED_STRING) { + // Double-quoted string literal. + $regexp = Constants::getDoubleQuotedVarRegexp(); + if (! empty($regexp) && preg_match($regexp, $argumentFirstToken['content'])) { + // Bail if the string needs variable expansion, that's runtime stuff. + continue; + } + // Substr is to strip the enclosing double-quotes. + $varName = substr($argumentFirstToken['content'], 1, -1); + $variableNames[] = $varName; + continue; + } + } + return $variableNames; + } + /** * Return the token index of the scope start for a token * diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 531cd40..3294530 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1869,61 +1869,6 @@ protected function processVariableInString(File $phpcsFile, $stackPtr) } } - /** - * @param File $phpcsFile - * @param int $stackPtr - * @param array> $arguments The stack pointers of each argument - * @param int $currScope - * - * @return void - */ - protected function processCompactArguments(File $phpcsFile, $stackPtr, $arguments, $currScope) - { - $tokens = $phpcsFile->getTokens(); - - foreach ($arguments as $argumentPtrs) { - $argumentPtrs = array_values(array_filter($argumentPtrs, function ($argumentPtr) use ($tokens) { - return isset(Tokens::$emptyTokens[$tokens[$argumentPtr]['code']]) === false; - })); - if (empty($argumentPtrs)) { - continue; - } - if (!isset($tokens[$argumentPtrs[0]])) { - continue; - } - $argumentFirstToken = $tokens[$argumentPtrs[0]]; - if ($argumentFirstToken['code'] === T_ARRAY) { - // It's an array argument, recurse. - $arrayArguments = Helpers::findFunctionCallArguments($phpcsFile, $argumentPtrs[0]); - $this->processCompactArguments($phpcsFile, $stackPtr, $arrayArguments, $currScope); - continue; - } - if (count($argumentPtrs) > 1) { - // Complex argument, we can't handle it, ignore. - continue; - } - if ($argumentFirstToken['code'] === T_CONSTANT_ENCAPSED_STRING) { - // Single-quoted string literal, ie compact('whatever'). - // Substr is to strip the enclosing single-quotes. - $varName = substr($argumentFirstToken['content'], 1, -1); - $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $argumentPtrs[0], $currScope); - continue; - } - if ($argumentFirstToken['code'] === T_DOUBLE_QUOTED_STRING) { - // Double-quoted string literal. - $regexp = Constants::getDoubleQuotedVarRegexp(); - if (! empty($regexp) && preg_match($regexp, $argumentFirstToken['content'])) { - // Bail if the string needs variable expansion, that's runtime stuff. - continue; - } - // Substr is to strip the enclosing double-quotes. - $varName = substr($argumentFirstToken['content'], 1, -1); - $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $argumentPtrs[0], $currScope); - continue; - } - } - } - /** * Called to process variables named in a call to compact(). * @@ -1934,13 +1879,18 @@ protected function processCompactArguments(File $phpcsFile, $stackPtr, $argument */ protected function processCompact(File $phpcsFile, $stackPtr) { - $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr); - if ($currScope === null) { - return; - } + Helpers::debug("processCompact at {$stackPtr}"); $arguments = Helpers::findFunctionCallArguments($phpcsFile, $stackPtr); - $this->processCompactArguments($phpcsFile, $stackPtr, $arguments, $currScope); + $variableNames = Helpers::getVariableNamesFromCompact($phpcsFile, $stackPtr, $arguments); + + foreach ( $variableNames as $variableName ) { + $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr, $variableName); + if ($currScope === null) { + continue; + } + $this->markVariableReadAndWarnIfUndefined($phpcsFile, $variableName, $stackPtr, $currScope); + } } /** From 445ed17b673be4d2d16e905f1332a4f75ee9baa7 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sun, 1 Dec 2024 14:29:05 -0500 Subject: [PATCH 3/6] Also track position of compact variables --- VariableAnalysis/Lib/Helpers.php | 20 +++++++++++-------- .../CodeAnalysis/VariableAnalysisSniff.php | 10 ++++------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 50b4ce1..d793e67 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -446,18 +446,18 @@ public static function findVariableScope(File $phpcsFile, $stackPtr, $varName = } /** - * Return the variable names of each variable targetted by a `compact()` call. + * Return the variable names and positions of each variable targetted by a `compact()` call. * * @param File $phpcsFile * @param int $stackPtr * @param array> $arguments The stack pointers of each argument; see findFunctionCallArguments * - * @return string[] + * @return array each variable's firstRead position and its name; other VariableInfo properties are not set! */ - public static function getVariableNamesFromCompact(File $phpcsFile, $stackPtr, $arguments) + public static function getVariablesInsideCompact(File $phpcsFile, $stackPtr, $arguments) { $tokens = $phpcsFile->getTokens(); - $variableNames = []; + $variablePositionsAndNames = []; foreach ($arguments as $argumentPtrs) { $argumentPtrs = array_values(array_filter($argumentPtrs, function ($argumentPtr) use ($tokens) { @@ -473,7 +473,7 @@ public static function getVariableNamesFromCompact(File $phpcsFile, $stackPtr, $ if ($argumentFirstToken['code'] === T_ARRAY) { // It's an array argument, recurse. $arrayArguments = Helpers::findFunctionCallArguments($phpcsFile, $argumentPtrs[0]); - $variableNames = array_merge($variableNames, self::getVariableNamesFromCompact($phpcsFile, $stackPtr, $arrayArguments)); + $variablePositionsAndNames = array_merge($variablePositionsAndNames, self::getVariablesInsideCompact($phpcsFile, $stackPtr, $arrayArguments)); continue; } if (count($argumentPtrs) > 1) { @@ -484,7 +484,9 @@ public static function getVariableNamesFromCompact(File $phpcsFile, $stackPtr, $ // Single-quoted string literal, ie compact('whatever'). // Substr is to strip the enclosing single-quotes. $varName = substr($argumentFirstToken['content'], 1, -1); - $variableNames[] = $varName; + $variable = new VariableInfo($varName); + $variable->firstRead = $argumentPtrs[0]; + $variablePositionsAndNames[] = $variable; continue; } if ($argumentFirstToken['code'] === T_DOUBLE_QUOTED_STRING) { @@ -496,11 +498,13 @@ public static function getVariableNamesFromCompact(File $phpcsFile, $stackPtr, $ } // Substr is to strip the enclosing double-quotes. $varName = substr($argumentFirstToken['content'], 1, -1); - $variableNames[] = $varName; + $variable = new VariableInfo($varName); + $variable->firstRead = $argumentPtrs[0]; + $variablePositionsAndNames[] = $variable; continue; } } - return $variableNames; + return $variablePositionsAndNames; } /** diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 3294530..e9c6f8a 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1880,16 +1880,14 @@ protected function processVariableInString(File $phpcsFile, $stackPtr) protected function processCompact(File $phpcsFile, $stackPtr) { Helpers::debug("processCompact at {$stackPtr}"); - $arguments = Helpers::findFunctionCallArguments($phpcsFile, $stackPtr); - $variableNames = Helpers::getVariableNamesFromCompact($phpcsFile, $stackPtr, $arguments); - - foreach ( $variableNames as $variableName ) { - $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr, $variableName); + $variables = Helpers::getVariablesInsideCompact($phpcsFile, $stackPtr, $arguments); + foreach ( $variables as $variable ) { + $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr, $variable->name); if ($currScope === null) { continue; } - $this->markVariableReadAndWarnIfUndefined($phpcsFile, $variableName, $stackPtr, $currScope); + $this->markVariableReadAndWarnIfUndefined($phpcsFile, $variable->name, $variable->firstRead, $currScope); } } From a792e232c0a6c13d4e477f6ca8e93a11fa689222 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sun, 1 Dec 2024 14:31:13 -0500 Subject: [PATCH 4/6] Guard against missing compact variable position --- VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index e9c6f8a..c347327 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1887,7 +1887,8 @@ protected function processCompact(File $phpcsFile, $stackPtr) if ($currScope === null) { continue; } - $this->markVariableReadAndWarnIfUndefined($phpcsFile, $variable->name, $variable->firstRead, $currScope); + $variablePosition = $variable->firstRead ? $variable->firstRead : $stackPtr; + $this->markVariableReadAndWarnIfUndefined($phpcsFile, $variable->name, $variablePosition, $currScope); } } From b1b9871f2231d771f417d7a963bed449b62aee6c Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sun, 1 Dec 2024 14:33:12 -0500 Subject: [PATCH 5/6] Fix linting errors --- VariableAnalysis/Lib/Helpers.php | 3 ++- VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index d793e67..73164e1 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -4,6 +4,7 @@ use PHP_CodeSniffer\Files\File; use VariableAnalysis\Lib\ScopeInfo; +use VariableAnalysis\Lib\Constants; use VariableAnalysis\Lib\ForLoopInfo; use VariableAnalysis\Lib\EnumInfo; use VariableAnalysis\Lib\ScopeType; @@ -472,7 +473,7 @@ public static function getVariablesInsideCompact(File $phpcsFile, $stackPtr, $ar $argumentFirstToken = $tokens[$argumentPtrs[0]]; if ($argumentFirstToken['code'] === T_ARRAY) { // It's an array argument, recurse. - $arrayArguments = Helpers::findFunctionCallArguments($phpcsFile, $argumentPtrs[0]); + $arrayArguments = self::findFunctionCallArguments($phpcsFile, $argumentPtrs[0]); $variablePositionsAndNames = array_merge($variablePositionsAndNames, self::getVariablesInsideCompact($phpcsFile, $stackPtr, $arrayArguments)); continue; } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index c347327..5eaea94 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1882,7 +1882,7 @@ protected function processCompact(File $phpcsFile, $stackPtr) Helpers::debug("processCompact at {$stackPtr}"); $arguments = Helpers::findFunctionCallArguments($phpcsFile, $stackPtr); $variables = Helpers::getVariablesInsideCompact($phpcsFile, $stackPtr, $arguments); - foreach ( $variables as $variable ) { + foreach ($variables as $variable) { $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr, $variable->name); if ($currScope === null) { continue; From e2f7e3d195663100f6582e1703b538b375c430c2 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sun, 1 Dec 2024 14:38:26 -0500 Subject: [PATCH 6/6] Also add test for outside var --- Tests/VariableAnalysisSniff/fixtures/CompactFixture.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php b/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php index e4da908..e10a2f9 100644 --- a/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/CompactFixture.php @@ -39,10 +39,13 @@ function foo() { function function_with_arrow_function_and_compact() { $make_array = fn ($arg) => compact('arg'); $make_nothing = fn ($arg) => []; // Unused variable $arg - $make_no_variable = fn () => compact('arg') // Undefined variable $arg + $make_no_variable = fn () => compact('arg'); echo $make_array('hello'); echo $make_nothing('hello'); echo $make_no_variable(); $make_array_multiple = fn ($arg1, $arg2, $arg3) => compact('arg1', 'arg2', 'arg3'); echo $make_array_multiple(); + $outside_var = 'hello world'; + $use_outside_var = fn($inside_var) => compact('outside_var', 'inside_var'); + echo $use_outside_var(); }