Generic/InlineControlStructure: stop listening for T_SWITCH #595

rodrigoprimo · 2024-08-13T20:49:27Z

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 check 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 because I believe it is still valid. 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 · 2024-10-09T20:35:16Z

@jrfnl, as we discussed last week, I reorganized the commits in this PR so that the changes to the sniff tests are not all in the commit that changes the InlineControlStructure sniff to stop listening for T_SWITCH. Instead, each change to the sniff tests is in the commit that creates the related tokenizer test.

This PR is now ready to be reviewed.

jrfnl

@rodrigoprimo Thanks for this PR and sorry it took me so long to get back to this one.

I've gone through the code now and left some inline remarks.
Not everything needs to be addressed in this PR. It is perfectly fine to open an issue that a follow-up will be needed to the PR at a later point in time to further stabilize and expand the Tokenizer tests.

All in all, looking good and thank you so much for reorganizing the commits. Made it much more straight forward to review now.

src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc

tests/Core/Tokenizers/Tokenizer/RecurseScopeMapSwitchTokenScopeTest.php

jrfnl · 2024-12-16T15:46:29Z

src/Standards/Generic/Tests/ControlStructures/InlineControlStructureUnitTest.1.inc

+if ($value):
+    if ($anotherValue):
+        foreach ($array as $element):
+            echo $element;


The original test has a T_COLON associated with the case in the statement between the alternative syntaxes. I'm not sure if that was important for the test or not, but might be worth it to have a statement on this line which also has an unrelated (non-alternative syntax) T_COLON. What do you think ?

That makes sense, I added the following: echo !is_null($element) ? $element : 'default';

Ah, but the : in that statement would be tokenized as T_INLINE_ELSE, not as T_COLON. Want to have another look at this ?

I misunderstood your suggestion during our call and should have read the comment more carefully.

I just pushed a new commit updating the code to use a T_COLON part of a function's return type declaration:

echo (function($element): string { return $element; } )($element);

The code is a bit weird in that it could be simplified to just echo $element, but that is probably ok for this test as it ensures there is a T_COLON token not related to an inline control structure alternative syntax.

Convoluted code samples are a good thing™ in the context of tests for PHPCS, so 👍🏻

jrfnl · 2024-12-16T15:48:10Z

tests/Core/Tokenizers/Tokenizer/RecurseScopeMapCaseKeywordConditionsTest.inc

@@ -93,3 +93,10 @@ enum Foo: string {
    /* testKeywordAsEnumCaseNameShouldBeString7 */
    case ARRAY = 'array';
 }
+
+// Test for https://github.com/squizlabs/PHP_CodeSniffer/issues/497


Would it be worth it to also have a variation of this test in the RecurseScopeMapDefaultKeywordConditionsTest ?

jrfnl · 2024-12-16T16:02:57Z

tests/Core/Tokenizers/Tokenizer/RecurseScopeMapSwitchTokenScopeTest.inc

