Skip to content

Tests/Tokenizer: fix conditions checking range in T_DEFAULT test #870

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
Merged
merged 1 commit into from
Mar 14, 2025
Merged

Tests/Tokenizer: fix conditions checking range in T_DEFAULT test #870

merged 1 commit into from
Mar 14, 2025

Conversation

@rodrigoprimo
Copy link
Contributor

Description

This PR addresses an issue that I missed when working on #850. Among other things, the RecurseScopeMapDefaultKeywordConditionsTest ::testSwitchDefault() method checks if tokens within the scope of a T_DEFAULT token have T_DEFAULT set as one of its conditions array.

The test incorrectly checked token conditions due to an off-by-one error in the loop range when the T_DEFAULT uses curly braces.

For a T_DEFAULT with curly braces, the tokenizer adds T_DEFAULT to the conditions array of all the tokens within its scope up to the T_BREAK|T_RETURN|T_CONTINUE, but the test was checking only until the token before the T_BREAK|T_RETURN|T_CONTINUE. While for a T_DEFAULT without curly braces, it adds to all the tokens within the scope until the token before the scope closer.

This commit updates the test to ensure that all tokens within a default case scope that should have the T_DEFAULT token in their conditions array are properly checked. To achieve that, the loop was modified:

  • The end point ($end) is now set to $closer - 1 when curly braces are not used.
  • The loop condition uses inclusive comparison (<=) to ensure that the $end is also checked.

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 rodrigoprimo mentioned this pull request Mar 11, 2025
Merged
11 tasks
jrfnl
jrfnl reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@jrfnl jrfnl left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigoprimo To me, this change is difficult to grasp. As the bug is supposed to be with the curly braces (which use the $conditionStop variable), why are you changing the $end for non-curly braced expressions ?

It would seem more logical to me to change line 162 to $end = ($token + $conditionStop + 1);.

@rodrigoprimo
Copy link
Contributor Author

I considered changing line 162 to $end = ($token + $conditionStop + 1);, but decided in favor of the approach in this PR because I think it is easier to understand the behavior of the code when reading it as it matches more accurately what the tokenizer does:

  • For non-curly braced expressions, the last token with T_DEFAULT set in the conditions array is the one just before the scope closer ($closer - 1).
  • For curly braced expressions, I find it better if $conditionsStop matches the exact offset in relation to the testMarker after which T_DEFAULT stops being set in the conditions array than setting $conditionStop to a value and then having to add 1 to actually get the value that is needed.

That being said, I think both options are ok, and I'm happy to change to what you are suggesting if you prefer.

@jrfnl
Copy link
Member

jrfnl commented Mar 13, 2025

@rodrigoprimo Maybe there shouldn't even be any change to the logic, just a change in how the $conditionStop is set and making sure that the definition of what token that parameter is supposed to hold is clearly defined in the method docblock... ?

@rodrigoprimo
Copy link
Contributor Author

@jrfnl, if you prefer this approach, that works for me, and I can update the PR.

@jrfnl
Copy link
Member

jrfnl commented Mar 13, 2025

@rodrigoprimo I have no real preference, other than that the commit message matches what the PR does and that's not the case with the current fix.

@rodrigoprimo
Tests/Tokenizer: fix conditions checking range in T_DEFAULT test
c07267e 
Among other things, the RecurseScopeMapDefaultKeywordConditionsTest
::testSwitchDefault() method checks if tokens within the scope of a
`T_DEFAULT` token have `T_DEFAULT` set as one of its `conditions` array.

The test was incorrectly checking token `conditions` due to an
off-by-one error in the loop range when the `T_DEFAULT` uses curly
braces.

For a `T_DEFAULT` with curly braces, the tokenizer adds `T_DEFAULT` to
the `conditions` array of all the tokens within its scope up to the
`T_BREAK|T_RETURN|T_CONTINUE`, but the test was checking only until the
token before the `T_BREAK|T_RETURN|T_CONTINUE`.
@rodrigoprimo rodrigoprimo force-pushed the another-fix-to-default-keyword-tests branch from 7723d59 to c07267e Compare March 13, 2025 17:19
@rodrigoprimo
Copy link
Contributor Author

@jrfnl, I went with your first suggestion and changed line 162 to $end = ($token + $conditionStop + 1);. I also updated the commit message. Since this is a small change, I amended the original commit and force pushed. I hope that is ok.

@jrfnl jrfnl added this to the 3.12.0 milestone Mar 14, 2025
@jrfnl jrfnl merged commit c0c08ad into PHPCSStandards:master Mar 14, 2025
46 checks passed
@jrfnl jrfnl deleted the another-fix-to-default-keyword-tests branch March 14, 2025 00:56
@rodrigoprimo rodrigoprimo mentioned this pull request Mar 25, 2025
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jrfnl jrfnl jrfnl left review comments

Assignees

No one assigned

Labels

Core Component: Tokenizer Type: chores/QA

Projects

None yet

Milestone

3.12.0

Development

Successfully merging this pull request may close these issues.

2 participants

@rodrigoprimo @jrfnl