From 29d0be3ecbbf9975614d5da009f463a7eee804ed Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Fri, 14 Mar 2025 09:12:06 -0300 Subject: [PATCH 1/3] 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 --- .../InlineControlStructureSniff.php | 30 +++++++++++-------- .../InlineControlStructureUnitTest.1.inc | 21 +++++++++++++ ...InlineControlStructureUnitTest.1.inc.fixed | 22 ++++++++++++++ .../InlineControlStructureUnitTest.php | 1 + 4 files changed, 61 insertions(+), 13 deletions(-) diff --git a/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php b/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php index 44dbfbf86b..c551413ac0 100644 --- 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 diff --git a/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc b/src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc index ada26fb7ff..1b02430859 100644 --- 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 index 53c6f0952c..ad03ad626f 100644 --- 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 index 6e8ece960f..e8aa91818a 100644 --- 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: From d2039eb74549c5ab104ad61b10d9e5f8984783f6 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Fri, 14 Mar 2025 09:14:50 -0300 Subject: [PATCH 2/3] Generic/InlineControlStructure: removed unnecessary code block 29d0be3e 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. --- .../Sniffs/ControlStructures/InlineControlStructureSniff.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php b/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php index c551413ac0..bd4114c276 100644 --- a/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php +++ b/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php @@ -155,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, ' { '); From 17601e27d41a5da1489b8d965aaf9be8efc1802f Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Fri, 14 Mar 2025 10:03:16 -0300 Subject: [PATCH 3/3] 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: https://github.com/squizlabs/PHP_CodeSniffer/commit/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: https://github.com/squizlabs/PHP_CodeSniffer/commit/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 (https://github.com/squizlabs/PHP_CodeSniffer/commit/13c803b#diff-2f069f3fe33bacdfc80485b97303aec66c98c451d07e6d86e41982b81ab1a294L49-R50). 29d0be3e 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. --- .../InlineControlStructureSniff.php | 129 +++++++----------- 1 file changed, 48 insertions(+), 81 deletions(-) diff --git a/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php b/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php index bd4114c276..63624a2e28 100644 --- a/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php +++ b/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php @@ -251,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();