+
+// Test for https://github.com/squizlabs/PHP_CodeSniffer/issues/543
+/* testSwitchClosureWithinCondition */
+switch((function () {


The test could be made even more "interesting" if the closure had a return type declaration (which also uses the T_COLON token).

Might actually even be better to have a second test sample for that.

On that note, maybe we should also have a test with an arrow function callback with/without return type declaration in a switch condition ?

Note: the above remarks do not need to be addressed in this PR. They can be left for a follow-up PR to further improve the tokenizer tests.

tests/Core/Tokenizers/Tokenizer/RecurseScopeMapSwitchTokenScopeTest.inc

jrfnl · 2024-12-16T16:25:52Z

tests/Core/Tokenizers/Tokenizer/RecurseScopeMapCaseKeywordConditionsTest.inc

Should this improvement also be done RecurseScopeMapDefaultKeywordConditionsTest tests ?

jrfnl · 2024-12-16T16:33:40Z

tests/Core/Tokenizers/Tokenizer/RecurseScopeMapCaseKeywordConditionsTest.inc

@@ -103,3 +103,17 @@ switch ($value):
    case 1:
        echo 'one';
 endswitch;
+
+// Test for https://github.com/squizlabs/PHP_CodeSniffer/issues/879


Would it make sense to have a similar test to this one in the RecurseScopeMapDefaultKeywordConditionsTest test ?

rodrigoprimo · 2024-12-17T19:46:29Z

Thanks for your review, @jrfnl!

I answered all the inline comments I believe should be addressed in this PR and pushed new commits. As you suggested, I plan to create a follow-up PR to address the comments that were not blocking this PR and that I didn't answer. If I cannot create the follow-up PR right after this one is merged, I will create an issue documenting what needs to be done.

I split the new changes per related original commit, hoping this will help you combine the commits before merging. Please let me know if this approach makes sense or not.

This PR is ready for another review.

jrfnl · 2024-12-19T22:58:17Z

As you suggested, I plan to create a follow-up PR to address the comments that were not blocking this PR and that I didn't answer. If I cannot create the follow-up PR right after this one is merged, I will create an issue documenting what needs to be done.

Thanks.

I split the new changes per related original commit, hoping this will help you combine the commits before merging. Please let me know if this approach makes sense or not.

It does 👍🏻

This PR is ready for another review.

@rodrigoprimo Just one comment left to address: #595 (comment)

rodrigoprimo · 2024-12-20T14:23:44Z

@rodrigoprimo Just one comment left to address: #595 (comment)

@jrfnl, I just pushed a new commit addressing this comment.

jrfnl

Thanks @rodrigoprimo ! All good and very happy to get this one merged and out of the way.

I'm going to rebase & squash a few commits now. Merging once the build has passed (again).

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@ad96eb#diff-e1ea4eabd79d6324057bbf726a27074250478f87d92c11a3725a97f0afbd5513R50 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. Those commits will also update the related sniff tests to use different control structures other than `T_SWITCH` when appropriate. 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

This commit copies, with some non-structural modifications, a sniff test to the tokenizer tests. The original test was added in b24b96b, part of squizlabs/PHP_CodeSniffer#497. b24b96b introduced a test to InlineControlStructureUnitTest.php as there were no Tokenizer tests back then. It 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. This commit also adds a test to ensure the scope is correctly set when using the normal switch syntax. This was not part of b24b96b, but it is relevant anyway, as no other tokenizer test covers this part. Since the InlineControlStructure sniff doesn't listen for the `T_SWITCH` token anymore (see baa4f65), the original test added in b24b96b became useless and thus was refactored to use a different control structure. The sniff test was kept as testing code that uses a control structure, and opening and closing PHP tags is an interesting case not covered in other tests.

This commit copies, with some non-structural modifications, a sniff test to the tokenizer tests. Two new tokenizer tests are created as the result of this copy. One to ensure that the scope of `T_CASE` is correctly set when the case shares the scope closer with `T_SWITCH` (no `T_BREAK`). And another to ensure that the scope of `T_IF` is correctly set when there is a nested `T_SWITCH` and `T_CASE` without a `T_BREAK`. The original sniff test was added in fddc61a, which is part of squizlabs/PHP_CodeSniffer#497. fddc61a introduced a test to InlineControlStructureUnitTest.php as there were no Tokenizer tests back then. The test was added to ensure that Tokenizer::recurseScopeMap() correctly adds 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`. Since the InlineControlStructure sniff doesn't listen for the `T_SWITCH` token anymore (see baa4f65), the original test added in fddc61a became useless and thus was refactored to use a different control structure. This new version of the sniff test was kept as testing code that uses nested alternative syntax control structures is still valid and is not covered in other tests.

This commit copies, with some modifications, a sniff test to the tokenizer tests. It ensures that the scope opener is set correctly for T_SWITCH when there is a closure in the condition. This safeguards against regressions related to 30c618e, which fixed https://github .com/squizlabs/PHP_CodeSniffer/issues/543. 30c618e originally added a test to InlineControlStructureUnitTest.php as there were no Tokenizer tests back then. Since the InlineControlStructure sniff doesn't listen for the `T_SWITCH` token anymore (see baa4f65), the original test added in 30c618e became useless and thus was refactored to use a different control structure. This new version of the sniff test was kept as testing code that uses a closure in the condition is still an interesting case and is not covered in other tests.

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 is needed for a new test case that will be added in a subsequent commit.

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 correctly set when there is an 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 `T_RETURN` instead of `T_BREAK`. The original sniff test was kept without any modification as testing if/elseif/else with and without curly braces inside another control structure is still valid.

This commit copies a sniff test from InlineControlStructureUnitTest.php to the Tokenizer::recurseScopeMap() tests for the `case` keyword. The copied test was added in 65ef053 before the Tokenizer tests were created. It ensures that the scope for the `T_CASE` token is correctly set when there is an inline control structure inside it with more than three lines. The original sniff test was modified to use `T_FOR` instead of `T_SWITCH`, but its purpose was not altered. The test is about adding braces to an `if` that returns a nested function call.

github-actions bot added Standard: Generic Status: triage labels Aug 13, 2024

rodrigoprimo removed the Status: triage label Aug 13, 2024

github-actions bot added the Status: has merge conflict label Sep 30, 2024

rodrigoprimo force-pushed the inline-control-strutucture-remove-switch branch 4 times, most recently from b76eb2b to 8211f0d Compare October 9, 2024 14:42

github-actions bot removed the Status: has merge conflict label Oct 9, 2024

rodrigoprimo force-pushed the inline-control-strutucture-remove-switch branch 5 times, most recently from e984a85 to 356f8bf Compare October 9, 2024 20:23

github-actions bot added the Status: has merge conflict label Oct 25, 2024

rodrigoprimo force-pushed the inline-control-strutucture-remove-switch branch from 356f8bf to 214372e Compare October 25, 2024 21:57

github-actions bot removed the Status: has merge conflict label Oct 25, 2024

rodrigoprimo force-pushed the inline-control-strutucture-remove-switch branch 2 times, most recently from 914faed to cbfa258 Compare October 28, 2024 12:22

jrfnl force-pushed the master branch from e06559a to aab0862 Compare December 7, 2024 10:54

jrfnl reviewed Dec 16, 2024

View reviewed changes

jrfnl added the Status: awaiting feedback label Dec 16, 2024

rodrigoprimo removed the Status: awaiting feedback label Dec 17, 2024

jrfnl added the Type: chores/QA label Dec 19, 2024

jrfnl added the Status: awaiting feedback label Dec 19, 2024

rodrigoprimo removed the Status: awaiting feedback label Dec 20, 2024

jrfnl approved these changes Dec 20, 2024

View reviewed changes

jrfnl added this to the 3.11.3 milestone Dec 20, 2024

rodrigoprimo added 7 commits December 20, 2024 22:39

jrfnl force-pushed the inline-control-strutucture-remove-switch branch from 363729b to 105b4be Compare December 20, 2024 21:48

jrfnl merged commit b35dba4 into PHPCSStandards:master Dec 20, 2024
45 checks passed

jrfnl deleted the inline-control-strutucture-remove-switch branch December 20, 2024 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic/InlineControlStructure: stop listening for T_SWITCH #595

Generic/InlineControlStructure: stop listening for T_SWITCH #595

rodrigoprimo commented Aug 13, 2024 •

edited

Loading

rodrigoprimo commented Oct 9, 2024

jrfnl left a comment

jrfnl Dec 16, 2024

rodrigoprimo Dec 17, 2024

jrfnl Dec 19, 2024

rodrigoprimo Dec 20, 2024

jrfnl Dec 20, 2024

jrfnl Dec 16, 2024

jrfnl Dec 16, 2024

jrfnl Dec 16, 2024

jrfnl Dec 16, 2024

rodrigoprimo commented Dec 17, 2024 •

edited

Loading

jrfnl commented Dec 19, 2024

rodrigoprimo commented Dec 20, 2024

jrfnl left a comment

Generic/InlineControlStructure: stop listening for T_SWITCH #595

Generic/InlineControlStructure: stop listening for T_SWITCH #595

Conversation

rodrigoprimo commented Aug 13, 2024 • edited Loading

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

rodrigoprimo commented Oct 9, 2024

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo commented Dec 17, 2024 • edited Loading

jrfnl commented Dec 19, 2024

rodrigoprimo commented Dec 20, 2024

jrfnl left a comment

Choose a reason for hiding this comment

rodrigoprimo commented Aug 13, 2024 •

edited

Loading

rodrigoprimo commented Dec 17, 2024 •

edited

Loading