From 368b767dfe32e06525a22af6991529a0c7a49902 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Tue, 18 Aug 2020 16:46:19 -0400 Subject: [PATCH 1/6] Add tests for unused before require --- .../UnusedFollowedByRequireTest.php | 35 +++++++++++++++++++ .../UnusedFollowedByRequireFixture.php | 6 ++++ 2 files changed, 41 insertions(+) create mode 100644 Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php create mode 100644 Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php diff --git a/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php b/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php new file mode 100644 index 00000000..b19b9f1d --- /dev/null +++ b/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php @@ -0,0 +1,35 @@ +getFixture('UnusedFollowedByRequireFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 2, + 3, + 4, + ]; + $this->assertEquals($expectedWarnings, $lines); + } + + public function testUnusedFollowedByRequireWhenSet() { + $fixtureFile = $this->getFixture('UnusedFollowedByRequireFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedVariablesBeforeRequire', + 'true' + ); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 4, + ]; + $this->assertEquals($expectedWarnings, $lines); + } +} diff --git a/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php b/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php new file mode 100644 index 00000000..acc92df2 --- /dev/null +++ b/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php @@ -0,0 +1,6 @@ + Date: Tue, 18 Aug 2020 16:54:41 -0400 Subject: [PATCH 2/6] Move areFollowingArgumentsUsed to Helpers --- VariableAnalysis/Lib/Helpers.php | 29 +++++++++++++++ .../CodeAnalysis/VariableAnalysisSniff.php | 37 +++++-------------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 684b230d..b3461fbd 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -3,6 +3,9 @@ namespace VariableAnalysis\Lib; use PHP_CodeSniffer\Files\File; +use VariableAnalysis\Lib\ScopeInfo; +use VariableAnalysis\Lib\ScopeType; +use VariableAnalysis\Lib\VariableInfo; use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\FunctionDeclarations; @@ -652,4 +655,30 @@ public static function getLastNonEmptyTokenIndexInFile(File $phpcsFile) { self::debug('no non-empty token found for end of file'); return 0; } + + /** + * @param VariableInfo $varInfo + * @param ScopeInfo $scopeInfo + * + * @return bool + */ + public static function areFollowingArgumentsUsed(VariableInfo $varInfo, ScopeInfo $scopeInfo) { + $foundVarPosition = false; + foreach ($scopeInfo->variables as $variable) { + if ($variable === $varInfo) { + $foundVarPosition = true; + continue; + } + if (! $foundVarPosition) { + continue; + } + if ($variable->scopeType !== ScopeType::PARAM) { + continue; + } + if ($variable->firstRead) { + return true; + } + } + return false; + } } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 21e5a757..26367262 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -131,6 +131,15 @@ class VariableAnalysisSniff implements Sniff { */ public $allowUnusedForeachVariables = true; + /** + * If set to true, unused variables in a function before a require or import + * statement will not be marked as unused because they may be used in the + * required file. + * + * @var bool + */ + public $allowUnusedVariablesBeforeRequire = false; + /** * @return (int|string)[] */ @@ -337,32 +346,6 @@ protected function getOrCreateVariableInfo($varName, $currScope) { return $scopeInfo->variables[$varName]; } - /** - * @param VariableInfo $varInfo - * @param ScopeInfo $scopeInfo - * - * @return bool - */ - protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) { - $foundVarPosition = false; - foreach ($scopeInfo->variables as $variable) { - if ($variable === $varInfo) { - $foundVarPosition = true; - continue; - } - if (! $foundVarPosition) { - continue; - } - if ($variable->scopeType !== ScopeType::PARAM) { - continue; - } - if ($variable->firstRead) { - return true; - } - } - return false; - } - /** * @param string $varName * @param int $stackPtr @@ -1601,7 +1584,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v if ($this->allowUnusedFunctionParameters && $varInfo->scopeType === ScopeType::PARAM) { return; } - if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === ScopeType::PARAM && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) { + if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === ScopeType::PARAM && Helpers::areFollowingArgumentsUsed($varInfo, $scopeInfo)) { Helpers::debug("variable {$varInfo->name} at end of scope has unused following args"); return; } From ddd3078d6f082817ab10d1ef40777d60a852f932 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Tue, 18 Aug 2020 17:06:14 -0400 Subject: [PATCH 3/6] Add test cases for all forms of require/include --- .../UnusedFollowedByRequireTest.php | 12 ++++++++++++ .../UnusedFollowedByRequireFixture.php | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php b/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php index b19b9f1d..0ded6210 100644 --- a/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php +++ b/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php @@ -13,6 +13,15 @@ public function testUnusedFollowedByRequireDefault() { 2, 3, 4, + 8, + 9, + 10, + 14, + 15, + 16, + 20, + 21, + 22, ]; $this->assertEquals($expectedWarnings, $lines); } @@ -29,6 +38,9 @@ public function testUnusedFollowedByRequireWhenSet() { $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ 4, + 10, + 16, + 22, ]; $this->assertEquals($expectedWarnings, $lines); } diff --git a/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php b/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php index acc92df2..15482963 100644 --- a/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/UnusedFollowedByRequireFixture.php @@ -1,6 +1,24 @@ Date: Tue, 18 Aug 2020 17:06:29 -0400 Subject: [PATCH 4/6] Add isRequireInScopeAfter and use allowUnusedVariablesBeforeRequire --- VariableAnalysis/Lib/Helpers.php | 30 +++++++++++++++++++ .../CodeAnalysis/VariableAnalysisSniff.php | 3 ++ 2 files changed, 33 insertions(+) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index b3461fbd..34cb52af 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -681,4 +681,34 @@ public static function areFollowingArgumentsUsed(VariableInfo $varInfo, ScopeInf } return false; } + + /** + * @param File $phpcsFile + * @param VariableInfo $varInfo + * @param ScopeInfo $scopeInfo + * + * @return bool + */ + public static function isRequireInScopeAfter(File $phpcsFile, VariableInfo $varInfo, ScopeInfo $scopeInfo) { + $requireTokens = [ + T_REQUIRE, + T_REQUIRE_ONCE, + T_INCLUDE, + T_INCLUDE_ONCE, + ]; + $indexToStartSearch = $varInfo->firstDeclared; + if (! empty($varInfo->firstInitialized)) { + $indexToStartSearch = $varInfo->firstInitialized; + } + $tokens = $phpcsFile->getTokens(); + $indexToStopSearch = isset($tokens[$scopeInfo->owner]['scope_closer']) ? $tokens[$scopeInfo->owner]['scope_closer'] : null; + if (! is_int($indexToStartSearch) || ! is_int($indexToStopSearch)) { + return false; + } + $requireTokenIndex = $phpcsFile->findNext($requireTokens, $indexToStartSearch + 1, $indexToStopSearch); + if (is_int($requireTokenIndex)) { + return true; + } + return false; + } } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 26367262..1f6a4aeb 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1607,6 +1607,9 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v if (empty($varInfo->firstDeclared) && empty($varInfo->firstInitialized)) { return; } + if ($this->allowUnusedVariablesBeforeRequire && Helpers::isRequireInScopeAfter($phpcsFile, $varInfo, $scopeInfo)) { + return; + } $this->warnAboutUnusedVariable($phpcsFile, $varInfo); } From b850c9fd306f4ead449a6db5b82aabed0493f145 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Tue, 18 Aug 2020 17:09:11 -0400 Subject: [PATCH 5/6] Add test to be sure option does not break other behavior --- .../UnusedFollowedByRequireTest.php | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php b/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php index 0ded6210..701ddcfd 100644 --- a/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php +++ b/Tests/VariableAnalysisSniff/UnusedFollowedByRequireTest.php @@ -4,7 +4,7 @@ use VariableAnalysis\Tests\BaseTestCase; class UnusedFollowedByRequire extends BaseTestCase { - public function testUnusedFollowedByRequireDefault() { + public function testUnusedFollowedByRequireWarnsByDefault() { $fixtureFile = $this->getFixture('UnusedFollowedByRequireFixture.php'); $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); $phpcsFile->process(); @@ -26,7 +26,7 @@ public function testUnusedFollowedByRequireDefault() { $this->assertEquals($expectedWarnings, $lines); } - public function testUnusedFollowedByRequireWhenSet() { + public function testUnusedFollowedByRequireDoesNotWarnWhenSet() { $fixtureFile = $this->getFixture('UnusedFollowedByRequireFixture.php'); $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); $phpcsFile->ruleset->setSniffProperty( @@ -44,4 +44,31 @@ public function testUnusedFollowedByRequireWhenSet() { ]; $this->assertEquals($expectedWarnings, $lines); } + + public function testUnusedFollowedByRequireDoesNotBreakOtherThingsWhenSet() { + $fixtureFile = $this->getFixture('FunctionWithoutParamFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedVariablesBeforeRequire', + 'true' + ); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 18, + 19, + ]; + $this->assertEquals($expectedWarnings, $lines); + } } From 4d6ef1e03084832b62745da5e82a9ea7ccc43298 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Tue, 18 Aug 2020 17:11:04 -0400 Subject: [PATCH 6/6] Add allowUnusedVariablesBeforeRequire to README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 71bcf486..42144206 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,7 @@ The available options are as follows: - `allowUnusedFunctionParameters` (bool, default `false`): if set to true, function arguments will never be marked as unused. - `allowUnusedCaughtExceptions` (bool, default `true`): if set to true, caught Exception variables will never be marked as unused. - `allowUnusedParametersBeforeUsed` (bool, default `true`): if set to true, unused function arguments will be ignored if they are followed by used function arguments. +- `allowUnusedVariablesBeforeRequire` (bool, default `false`): if set to true, variables defined before a `require`, `require_once`, `include`, or `include_once` will not be marked as unused. They may be intended for the required file. - `allowUndefinedVariablesInFileScope` (bool, default `false`): if set to true, undefined variables in the file's top-level scope will never be marked as undefined. - `validUnusedVariableNames` (string, default `null`): a space-separated list of names of placeholder variables that you want to ignore from unused variable warnings. For example, to ignore the variables `$junk` and `$unused`, this could be set to `'junk unused'`. - `ignoreUnusedRegexp` (string, default `null`): a PHP regexp string (note that this requires explicit delimiters) for variables that you want to ignore from unused variable warnings. For example, to ignore the variables `$_junk` and `$_unused`, this could be set to `'/^_/'`.