From 5bddc931c3b6f3eda5b7cb83dc73dd41f60f2b1a Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 7 Sep 2020 16:54:42 -0400 Subject: [PATCH 1/6] Add tests for isset and empty --- Tests/VariableAnalysisSniff/IssetTest.php | 18 +++++++++++++ .../fixtures/IssetFixture.php | 25 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 Tests/VariableAnalysisSniff/IssetTest.php create mode 100644 Tests/VariableAnalysisSniff/fixtures/IssetFixture.php diff --git a/Tests/VariableAnalysisSniff/IssetTest.php b/Tests/VariableAnalysisSniff/IssetTest.php new file mode 100644 index 00000000..736dcf97 --- /dev/null +++ b/Tests/VariableAnalysisSniff/IssetTest.php @@ -0,0 +1,18 @@ +getFixture('IssetFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 4, + 23, // ideally this should not be a warning, but will be because it is difficult to know: https://github.com/sirbrillig/phpcs-variable-analysis/issues/202#issuecomment-688507314 + ]; + $this->assertEquals($expectedWarnings, $lines); + } +} diff --git a/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php b/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php new file mode 100644 index 00000000..3e47bb50 --- /dev/null +++ b/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php @@ -0,0 +1,25 @@ + Date: Mon, 7 Sep 2020 17:19:26 -0400 Subject: [PATCH 2/6] Ignore undefined variables inside isset or empty --- VariableAnalysis/Lib/Helpers.php | 55 +++++++++++++++++++ .../CodeAnalysis/VariableAnalysisSniff.php | 6 ++ 2 files changed, 61 insertions(+) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index ae039994..ae1d5df1 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -107,6 +107,9 @@ public static function isTokenInsideFunctionDefinitionArgumentList(File $phpcsFi * * Will also work for the parenthesis that make up the function definition's arguments list. * + * For arguments inside a function call, rather than a definition, use + * `getFunctionIndexForFunctionCallArgument`. + * * @param File $phpcsFile * @param int $stackPtr * @@ -730,4 +733,56 @@ public static function isRequireInScopeAfter(File $phpcsFile, VariableInfo $varI } return false; } + + /** + * Find the index of the function keyword for a token in a function call's arguments + * + * @param File $phpcsFile + * @param int $stackPtr + * + * @return ?int + */ + public static function getFunctionIndexForFunctionCallArgument(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + $token = $tokens[$stackPtr]; + if (empty($token['nested_parenthesis'])) { + return null; + } + $startingParenthesis = array_keys($token['nested_parenthesis']); + $startOfArguments = end($startingParenthesis); + + $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens); + $nonFunctionTokenTypes[] = T_STRING; + $nonFunctionTokenTypes[] = T_BITWISE_AND; + $functionPtr = self::getIntOrNull($phpcsFile->findPrevious($nonFunctionTokenTypes, $startOfArguments - 1, null, true, null, true)); + if (! is_int($functionPtr)) { + return null; + } + return $functionPtr; + } + + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return bool + */ + public static function isVariableInsideIssetOrEmpty(File $phpcsFile, $stackPtr) { + $functionIndex = self::getFunctionIndexForFunctionCallArgument($phpcsFile, $stackPtr); + if (! is_int($functionIndex)) { + return false; + } + $tokens = $phpcsFile->getTokens(); + if (! isset($tokens[$functionIndex])) { + return false; + } + $allowedFunctionNames = [ + 'isset', + 'empty', + ]; + if (in_array($tokens[$functionIndex]['content'], $allowedFunctionNames, true)) { + return true; + } + return false; + } } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index d04d0917..53bf6800 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1363,6 +1363,12 @@ protected function processVariable(File $phpcsFile, $stackPtr) { return; } + // Are we an isset or empty call? + if (Helpers::isVariableInsideIssetOrEmpty($phpcsFile, $stackPtr)) { + Helpers::debug('found isset or empty'); + return; + } + // Are we a $GLOBALS, $_REQUEST, etc superglobal? if ($this->processVariableAsSuperGlobal($varName)) { Helpers::debug('found superglobal'); From e5a212bc55addc9ccf5917c500ffd75032caab30 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 7 Sep 2020 17:50:57 -0400 Subject: [PATCH 3/6] Make sure we detect the start of arguments --- VariableAnalysis/Lib/Helpers.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index ae1d5df1..c440bb6a 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -133,6 +133,10 @@ public static function getFunctionIndexForFunctionArgument(File $phpcsFile, $sta $startOfArguments = end($startingParenthesis); } + if (! is_int($startOfArguments)) { + return null; + } + $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens); $nonFunctionTokenTypes[] = T_STRING; $nonFunctionTokenTypes[] = T_BITWISE_AND; @@ -750,6 +754,9 @@ public static function getFunctionIndexForFunctionCallArgument(File $phpcsFile, } $startingParenthesis = array_keys($token['nested_parenthesis']); $startOfArguments = end($startingParenthesis); + if (! is_int($startOfArguments)) { + return null; + } $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens); $nonFunctionTokenTypes[] = T_STRING; From 9cdb43f79236c3e1d470bc6113ce9d03c885f962 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 7 Sep 2020 18:06:25 -0400 Subject: [PATCH 4/6] Add test to make sure isset still counts as read --- .../VariableAnalysisSniff/fixtures/IssetFixture.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php b/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php index 3e47bb50..e7c00495 100644 --- a/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/IssetFixture.php @@ -23,3 +23,15 @@ function shouldIgnoreUndefinedVariableUseAfterIsset() { doSomething($undefinedVar); // ideally this should not be a warning, but will be because it is difficult to know: https://github.com/sirbrillig/phpcs-variable-analysis/issues/202#issuecomment-688507314 } } + +function shouldCountVariableUseInsideIssetAsRead($definedVar) { + if (isset($definedVar)) { + doSomething(); + } +} + +function shouldCountVariableUseInsideEmptyAsRead($definedVar) { + if (empty($definedVar)) { + doSomething(); + } +} From 574428e8aa49ece015f550de395b6dd1a35cdbf7 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 7 Sep 2020 18:07:14 -0400 Subject: [PATCH 5/6] Explicitly do not count variable definitions for getFunctionIndexForFunctionCallArgument --- VariableAnalysis/Lib/Helpers.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index c440bb6a..0e49183a 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -759,10 +759,11 @@ public static function getFunctionIndexForFunctionCallArgument(File $phpcsFile, } $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens); - $nonFunctionTokenTypes[] = T_STRING; - $nonFunctionTokenTypes[] = T_BITWISE_AND; $functionPtr = self::getIntOrNull($phpcsFile->findPrevious($nonFunctionTokenTypes, $startOfArguments - 1, null, true, null, true)); - if (! is_int($functionPtr)) { + if (! is_int($functionPtr) || ! isset($tokens[$functionPtr]['code'])) { + return null; + } + if ($tokens[$functionPtr]['code'] === 'function' || ($tokens[$functionPtr]['content'] === 'fn' && FunctionDeclarations::isArrowFunction($phpcsFile, $functionPtr))) { return null; } return $functionPtr; From d7f17c16dee3be989a373b4fe8e58b948598cd25 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 7 Sep 2020 18:07:52 -0400 Subject: [PATCH 6/6] Mark isset/empty usage as a read --- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 53bf6800..186b4c1d 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1363,12 +1363,6 @@ protected function processVariable(File $phpcsFile, $stackPtr) { return; } - // Are we an isset or empty call? - if (Helpers::isVariableInsideIssetOrEmpty($phpcsFile, $stackPtr)) { - Helpers::debug('found isset or empty'); - return; - } - // Are we a $GLOBALS, $_REQUEST, etc superglobal? if ($this->processVariableAsSuperGlobal($varName)) { Helpers::debug('found superglobal'); @@ -1446,6 +1440,13 @@ protected function processVariable(File $phpcsFile, $stackPtr) { return; } + // Are we an isset or empty call? + if (Helpers::isVariableInsideIssetOrEmpty($phpcsFile, $stackPtr)) { + Helpers::debug('found isset or empty'); + $this->markVariableRead($varName, $stackPtr, $currScope); + return; + } + // OK, we don't appear to be a write to the var, assume we're a read. Helpers::debug('looks like a variable read'); $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);