From 328f2fd865aa18b30154432ccfb67ff86764782e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 12 Oct 2023 02:56:07 +0200 Subject: [PATCH 1/3] Squiz/NonExecutableCode: make sniff more code style independent When determining whether a `return` statement is the last code token in a function body, comments should be ignored, but weren't. Fixed now. Includes tests. --- .../Squiz/Sniffs/PHP/NonExecutableCodeSniff.php | 4 ++-- .../Tests/PHP/NonExecutableCodeUnitTest.1.inc | 17 +++++++++++++++++ .../Tests/PHP/NonExecutableCodeUnitTest.php | 2 ++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php index 31d9c5a351..4ea1306cb2 100644 --- a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php +++ b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php @@ -114,9 +114,9 @@ public function process(File $phpcsFile, $stackPtr) } if ($tokens[$stackPtr]['code'] === T_RETURN) { - $next = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true); + $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($stackPtr + 1), null, true); if ($tokens[$next]['code'] === T_SEMICOLON) { - $next = $phpcsFile->findNext(T_WHITESPACE, ($next + 1), null, true); + $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); if ($tokens[$next]['code'] === T_CLOSE_CURLY_BRACKET) { // If this is the closing brace of a function // then this return statement doesn't return anything diff --git a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc index 051b6c6b11..347868d404 100644 --- a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc @@ -396,5 +396,22 @@ function executionStoppingThrowExpressionsE() { echo 'non-executable'; } +function returnNotRequiredIgnoreCommentsA() +{ + if ($something === TRUE) { + return /*comment*/; + } + + echo 'foo'; + return /*comment*/; +} + +function returnNotRequiredIgnoreCommentsB() +{ + echo 'foo'; + return; + /*comment*/ +} + // Intentional syntax error. return array_map( diff --git a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php index 40e4068e42..887c6b5044 100644 --- a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php +++ b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php @@ -82,6 +82,8 @@ public function getWarningList($testFile='') 386 => 1, 391 => 1, 396 => 1, + 406 => 1, + 412 => 1, ]; break; case 'NonExecutableCodeUnitTest.2.inc': From e83b12bd50c8a30aedd9b12b248fa0b37c8c9a33 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 12 Oct 2023 03:02:31 +0200 Subject: [PATCH 2/3] Squiz/NonExecutableCode: flag redundant `return` statements in closures too A return statement which doesn't return a value at the end of a function body would be flagged as "not required" for named functions, but not so for anonymous functions. Fixed now. Includes tests. --- src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php | 4 +++- .../Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc | 6 ++++++ src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php index 4ea1306cb2..7959a95bff 100644 --- a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php +++ b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php @@ -122,7 +122,9 @@ public function process(File $phpcsFile, $stackPtr) // then this return statement doesn't return anything // and is not required anyway. $owner = $tokens[$next]['scope_condition']; - if ($tokens[$owner]['code'] === T_FUNCTION) { + if ($tokens[$owner]['code'] === T_FUNCTION + || $tokens[$owner]['code'] === T_CLOSURE + ) { $warning = 'Empty return statement not required here'; $phpcsFile->addWarning($warning, $stackPtr, 'ReturnNotRequired'); return; diff --git a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc index 347868d404..2efcc78e57 100644 --- a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc @@ -413,5 +413,11 @@ function returnNotRequiredIgnoreCommentsB() /*comment*/ } +$closure = function () +{ + echo 'foo'; + return; // This return should be flagged as not required. +}; + // Intentional syntax error. return array_map( diff --git a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php index 887c6b5044..c6955930c5 100644 --- a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php +++ b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php @@ -84,6 +84,7 @@ public function getWarningList($testFile='') 396 => 1, 406 => 1, 412 => 1, + 419 => 1, ]; break; case 'NonExecutableCodeUnitTest.2.inc': From 15fceb3654be5e7f0ea5042c808f52d3cdbea300 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Jun 2023 01:00:37 +0200 Subject: [PATCH 3/3] Squiz/NonExecutableCode: fold duplicate code Follow up on commits 0e10f432e18a97135a7ec7ecbe61ad9a8d2e4231 and 01754d9c1a622def2f7d04747ab7ebcce767aef4, which both deal with fixing bugs where the sniff would not handle if/elseif/else conditions without curly braces correctly. This commit merges the two near duplicate code blocks, which the above mentioned commits introduced, each containing code doing essentially the same thing. Also note that `T_ELSE` is handled separately now as `else` does not take parentheses and can therefore not be a parenthesis owner. This change is already covered by pre-existing tests. --- .../Sniffs/PHP/NonExecutableCodeSniff.php | 37 ++++--------------- 1 file changed, 7 insertions(+), 30 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php index 7959a95bff..89a56154ed 100644 --- a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php +++ b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php @@ -94,21 +94,13 @@ public function process(File $phpcsFile, $stackPtr) } }//end if - // Check if this token is actually part of a one-line IF or ELSE statement. - for ($i = ($stackPtr - 1); $i > 0; $i--) { - if ($tokens[$i]['code'] === T_CLOSE_PARENTHESIS) { - $i = $tokens[$i]['parenthesis_opener']; - continue; - } else if (isset(Tokens::$emptyTokens[$tokens[$i]['code']]) === true) { - continue; - } - - break; - } - - if ($tokens[$i]['code'] === T_IF - || $tokens[$i]['code'] === T_ELSE - || $tokens[$i]['code'] === T_ELSEIF + // This token may be part of an inline condition. + // If we find a closing parenthesis that belongs to a condition, + // or an "else", we should ignore this token. + if ($tokens[$prev]['code'] === T_ELSE + || (isset($tokens[$prev]['parenthesis_owner']) === true + && ($tokens[$tokens[$prev]['parenthesis_owner']]['code'] === T_IF + || $tokens[$tokens[$prev]['parenthesis_owner']]['code'] === T_ELSEIF)) ) { return; } @@ -176,21 +168,6 @@ public function process(File $phpcsFile, $stackPtr) }//end if }//end if - // This token may be part of an inline condition. - // If we find a closing parenthesis that belongs to a condition - // we should ignore this token. - if (isset($tokens[$prev]['parenthesis_owner']) === true) { - $owner = $tokens[$prev]['parenthesis_owner']; - $ignore = [ - T_IF => true, - T_ELSE => true, - T_ELSEIF => true, - ]; - if (isset($ignore[$tokens[$owner]['code']]) === true) { - return; - } - } - $ourConditions = array_keys($tokens[$stackPtr]['conditions']); if (empty($ourConditions) === false) {