-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Merged
jrfnl
merged 4 commits into
master
from
feature/110-437-generic-scopeindent-fix-undefined-array-index-notice
May 21, 2024
Merged
File::findStartOfStatement(): 3 bug fixes related to match
expressions
#502
jrfnl
merged 4 commits into
master
from
feature/110-437-generic-scopeindent-fix-undefined-array-index-notice
May 21, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 19, 2024
jrfnl
force-pushed
the
feature/110-437-generic-scopeindent-fix-undefined-array-index-notice
branch
2 times, most recently
from
May 19, 2024 22:38
7281c1d
to
f2ac92f
Compare
3 tasks
…"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
force-pushed
the
feature/110-437-generic-scopeindent-fix-undefined-array-index-notice
branch
from
May 21, 2024 22:20
f2ac92f
to
28c376e
Compare
Rebased without changes. Merging once the build passes. |
jrfnl
deleted the
feature/110-437-generic-scopeindent-fix-undefined-array-index-notice
branch
May 21, 2024 22:48
This was referenced Oct 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 othermatch
expressions.The
File::findStartOfStatement()
method has special handling formatch
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 thematch
, 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 thematch
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 formatch
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 thematch
, 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 thematch
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 formatch
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 aT_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 abracket_closer
which is beyond the$start
pointer.With these changes, the
$prevMatch
token will now always either be thematch
scope opener or aT_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
match
expressions, was incorrect in most cases.The trickle down effect of these bug fixes is that the
Generic.WhiteSpace.ScopeIndent
and thePEAR.WhiteSpace.ScopeIndent
sniffs should now be able to correctly determine and fix the indent formatch
expressions containing nested expressions.Related issues/external references
Fixes #110
Fixes #437
Fixes #475
Fixes squizlabs/PHP_CodeSniffer#3875
Types of changes