Generic/InlineControlStructure: fix two bugs and improve code coverage #482
Conversation
This commit fixes a bug in this sniff where it would not handle correctly `elseif` statements when there is a comment between the `else` and `if` tokens. The affected code was added in squizlabs/PHP_CodeSniffer@18f98cc to fix a similar bug when there is more than one whitespace between `else` and `if`, but it failed to consider that there might be comment tokens between `else` and `if`.
There is no inline version of a `switch` in PHP so there is no reason for this sniff to listen to `T_SWITCH` tokens. Before this change, the sniff would bail early on the line below as a switch always has a scope opener, so this change should not impact in any way the behavior of the sniff. https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/9a0c2546ea2fa7aac19881da7b655cc5f022bc10/src/Standards/Generic/Sniffs/ControlStructures/InlineControlStructureSniff.php#L71 The InlineControlStructure sniff started listening for T_SWITCH tokens since the commit that added this sniff to PHPCS: PHPCSStandards@ad96ebb#diff-e1ea4eabd79d6324057bbf726a27074250478f87d92c11a3725a97f0afbd5513R50
The removed condition was added in a5d3f14. The tests added in this commit still pass without the removed condition. The condition is unreachable because `$end` can never be equal to `$phpcsFile->numTokens`. It will always be less than `$phpcsFile->numTokens`. `$end` represents the index of a token in the tokens array. The indexes of the tokens array are zero-based, and the `$phpcsFile->numTokens` property is one-based. So even if `$end` is pointing to the index of the last token, it will be less than `$phpcsFile->numTokens`. `$end` is first set as `$end = ($closer + 1)`. In theory, if `$closer` is the index of the last token, `$end` could be equal to `$phpcsFile->numTokens`. But that is a situation that will never occur as it will represent a syntax error, and the sniff would have bailed already.
Doing this to be able to create tests with syntax errors on separate files.
This commit removes a redundant condition that was added in fbea319 to support JS braceless do-while loops. A subsequent commit added similar code to support braceless do-while loops (plus for/while loops without body) for PHP, but it also works for JS (13c803b). There are a few syntax error cases that were handled by the code that is removed by this commit and are not handled by the code introduced in 13c803b. Without the removed code, they are now handled in a if condition right below. I added two tests with those cases to ensure the sniff continues working as expected.
…ch without body In PHP, `elseif`, `if` and `foreach` can be declared without a body. The sniff was improperly flagging those as inline control structures. This commit changes that, and now the sniff does not trigger an error anymore for elseif/if/foreach without a body. This is consistent with the behavior of the sniff for `while` and `for` statements without a body. It is likely that this problem was not found before because it is hard to imagine a case in which an `elseif`, `if` or `foreach` without a body would be useful.
This commit removes an unreachable condition in the fixer part of the sniff. The original version of this condition was added in the early days by the commit that enabled this sniff to fix errors: squizlabs/PHP_CodeSniffer@a54c619#diff-4b3945c2100b0a92a56509de1b797bf58ad804cf36233c95c492479b665655dcL108-L110 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-4b3945c2100b0a92a56509de1b797bf58ad804cf36233c95c492479b665655dcL108-L110 I believe 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 subsequent 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-R51 - f4afa10 expanded the same approach to also include elseif/if/foreach control structures Those changes rendered the condition that is removed by this commit unreachable.
There was a problem hiding this comment.
Hi @rodrigoprimo, thank you for yet another PR in this series.
As this PR contains lots of different decision points, it would be better if this would be a series of (chained) PRs, where each of those decisions could be discussed in isolation.
I imagine this PR chain could look something like this:
- PR 1 - commit 1
- PR 2 - commit 2
- PR 3 - commit 6 + 7
- PR 4 - commit 3 + 4 + 5 + 8
Either way, I've reviewed the PR as-is now, going through each commit individually and evaluating each commit on its own merits.
Commit 1 - error when handling else if
✅
Commit 2 - stop listening for T_SWITCH
✔️ on the principle of the change, but not on the execution.
While the commit message only mentions PHP, the sniff also checks javascript, so the question needs to be asked whether the conclusion - "switch does not support use without curly braces" - is also valid for JS.
I've now verified this via the specs: https://tc39.es/ecma262/multipage/ecmascript-language-statements-and-declarations.html, but this should have been verified when creating the PR/commit.
For the record, a link to the PHP manual about switch would also have been appropriate here: https://www.php.net/manual/en/control-structures.switch.php
Now, while the token is being removed, the tests still contain quite some code samples with switch. These may now no longer be testing anything, while some contain syntax sequences which would be good to have tests for.
Test case file *.1.inc:
- line 82-89: related to Generic InlineControlStructureSniff does not support alternative SWITCH syntax squizlabs/PHP_CodeSniffer#497 - looks like this test is about
switchand the alternative syntax, which is no longer relevant in this sniff, but the use of... ?> ... <?php ...within a control structure is an interesting case. Can you think of a code sample which would maintain the value of this test, but uses one of the other control structures ?
Also: should the original test - which was really a tokenizer bug - be moved to a Tokenizer test ? - line 93 - 96: also related to Generic InlineControlStructureSniff does not support alternative SWITCH syntax squizlabs/PHP_CodeSniffer#497 - looks like this test is about
switchand the alternative syntax, which is no longer relevant in this sniff, but the use of nested alternative syntax control structures could still be a valid test case. Can you think of a code sample which would maintain the value of this test, but uses one of the other control structures ?
Also: should the original test - which was really a tokenizer bug - be moved to a Tokenizer test ? - Line 109 - 117: related to SWITCH with closure in condition generates inline control structure error squizlabs/PHP_CodeSniffer#543 - looks like this test is about a closure being used within the condition of a
switch, which is no longer relevant in this sniff, but the use of the closure in the condition is still an interesting case. Can you think of a code sample which would maintain the value of this test, but uses one of the other control structures (without curlies) ?
Also: should the original test - which was really a tokenizer bug - be moved to a Tokenizer test ? - Line 146 - 155: related to Generic InlineControlStructureSniff can create parse error when case/if/elseif/else have mixed brace and braceless definitions squizlabs/PHP_CodeSniffer#879 - looks like this test is about mixing if/elseif/else with braces and without braces in one code sample. I imagine, this test is still valid as-is, or maybe the
switchcontrol structure wrapper should be replaced with another control structure wrapper ?
Also: should the original test - which was, in part, a tokenizer bug - be moved to a Tokenizer test ? - Line 204 - 213: related to InlineControlStructure CBF issue while adding braces to an if thats returning a nested function squizlabs/PHP_CodeSniffer#1590 - looks like this test is about adding braces to an if that's returning a nested function call. I imagine, this test is still valid as-is, or maybe the
switchcontrol structure wrapper should be replaced with another control structure wrapper ?
Also: should the original test - which was, in part, a tokenizer bug - be moved to a Tokenizer test ?
Test case file *.js:
- line 23 - can a meaningful replacement test be created which would use the keyword of one of the other control structures ?
Commit 3 - remove unreachable condition
❌
The condition is unreachable because
$endcan never be equal to$phpcsFile->numTokens. It will always be less than$phpcsFile->numTokens.$endrepresents the index of a token in the tokens array. The indexes of the tokens array are zero-based, and the$phpcsFile->numTokensproperty is one-based. So even if$endis pointing to the index of the last token, it will be less than$phpcsFile->numTokens.
$endis first set as$end = ($closer + 1). In theory, if$closeris the index of the last token,$endcould be equal to$phpcsFile->numTokens. But that is a situation that will never occur as it will represent a syntax error, and the sniff would have bailed already.
Would have been nice to have an indication of where the sniff would bail early as the for loop setting $end doesn't contain a return statement, so, based on that code alone, it is perfectly possible for $end === $phpcs->numTokens to be true.
With this in mind, I've now investigated this myself and find that the above statement is just not true. The $end === $phpcs->numTokens is definitely reachable and removing it introduces a bug in the sniff.
This commit should be dropped and a new test case file should be added with the following code (note: NO new line or semicolon at the end of the file!):
<?php
// Live coding. Should still trigger the error.
// This test should explicitly NOT contain a new line at the end of the file.
if ($bar)
doSomething()Commit 4 - test case file rename
☑️
Commit 5 - remove redundant condition
✔️
While I have verified the claim made in the commit message and found it correct, I haven't dug in deeper to see if anything else could break.
The reason for this is two-fold:
- This is JS specific code and using PHPCS with JS is already discouraged and support will be removed in the near future.
- The code snippet being removed looks to be unstable anyway as the unlimited
find*()calls can walk way too far.
Commit 6 - bug when handling * without body
❓
😁 Actually, I did consider that case when creating squizlabs/PHP_CodeSniffer#2827 - see: squizlabs/PHP_CodeSniffer#2827 (comment). At the time, we decided not to handle it as none of those code samples make for useful/effective code. That kind of code would always effectively be redundant code.
If that type of code would now be handled by the sniff, let's do it correctly. Even an else without a body is valid: https://3v4l.org/W2Bao, so excluding else from this check is incorrect.
Also note that as commit 5 removes the JS specific block, this change will now also affect JS code, so it will need tests in the JS file and it should be verified if the change is even valid for JS.
Other notes:
As you intend for that code sample to now run for nearly all tokens this sniff listens for, the new condition can be simplified to:
-if (in_array($tokens[$stackPtr]['code'], [T_ELSEIF, T_IF, T_FOR, T_FOREACH, T_WHILE], true) === true) {
+if ($tokens[$stackPtr]['code'] !== T_DO) {I also think the condition on line 181 can now no longer be true, so is redundant ?
Commit 7 - remove redundant condition
❓
If accepted, this commit should be included in commit 6, as that is the commit which makes the code redundant. It wasn't redundant before, it was only missing tests to trigger the code.
Note for future reviews: this commit is easier to evaluate when ignoring whitespace.
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.
Commit 8 - improve code coverage
What with some of the above commit being pulled into question, this commit cannot be verified properly until the other questions have been resolved and the code has been updated to reflect this.
|
Closing this PR. As discussed via chat, I will open smaller PRs following @jrfnl suggestion:
|
|
@rodrigoprimo The first follow-up PR, cherrypicked out of this PR was pulled and merged a while ago. What is the status of the rest ? Note: it's fine if you want to divert this for later, but in that case, let's open a reminder ticket to make sure this is not overlooked as this PR is closed. |
|
@jrfnl, I started working on the issues related to commit 2, but I paused because I wasn't sure about the changes that I was making to the tokenizer tests. I plan to discuss this with you in our pairing session tomorrow. I can create an issue (or a few issues) for the items that are still pending after our conversation tomorrow. |
@rodrigoprimo That sounds like a good idea as it will allow us to track what still needs doing more easily (at least more easily than to remember to look at a closed issue) |
Description
This PR makes a few changes to the Generic.ControlStructures.InlineControlStructure sniff:
else ifstatements (daa1940) and another one when handlingelseif/if/foreachwithout a body (716cbf6).T_SWITCHfrom the list of tokens that this sniff listens to (3db1049).While working on this PR, I noticed that the sniff does not handle well a few syntax errors related to the
do-whilecontrol structure. I will open a separate issue describing what I found.Suggested changelog entry
Generic.ControlStructures.InlineControlStructure: properly handle
else ifstatements where there is a comment betweenelseandifGeneric.ControlStructures.InlineControlStructure: fix false positives when handling
elseif,if, andforeachwithout a bodyRelated issues/external references
Part of #146
Types of changes
PR checklist