From b38787574a7ba921c6b79a4b1d8e00d69addb6b2 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. --- CHANGELOG.md | 2 ++ .../Squiz/Sniffs/PHP/NonExecutableCodeSniff.php | 4 ++-- .../Tests/PHP/NonExecutableCodeUnitTest.1.inc | 17 +++++++++++++++++ .../Tests/PHP/NonExecutableCodeUnitTest.php | 2 ++ 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b981ccf6d..6f0c5c5deb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -175,6 +175,8 @@ The file documents changes to the PHP_CodeSniffer project. - Thanks to @simonsan for the patch - Fixed bug #3893 : Generic/DocComment : the SpacingAfterTagGroup fixer could accidentally remove ignore annotations - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch +- Fixed bug #3898 : Squiz/NonExecutableCode : the sniff could get confused over comments in unexpected places + - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Fixed bug #3904 : Squiz/FunctionSpacing : prevent potential fixer conflict - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Fixed bug #3906 : Tokenizer/CSS: fixed a bug related to the unsupported slash comment syntax diff --git a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php index 7bcb89dcfe..202c338ebe 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 9097c36196..25cc7211cb 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, ]; case 'NonExecutableCodeUnitTest.2.inc': From fb9447fe08b5942144c0aac46e6670ad8fccd65f 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. --- CHANGELOG.md | 2 ++ src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php | 4 +++- .../Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc | 6 ++++++ src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php | 1 + 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f0c5c5deb..986dfa413f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,8 @@ The file documents changes to the PHP_CodeSniffer project. - Thanks to Dan Wallis (@fredden) for the patch - Squiz.PHP.InnerFunctions sniff no longer reports on OO methods for OO structures declared within a function or closure - Thanks to @Daimona for the patch +- Squiz.PHP.NonExecutableCode will now also flag redundant return statements just before a closure close brace + - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Runtime performance improvement for PHPCS CLI users. The improvement should be most noticeable for users on Windows. - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - The -e (explain) command will now list sniffs in natural order diff --git a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php index 202c338ebe..74e00eab47 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 25cc7211cb..30ccad4d26 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, ]; case 'NonExecutableCodeUnitTest.2.inc': From 632600bc8287c16029d09c29f35e0d742d42fa45 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 74e00eab47..f4bffff75a 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) {