search

PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
937 stars 56 forks source link

Generic/InlineControlStructure: stop listening for T_SWITCH #595

Open rodrigoprimo opened 2 months ago

rodrigoprimo commented 2 months ago

Description

As originally discussed in https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/482#pullrequestreview-2062765716 (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 https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/482#pullrequestreview-2062765716.

line 82-89: related to https://github.com/squizlabs/PHP_CodeSniffer/issues/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 https://github.com/squizlabs/PHP_CodeSniffer/issues/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 https://github.com/squizlabs/PHP_CodeSniffer/issues/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 https://github.com/squizlabs/PHP_CodeSniffer/issues/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 https://github.com/squizlabs/PHP_CodeSniffer/issues/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 https://github.com/PHPCSStandards/PHP_CodeSniffer/commit/b3f4f83b2c1ad1a07086a8af09cb371fb0f4b1a4.

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 https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/482#pullrequestreview-2062765716

Types of changes

PR checklist

rodrigoprimo commented 2 weeks ago

@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.