Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generic/InlineControlStructure: stop listening for T_SWITCH #595

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Generic/InlineControlStructure: stop listening for T_SWITCH #595

wants to merge 7 commits into from

Conversation

rodrigoprimo
Copy link
Contributor

Description

As originally discussed in #482 (review) (see the "Commit 2 - stop listening for T_SWITCH" section), this PR changes the Generic.ControlStructures.InlineControlStructure sniff to stop listening for T_SWITCH tokens as there is no inline version of a switch in PHP and JS.

Before this change, the sniff would bail early for a T_SWITCH token either because it has a scope opener or because of a syntax error.

Code samples used for the sniff tests that contained a T_SWITCH token were updated to reflect this change. Some of those were actually added as Tokenizer tests before PHPCS had dedicated Tokenizer tests. Below there is more information about each of the modified tests answering the questions left by @jrfnl in #482 (review).

line 82-89: related to squizlabs/PHP_CodeSniffer#497 - looks like this test is about switch and 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 ?

I updated the original code sample used in the sniff test to use T_IF and T_ELSEIF instead of T_SWITCH. I also created a new Tokenizer::recurseScopeMap() test file and added tests to ensure that the scope indexes are set correctly for switches using the normal and alternative syntaxes.

line 93 - 96: also related to squizlabs/PHP_CodeSniffer#497 - looks like this test is about switch and 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 ?

Similar to the above, I updated the code sample to use T_FOREACH instead of T_SWITCH and created dedicated Tokenizer tests to set setting the scope for T_IF with a nested T_SWITCH and T_CASE when T_CASE is missing T_BREAK.

Line 109 - 117: related to 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 ?

I replaced the code sample with an inline T_IF with a closure within the condition. Also, I added a dedicated Tokenizer test to ensure that the scope for a T_SWITCH is set correctly when there is a closure within the condition.

Line 146 - 155: related to 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 switch control 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 ?

I left the code sample as is as I believe it is still valid and the original problem only manifested when the T_IF was nested within a T_SWITCH. A dedicated tokenizer test was added using the same code sample.

Line 204 - 213: related to 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 switch control 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 ?

I replaced the T_SWITCH in the original code sample with a T_FOR and created a dedicated 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 ?

I did replace switch with for. But it is important to note that the original code sample lost part of its value over time as now the sniff bails early if there is no opening parenthesis after the control structure. Ideally, it should be moved to a tokenizer test, but since there are no tests for the JS tokenizer, and support will be dropped in
PHPCS 4.0, I opted to simply use a control structure other than T_SWITCH. This test was originally added in b3f4f83.

Suggested changelog entry

Generic.ControlStructures.InlineControlStructure stop listening for T_SWITCH as there is no inline version of this control structure in PHP and JS

Related issues/external references

First discussed in #482 (review)

Types of changes

  • 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

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • 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).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@rodrigoprimo
Generic/InlineControlStructure: stop listening for T_SWITCH tokens
9dae2f6 
There is no inline version of a `switch` in PHP and JS 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 it to PHPCS:

PHPCSStandards@ad96ebb#diff-e1ea4eabd79d6324057bbf726a27074250478f87d92c11a3725a97f0afbd5513R50

The sniff tests that were using T_SWITCH were changed to use different
control structures that trigger this sniff or were removed when not
necessary anymore.

Some of the sniff tests using the T_SWITCH token were added to
protect against regressions for changes in the tokenizer. For those,
dedicated tokenizer tests will be created in subsequent commits.

The modified JS test lost part of its value over time as now the sniff
bails early if there is no opening parenthesis after the control
structure. Ideally, it should be moved to a tokenizer test, but since
there are no tests for the JS tokenizer and support will be dropped in
PHPCS 4.0, I opted to simply use a control structure other than
T_SWITCH. This test was originally added in b3f4f83.

References:

