diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b981ccf6d..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 @@ -175,6 +177,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..f4bffff75a 100644 --- a/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php +++ b/src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php @@ -94,35 +94,29 @@ 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; } 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 // 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; @@ -174,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) { diff --git a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc index 051b6c6b11..2efcc78e57 100644 --- a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.1.inc @@ -396,5 +396,28 @@ function executionStoppingThrowExpressionsE() { echo 'non-executable'; } +function returnNotRequiredIgnoreCommentsA() +{ + if ($something === TRUE) { + return /*comment*/; + } + + echo 'foo'; + return /*comment*/; +} + +function returnNotRequiredIgnoreCommentsB() +{ + echo 'foo'; + return; + /*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 9097c36196..30ccad4d26 100644 --- a/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php +++ b/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php @@ -82,6 +82,9 @@ public function getWarningList($testFile='') 386 => 1, 391 => 1, 396 => 1, + 406 => 1, + 412 => 1, + 419 => 1, ]; case 'NonExecutableCodeUnitTest.2.inc':