Merge pull request #92 from PHPCSStandards/feature/squiz-nonexecutablecode-various-tweaks

Squiz/NonExecutableCode: simplify + improve handling of comments and closures

Author: jrfnl

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

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) {

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(

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':