- PHP: https://www.php.net/manual/en/control-structures.switch.php
- JS: https://tc39.es/ecma262/multipage/ecmascript-language-statements-and-declarations.html
@rodrigoprimo
Tests/Tokenizer: tests to ensure scope is set correctly for T_SWITCH
6aa3382 
This commit adds tests to safeguard against regressions related to
b24b96b which was added to fix one of the issues in
squizlabs/PHP_CodeSniffer#497. b24b96b
originally added a test to a sniff test
(InlineControlStructureUnitTest.php) as back then there were no
Tokenizer tests. This test was added to ensure that
Tokenizer::recurseScopeMap() would correctly add scope information to
the T_SWITCH token when handling a `switch` that uses the alternative
syntax.

Now that the InlineControlStructure sniff doesn't listen for the
T_SWITCH token anymore, the original test added in b24b96b became
useless and thus was refactored in a previous commit to use different
control structure.
@rodrigoprimo
Tests/Tokenizer: tests for T_CASE and T_IF scope condition
770183f 
This commit adds tests to safeguard against regressions related to
fddc61a which is part of
squizlabs/PHP_CodeSniffer#497. fddc61a
originally added a test to a sniff test
(InlineControlStructureUnitTest.php) as back then there were no
Tokenizer tests. Those tests were added to ensure that
Tokenizer::recurseScopeMap() would correctly add scope information to
the `T_CASE` token when it shares the closer with `T_SWITCH` and to
`T_IF` when a nested `T_CASE` shares the scope closer with `T_SWITCH`.

Now that the InlineControlStructure sniff doesn't listen for the
T_SWITCH token anymore, the original test added in fddc61a became
useless and thus was refactored in a previous commit to use different
control structure.
@rodrigoprimo
Tests/Tokenizer: scope opener is set correctly for T_SWITCH
f1049ac 
This commit adds a test to ensure that the scope opener is set correctly
for T_SWITCH when there is a closure in the condition. This should
safeguard against regressions related to 30c618e which fixed
squizlabs/PHP_CodeSniffer#543. 30c618e originally
added a test to InlineControlStructureUnitTest.php as back then there were
no Tokenizer tests.

Now that the InlineControlStructure sniff doesn't listen for the
T_SWITCH token anymore, the original test added in 30c618e became
useless and was removed in a previous commit.
@rodrigoprimo
Tests/Tokenizer: expand recurseScopeMap() tests for the case keyword
892df41 
This commit expands the existing `Tokenizer::recurseScopeMap()` tests for the
case keyword when used in a switch statement to check the value of the
`scope_condition`, `scope_opener` and `scope_closer` indexes besides
checking if they are set. This will be needed for a new test case that
will be added in a subsequent commit.
@rodrigoprimo
Tests/Tokenizer: test to ensure scope closer is set correctly for T_CASE
09305da 
This commit copies a sniff test from InlineControlStructureUnitTest.php
to the Tokenizer::recurseScopeMap() tests for the case keyword. This
test was added in b498dbe before the Tokenizer tests were created. It
ensures that the scope for the T_CASE token is set correctly when there
is a if/elseif/else with and without braces inside it.

Note that this test currently does not fail even when the changes to the
tokenizer applied in b498dbe are reverted. I'm assuming that a
subsequent commit further improved the tokenizer and made the changes
from b498dbe redundant, but I was not able to find the specific commit
using git bissect. That being said, I was able to confirm that before
b498dbe, the scope closer for the T_CASE token was incorrectly set to
the T_RETURN token instead of T_BREAK.
@rodrigoprimo
Tests/Tokenizer: test related to setting the scope for T_CASE
91c66d8 
This commit copies a sniff test from InlineControlStructureUnitTest.php
to the Tokenizer::recurseScopeMap() tests for the case keyword. This
test was added in 65ef053 before the Tokenizer tests were created. It
ensures that the scope for the T_CASE token is set correctly when there
is a inline control structure inside it with more than three lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Standard: Generic Status: has merge conflict
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@rodrigoprimo