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.

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

File::findStartOfStatement(): 3 bug fixes related to match expressions #502

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented May 19, 2024

Description

File::findStartOfStatement(): bug fix - don't give nested scopes the "match" treatment

The only "scopes" which can be nested within a match expression are closures, anonymous classes and other match expressions.

The File::findStartOfStatement() method has special handling for match expressions to find the start of a statement, but that special handling would also kick in when the $start token is within another scope nested within the match, while, in that case, the special handling is not needed and ends up resulting in an incorrect "start" pointer being returned, in most cases, even a "start pointer" which is after the token for which the start of statement is requested, which should never be possible.

Fixed now by changing the special handling for match expressions to only kick in when the match expression is the deepest nested scope.

Includes unit tests.

File::findStartOfStatement(): bug fix - don't give tokens within nested parentheses the "match" treatment

The File::findStartOfStatement() method has special handling for match expressions to find the start of a statement, but that special handling would also kick in when the $start token is within a parenthesized expression within the match, while, in that case, the special handling is not needed and ends up returning an incorrect "start" pointer, in most cases, even a "start pointer" which is after the token for which the start of statement is requested, which should never be possible.

Fixed now by changing the special handling for match expressions to only kick in when the $start pointer and the match pointer are at the same parentheses nesting level.

Includes unit tests.
Of the tests, the first array item/parameter was not affected by this bug, but subsequent array items/parameters were all affected by this bug.

File::findStartOfStatement(): bug fix - don't confuse comma's in short arrays with comma's belonging to a match expression

The File::findStartOfStatement() method has special handling for match expressions to find the start of a statement, but that special handling did not correctly take the potential of comma's in nested short arrays into account.
This would lead to any array item after the first getting the wrong return value.

As there is currently isn't a "nested_brackets" index in the token array (also see the proposal for this in #12), there is no information available within the token array to prevent the special handling for match expressions from kicking in.

So, we need to skip out of the special handling if it is detected that the $start token is within a short array.
In practice, this means we cannot break on a T_COMMA, as at that point we don't know if it is a comma from within a nested short array.
Instead, we have to walk back to the last T_MATCH_ARROW to be sure and break out off the special handling if we encounter a [/bracket opener with a bracket_closer which is beyond the $start pointer.

With these changes, the $prevMatch token will now always either be the match scope opener or a T_MATCH_ARROW. With this knowledge, we can now simplify the special handling for tokens which do belong to the match expression itself a little.

Includes unit tests.
Of the tests, the first array item was not affected by this bug, but subsequent array items were all affected by this bug.

Generic/ScopeIndent: add tests for issues #110 and #437

Both the mentioned issues are fixed by the improvements to the File::findStartOfStatement() method in this same PR.

Suggested changelog entry

  • File::findStartOfStatement(): the start of statement/expression determination for tokens in parentheses/short array brackets/others scopes, nested within match expressions, was incorrect in most cases.
    The trickle down effect of these bug fixes is that the Generic.WhiteSpace.ScopeIndent and the PEAR.WhiteSpace.ScopeIndent sniffs should now be able to correctly determine and fix the indent for match expressions containing nested expressions.

Related issues/external references

Fixes #110
Fixes #437
Fixes #475
Fixes squizlabs/PHP_CodeSniffer#3875

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

jrfnl added 4 commits May 22, 2024 00:20
…"match" treatment

The only "scopes" which can be nested within a `match` expression are closures, anonymous classes and other `match` expressions.

The `File::findStartOfStatement()` method has special handling for `match` expressions to find the start of a statement, but that special handling would also kick in when the `$start` token is within another scope nested within the `match`, while, in that case, the special handling is not needed and ends up resulting in an incorrect "start" pointer being returned, in most cases, even a "start pointer" which is _after_ the token for which the start of statement is requested, which should never be possible.

Fixed now by changing the special handling for `match` expressions to only kick in when the `match` expression is the _deepest_ nested scope.

Includes unit tests.
…ed parentheses the "match" treatment

The `File::findStartOfStatement()` method has special handling for `match` expressions to find the start of a statement, but that special handling would also kick in when the `$start` token is within a parenthesized expression within the `match`, while, in that case, the special handling is not needed and ends up returning an incorrect "start" pointer, in most cases, even a "start pointer" which is _after_ the token for which the start of statement is requested, which should never be possible.

Fixed now by changing the special handling for `match` expressions to only kick in when the `$start` pointer and the `match` pointer are at the same parentheses nesting level.

Includes unit tests.
Of the tests, the first array item/parameter was not affected by this bug, but subsequent array items/parameters were all affected by this bug.
…t arrays with comma's belonging to a match expression

The `File::findStartOfStatement()` method has special handling for `match` expressions to find the start of a statement, but that special handling did not correctly take the potential of comma's in nested short arrays into account.
This would lead to any array item after the first getting the wrong return value.

As there is currently isn't a "nested_brackets" index in the token array (also see the proposal for this in 12), there is no information available within the token array to prevent the special handling for `match` expressions from kicking in.

So, we need to skip _out_ of the special handling if it is detected that the `$start` token is within a short array.
In practice, this means we cannot `break` on a `T_COMMA`, as at that point we don't know if it is a comma from within a nested short array.
Instead, we have to walk back to the last `T_MATCH_ARROW` to be sure and break out off the special handling if we encounter a `[`/`bracket opener` with a `bracket_closer` which is beyond the `$start` pointer.

With these changes, the `$prevMatch` token will now always either be the `match` scope opener or a `T_MATCH_ARROW`. With this knowledge, we can now simplify the special handling for tokens which _do_ belong to the match expression itself a little.

Includes unit tests.
Of the tests, the first array item was not affected by this bug, but subsequent array items were all affected by this bug.
Both the mentioned issues are fixed by the improvements to the `File::findStartOfStatement()` method in this same PR.

Fixes 110
Fixes 437
Fixes squizlabs/PHP_CodeSniffer 3875
@jrfnl jrfnl force-pushed the feature/110-437-generic-scopeindent-fix-undefined-array-index-notice branch from f2ac92f to 28c376e Compare May 21, 2024 22:20
@jrfnl
Copy link
Member Author

jrfnl commented May 21, 2024

Rebased without changes. Merging once the build passes.

@jrfnl jrfnl merged commit 027c0cb into master May 21, 2024
48 checks passed
@jrfnl jrfnl deleted the feature/110-437-generic-scopeindent-fix-undefined-array-index-notice branch May 21, 2024 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment