Generic/InlineControlStructure: bail early for control structures without body (#880)

rodrigoprimo · web-flow · commit e1b95951f377 · 2025-06-18T00:10:03.000+02:00
* Generic/InlineControlStructure: bail early for control structures without body The sniff now consistently handles all supported control structures without a body by bailing early. Extending the existing behavior for `while` and `for` to also include `do while`, `else`, `elseif`, `if`, and `foreach`. Previously, the sniff would incorrectly flag these empty control structures as inline control structures that needed curly braces. For `else`, `elseif`, `if`, and `foreach`, the fixer would remove the semicolon and add the curly braces. For `do while`, the fixer would add the curly braces and keep the semicolon in between the braces. In all the cases, the resulting code was syntactically correct. Consider the following example: ``` do ; while ($foo < 5); ``` Previously, PHPCS would flag this as an inline control structure and PHPCBF would fix it to: ``` do { ; } while ($foo < 5); ``` Now an empty `do while` is ignored by the sniff (no warnings and no fixes). Here is a link showing that control structures without a body are valid in PHP: https://3v4l.org/slnYL And here is a link showing that the way that they were being fixed by PHPCBF was resulting in valid code (`while` and `for` are not included below as they were already ignored before this commit): https://3v4l.org/8k1N3 * Generic/InlineControlStructure: removed unnecessary code block 29d0be3 changed the sniff to bail early for all control structures without body, so the code will never reach the fixer if `$closer + 1` is `T_SEMICOLON` and thus the removed condition is not necessary anymore. * Generic/InlineControlStructure: removed another unnecessary code block The original version of this part of the code that is now being removed was added in the early days by the commit that enabled this sniff to fix errors: squizlabs/PHP_CodeSniffer@a54c619#diff-4b3945c2100b0a92a56509de1b797bf58ad804cf36233c95c492479b665655dcR148-R154 The only two tests that were added with the commit mentioned above that trigger the removed condition are tests using `while` loops without body: squizlabs/PHP_CodeSniffer@a54c619#diff-116c49a7b0b31f724fc25409e31ba119d7f023146818bcb63edbe8f4071422e2R42-R43 Control structures without a body are the only cases where `$next` would be equal to `$end`. Thus, these are the only cases where the removed condition would be executed. But two previous commits, changed the sniff to bail early and not get to the fixer part when handling control structures without a body. 13c803b changed the sniff to ignore `while`/`for` without a body and updated the existing tests (squizlabs/PHP_CodeSniffer@13c803b#diff-2f069f3fe33bacdfc80485b97303aec66c98c451d07e6d86e41982b81ab1a294L49-R50). 29d0be3 expanded the same approach for `do while`/`else`/`elseif`/`if`/`foreach` control structures. After the removal of the `$next !== $end` check, the `$next` variable became unused allowing for further simplification of the code by removing the place where it was being defined. Note for reviewers: this commit is easier to evaluate when ignoring whitespaces.
diff --git a/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php b/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php
@@ -70,21 +70,25 @@ public function process(File $phpcsFile, $stackPtr)
             }
         }
 
-        if ($tokens[$stackPtr]['code'] === T_WHILE || $tokens[$stackPtr]['code'] === T_FOR) {
-            // This could be from a DO WHILE, which doesn't have an opening brace or a while/for without body.
-            if (isset($tokens[$stackPtr]['parenthesis_closer']) === true) {
-                $afterParensCloser = $phpcsFile->findNext(Tokens::EMPTY_TOKENS, ($tokens[$stackPtr]['parenthesis_closer'] + 1), null, true);
-                if ($afterParensCloser === false) {
-                    // Live coding.
-                    return;
-                }
+        if (isset($tokens[$stackPtr]['parenthesis_closer']) === true) {
+            $nextTokenIndex = ($tokens[$stackPtr]['parenthesis_closer'] + 1);
+        } else if (in_array($tokens[$stackPtr]['code'], [T_ELSE, T_DO], true) === true) {
+            $nextTokenIndex = ($stackPtr + 1);
+        }
 
-                if ($tokens[$afterParensCloser]['code'] === T_SEMICOLON) {
-                    $phpcsFile->recordMetric($stackPtr, 'Control structure defined inline', 'no');
-                    return;
-                }
+        if (isset($nextTokenIndex) === true) {
+            $firstNonEmptyToken = $phpcsFile->findNext(Tokens::EMPTY_TOKENS, $nextTokenIndex, null, true);
+            if ($firstNonEmptyToken === false) {
+                // Live coding.
+                return;
             }
-        }//end if
+
+            if ($tokens[$firstNonEmptyToken]['code'] === T_SEMICOLON) {
+                // This is a control structure without a body. Bow out.
+                $phpcsFile->recordMetric($stackPtr, 'Control structure defined inline', 'no');
+                return;
+            }
+        }
 
         if (isset($tokens[$stackPtr]['parenthesis_opener'], $tokens[$stackPtr]['parenthesis_closer']) === false
             && $tokens[$stackPtr]['code'] !== T_ELSE
@@ -151,9 +155,7 @@ public function process(File $phpcsFile, $stackPtr)
             $closer = $stackPtr;
         }
 
-        if ($tokens[($closer + 1)]['code'] === T_WHITESPACE
-            || $tokens[($closer + 1)]['code'] === T_SEMICOLON
-        ) {
+        if ($tokens[($closer + 1)]['code'] === T_WHITESPACE) {
             $phpcsFile->fixer->addContent($closer, ' {');
         } else {
             $phpcsFile->fixer->addContent($closer, ' { ');
@@ -249,102 +251,69 @@ public function process(File $phpcsFile, $stackPtr)
         }
 
         $nextContent = $phpcsFile->findNext(Tokens::EMPTY_TOKENS, ($end + 1), null, true);
-        if ($nextContent === false || $tokens[$nextContent]['line'] !== $tokens[$end]['line']) {
-            // Looks for completely empty statements.
-            $next = $phpcsFile->findNext(T_WHITESPACE, ($closer + 1), ($end + 1), true);
-        } else {
-            $next    = ($end + 1);
-            $endLine = $end;
-        }
 
-        if ($next !== $end) {
-            if ($nextContent === false || $tokens[$nextContent]['line'] !== $tokens[$end]['line']) {
-                // Account for a comment on the end of the line.
-                for ($endLine = $end; $endLine < $phpcsFile->numTokens; $endLine++) {
-                    if (isset($tokens[($endLine + 1)]) === false
-                        || $tokens[$endLine]['line'] !== $tokens[($endLine + 1)]['line']
-                    ) {
-                        break;
-                    }
-                }
-
-                if (isset(Tokens::COMMENT_TOKENS[$tokens[$endLine]['code']]) === false
-                    && ($tokens[$endLine]['code'] !== T_WHITESPACE
-                    || isset(Tokens::COMMENT_TOKENS[$tokens[($endLine - 1)]['code']]) === false)
-                ) {
-                    $endLine = $end;
-                }
-            }
-
-            if ($endLine !== $end) {
-                $endToken     = $endLine;
-                $addedContent = '';
-            } else {
-                $endToken     = $end;
-                $addedContent = $phpcsFile->eolChar;
-
-                if ($tokens[$end]['code'] !== T_SEMICOLON
-                    && $tokens[$end]['code'] !== T_CLOSE_CURLY_BRACKET
+        if ($nextContent === false || $tokens[$nextContent]['line'] !== $tokens[$end]['line']) {
+            // Account for a comment on the end of the line.
+            for ($endLine = $end; $endLine < $phpcsFile->numTokens; $endLine++) {
+                if (isset($tokens[($endLine + 1)]) === false
+                    || $tokens[$endLine]['line'] !== $tokens[($endLine + 1)]['line']
                 ) {
-                    $phpcsFile->fixer->addContent($end, '; ');
+                    break;
                 }
             }
 
-            $next = $phpcsFile->findNext(T_WHITESPACE, ($endToken + 1), null, true);
-            if ($next !== false
-                && ($tokens[$next]['code'] === T_ELSE
-                || $tokens[$next]['code'] === T_ELSEIF)
+            if (isset(Tokens::COMMENT_TOKENS[$tokens[$endLine]['code']]) === false
+                && ($tokens[$endLine]['code'] !== T_WHITESPACE
+                || isset(Tokens::COMMENT_TOKENS[$tokens[($endLine - 1)]['code']]) === false)
             ) {
-                $phpcsFile->fixer->addContentBefore($next, '} ');
-            } else {
-                $indent = '';
-                for ($first = $stackPtr; $first > 0; $first--) {
-                    if ($tokens[$first]['column'] === 1) {
-                        break;
-                    }
-                }
+                $endLine = $end;
+            }
+        } else {
+            $endLine = $end;
+        }
 
-                if ($tokens[$first]['code'] === T_WHITESPACE) {
-                    $indent = $tokens[$first]['content'];
-                } else if ($tokens[$first]['code'] === T_INLINE_HTML
-                    || $tokens[$first]['code'] === T_OPEN_TAG
-                ) {
-                    $addedContent = '';
-                }
+        if ($endLine !== $end) {
+            $endToken     = $endLine;
+            $addedContent = '';
+        } else {
+            $endToken     = $end;
+            $addedContent = $phpcsFile->eolChar;
 
-                $addedContent .= $indent.'}';
-                if ($next !== false && $tokens[$endToken]['code'] === T_COMMENT) {
-                    $addedContent .= $phpcsFile->eolChar;
-                }
+            if ($tokens[$end]['code'] !== T_SEMICOLON
+                && $tokens[$end]['code'] !== T_CLOSE_CURLY_BRACKET
+            ) {
+                $phpcsFile->fixer->addContent($end, '; ');
+            }
+        }
 
-                $phpcsFile->fixer->addContent($endToken, $addedContent);
-            }//end if
+        $next = $phpcsFile->findNext(T_WHITESPACE, ($endToken + 1), null, true);
+        if ($next !== false
+            && ($tokens[$next]['code'] === T_ELSE
+            || $tokens[$next]['code'] === T_ELSEIF)
+        ) {
+            $phpcsFile->fixer->addContentBefore($next, '} ');
         } else {
-            if ($nextContent === false || $tokens[$nextContent]['line'] !== $tokens[$end]['line']) {
-                // Account for a comment on the end of the line.
-                for ($endLine = $end; $endLine < $phpcsFile->numTokens; $endLine++) {
-                    if (isset($tokens[($endLine + 1)]) === false
-                        || $tokens[$endLine]['line'] !== $tokens[($endLine + 1)]['line']
-                    ) {
-                        break;
-                    }
+            $indent = '';
+            for ($first = $stackPtr; $first > 0; $first--) {
+                if ($tokens[$first]['column'] === 1) {
+                    break;
                 }
+            }
 
-                if ($tokens[$endLine]['code'] !== T_COMMENT
-                    && ($tokens[$endLine]['code'] !== T_WHITESPACE
-                    || $tokens[($endLine - 1)]['code'] !== T_COMMENT)
-                ) {
-                    $endLine = $end;
-                }
+            if ($tokens[$first]['code'] === T_WHITESPACE) {
+                $indent = $tokens[$first]['content'];
+            } else if ($tokens[$first]['code'] === T_INLINE_HTML
+                || $tokens[$first]['code'] === T_OPEN_TAG
+            ) {
+                $addedContent = '';
             }
 
-            if ($endLine !== $end) {
-                $phpcsFile->fixer->replaceToken($end, '');
-                $phpcsFile->fixer->addNewlineBefore($endLine);
-                $phpcsFile->fixer->addContent($endLine, '}');
-            } else {
-                $phpcsFile->fixer->replaceToken($end, '}');
+            $addedContent .= $indent.'}';
+            if ($next !== false && $tokens[$endToken]['code'] === T_COMMENT) {
+                $addedContent .= $phpcsFile->eolChar;
             }
+
+            $phpcsFile->fixer->addContent($endToken, $addedContent);
         }//end if
 
         $phpcsFile->fixer->endChangeset();
diff --git a/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc b/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc
@@ -276,3 +276,24 @@ function testFinally()
 if ($something) {
     echo 'hello';
 } else /* comment */ if ($somethingElse) echo 'hi';
+
+if ($sniffShouldBailEarly);
+
+if (false) {
+} elseif ($sniffShouldBailEarly);
+
+if (false) {
+} else if ($sniffShouldBailEarly);
+
+if (false) {
+} else ($sniffShouldGenerateError);
+
+if (false) {
+} else; // Sniff should bail early.
+
+foreach ($array as $sniffShouldBailEarly);
+
+foreach ($array as $sniffShouldBailEarly)
+	/* some comment */;
+
+do ; while ($sniffShouldBailEarly > 5);
diff --git a/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc.fixed b/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc.fixed
@@ -314,3 +314,25 @@ if ($something) {
     echo 'hello';
 } else /* comment */ if ($somethingElse) { echo 'hi';
 }
+
+if ($sniffShouldBailEarly);
+
+if (false) {
+} elseif ($sniffShouldBailEarly);
+
+if (false) {
+} else if ($sniffShouldBailEarly);
+
+if (false) {
+} else { ($sniffShouldGenerateError);
+}
+
+if (false) {
+} else; // Sniff should bail early.
+
+foreach ($array as $sniffShouldBailEarly);
+
+foreach ($array as $sniffShouldBailEarly)
+	/* some comment */;
+
+do ; while ($sniffShouldBailEarly > 5);
diff --git a/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.php b/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.php
@@ -81,6 +81,7 @@ public function getErrorList($testFile='')
                 260 => 1,
                 269 => 1,
                 278 => 1,
+                289 => 1,
             ];
 
         default:
