Generic/InlineControlStructure: bail early for control structures without body

[rodrigoprimo]

Description

As discussed in https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/482#pullrequestreview-2062765716 (see "Commit 6" and "Commit 7" sections), Generic.ControlStructures.InlineControlStructure now consistently handles all control structures without a body by bailing early. This PR extends the existing behavior for while and for to also include do while, else, elseif, if, and foreach. Note that in #482, I did not include do while, but I believe it should be included, and it is now part of this PR.

Previously, the sniff would incorrectly flag these empty control structures as inline control structures that needed curly braces. This change makes the behavior consistent across all control structures. 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 PHPCBF was fixing them was resulting in valid code (while and for are not included below as they were already ignored before this commit):

https://3v4l.org/8k1N3

Additionally, this PR removes two code blocks from the fixer that became unnecessary. They are removed in separate commits to hopefully make it easier to review this PR. I suggest combining all the commits in this PR into a single commit before merging.

First removed block

The sniff now bails early for all control structures without body, so the code will never reach the fixer if $closer + 1 is T_SEMICOLON.

Second removed block

The original version of this now removed condition 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).
• d4e5d28 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.

Regarding what was commented in https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/482#pullrequestreview-2062765716:

Looking at this more closely, I think this needs further investigation before this commit could be considered. While looking at this in combination with the review for commit 3, I started wondering about line 280. I have a niggly feeling that that findNext() could return false and needs a test + safeguard. As things are, without digging deeper into the potentially problematic result of line 280, the code snippet you are proposing to remove, cannot be removed, as if line 280 would return false, the if ($next !== $end) condition could be a false !== $end, which means that the code snippet being removed would be hit and the fixer would end up creating a parse error in the file under scan.

I was not able to create a test case that would make findNext() return false on that particular line. Please let me know if you can. Despite that, after further checking the changes I proposed on the original commit back in #482, I noticed that the variable $next was not used anymore and could be removed. So, in the changes I'm proposing here, the findNext() call has also been removed.

Note for reviewers: the third commit is easier to evaluate when ignoring whitespace.

Suggested changelog entry

Generic.ControlStructures.InlineControlStructure: bail early when encountering do while, else, elseif, if, and foreach control structures without a body.

Related issues/external references

Initially suggested in #482.

Types of changes

I'm not sure if this should be considered a bug fix or an enhancement. So, I'm not selecting the type of change.

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] This change is only breaking for integrators, not for external standards or end-users.
• [ ] Documentation improvement

PR checklist

• [x] I have checked there is no other PR open for the same change.
• [x] I have read the Contribution Guidelines.
• [x] I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• [x] I have added tests to cover my changes.
• [x] I have verified that the code complies with the projects coding standards.
• [ ] [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

We discussed this PR in a call and as PHPCS 4.0 is coming closer and will drop JS support, it might well be better to leave this PR until the renewed 4.0 branch is public (and rebase the PR on that). Should also make reviewing easier as it reduces cognitive load as JS would no longer need to be considered.

[rodrigoprimo]

Just documenting here that since this PR will be reviewed after the release of PHPCS 4.0, I went ahead and updated its commits and PR description to consider only PHP. Before the changes also included tests for JS.

I also improved the PR description by adding links to 3v4l.org showing that empty control structures are valid in PHP and that the way that PHPCBF was fixing those empty control structures is also valid (thus PHPCBF was not introducing syntax errors in the fixed files).

[jrfnl]

@rodrigoprimo As the 4.x branch is now available, want to rebase the PR on that and change the branch against which this PR has been pulled ?

[rodrigoprimo]

@jrfnl, done. This PR is now ready to be reviewed. Thanks.