Skip to content

Commit 8ebf0cf

Browse files
jrfnljrfnl
committed
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.
1 parent f681f8f commit 8ebf0cf

File tree

3 files changed

+105
-21
lines changed

3 files changed

+105
-21
lines changed

‎src/Files/File.php

Lines changed: 46 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2448,49 +2448,74 @@ public function findStartOfStatement($start, $ignore=null)
24482448
&& empty($this->tokens[$start]['nested_parenthesis']) === false)
24492449
&& $this->tokens[$matchExpression]['nested_parenthesis'] === $this->tokens[$start]['nested_parenthesis']))
24502450
) {
2451+
// Walk back to the previous match arrow (if it exists).
2452+
$lastComma = null;
2453+
$inNestedExpression = false;
24512454
for ($prevMatch = $start; $prevMatch > $this->tokens[$matchExpression]['scope_opener']; $prevMatch--) {
2452-
if ($prevMatch !== $start
2453-
&& ($this->tokens[$prevMatch]['code'] === T_MATCH_ARROW
2454-
|| $this->tokens[$prevMatch]['code'] === T_COMMA)
2455-
) {
2455+
if ($prevMatch !== $start && $this->tokens[$prevMatch]['code'] === T_MATCH_ARROW) {
24562456
break;
24572457
}
24582458

2459+
if ($prevMatch !== $start && $this->tokens[$prevMatch]['code'] === T_COMMA) {
2460+
$lastComma = $prevMatch;
2461+
continue;
2462+
}
2463+
24592464
// Skip nested statements.
24602465
if (isset($this->tokens[$prevMatch]['bracket_opener']) === true
24612466
&& $prevMatch === $this->tokens[$prevMatch]['bracket_closer']
24622467
) {
24632468
$prevMatch = $this->tokens[$prevMatch]['bracket_opener'];
2464-
} else if (isset($this->tokens[$prevMatch]['parenthesis_opener']) === true
2469+
continue;
2470+
}
2471+
2472+
if (isset($this->tokens[$prevMatch]['parenthesis_opener']) === true
24652473
&& $prevMatch === $this->tokens[$prevMatch]['parenthesis_closer']
24662474
) {
24672475
$prevMatch = $this->tokens[$prevMatch]['parenthesis_opener'];
2476+
continue;
24682477
}
2469-
}
24702478

2471-
if ($prevMatch <= $this->tokens[$matchExpression]['scope_opener']) {
2472-
// We're before the arrow in the first case.
2473-
$next = $this->findNext(Tokens::$emptyTokens, ($this->tokens[$matchExpression]['scope_opener'] + 1), null, true);
2474-
if ($next === false) {
2475-
return $start;
2479+
// Stop if we're _within_ a nested short array statement, which may contain comma's too.
2480+
// No need to deal with parentheses, those are handled above via the `nested_parenthesis` checks.
2481+
if (isset($this->tokens[$prevMatch]['bracket_opener']) === true
2482+
&& $this->tokens[$prevMatch]['bracket_closer'] > $start
2483+
) {
2484+
$inNestedExpression = true;
2485+
break;
24762486
}
2487+
}//end for
24772488

2478-
return $next;
2479-
}
2480-
2481-
if ($this->tokens[$prevMatch]['code'] === T_COMMA) {
2482-
// We're before the arrow, but not in the first case.
2483-
$prevMatchArrow = $this->findPrevious(T_MATCH_ARROW, ($prevMatch - 1), $this->tokens[$matchExpression]['scope_opener']);
2484-
if ($prevMatchArrow === false) {
2489+
if ($inNestedExpression === false) {
2490+
// $prevMatch will now either be the scope opener or a match arrow.
2491+
// If it is the scope opener, go the first non-empty token after. $start will have been part of the first condition.
2492+
if ($prevMatch <= $this->tokens[$matchExpression]['scope_opener']) {
24852493
// We're before the arrow in the first case.
24862494
$next = $this->findNext(Tokens::$emptyTokens, ($this->tokens[$matchExpression]['scope_opener'] + 1), null, true);
2495+
if ($next === false) {
2496+
// Shouldn't be possible.
2497+
return $start;
2498+
}
2499+
24872500
return $next;
24882501
}
24892502

2490-
$end = $this->findEndOfStatement($prevMatchArrow);
2491-
$next = $this->findNext(Tokens::$emptyTokens, ($end + 1), null, true);
2503+
// Okay, so we found a match arrow.
2504+
// If $start was part of the "next" condition, the last comma will be set.
2505+
// Otherwise, $start must have been part of a return expression.
2506+
if (isset($lastComma) === true && $lastComma > $prevMatch) {
2507+
$prevMatch = $lastComma;
2508+
}
2509+
2510+
// In both cases, go to the first non-empty token after.
2511+
$next = $this->findNext(Tokens::$emptyTokens, ($prevMatch + 1), null, true);
2512+
if ($next === false) {
2513+
// Shouldn't be possible.
2514+
return $start;
2515+
}
2516+
24922517
return $next;
2493-
}
2518+
}//end if
24942519
}//end if
24952520
}//end if
24962521

‎tests/Core/File/FindStartOfStatementTest.inc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,3 +192,9 @@ callMe($paramA, match ($var) {
192192
'c' => fn($p1, /* test437FnSecondParamWithinNestedMatch */ $p2) => $p1 + $p2,
193193
default => false
194194
});
195+
196+
match ($var) {
197+
/* test437NestedShortArrayWithinMatch */
198+
'a' => [ 1, 2.5, $var],
199+
default => false
200+
};

‎tests/Core/File/FindStartOfStatementTest.php

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -880,4 +880,57 @@ public static function dataFindStartInsideParenthesesNestedWithinNestedMatch()
880880
}//end dataFindStartInsideParenthesesNestedWithinNestedMatch()
881881

882882

883+
/**
884+
* Test finding the start of a statement for a token within a short array within a match expressions.
885+
*
886+
* @param string $testMarker The comment which prefaces the target token in the test file.
887+
* @param int|string $target The token to search for after the test marker.
888+
* @param int|string $expectedTarget Token code of the expected start of statement stack pointer.
889+
*
890+
* @link https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/437
891+
*
892+
* @dataProvider dataFindStartInsideShortArrayNestedWithinMatch
893+
*
894+
* @return void
895+
*/
896+
public function testFindStartInsideShortArrayNestedWithinMatch($testMarker, $target, $expectedTarget)
897+
{
898+
$testToken = $this->getTargetToken($testMarker, $target);
899+
$expected = $this->getTargetToken($testMarker, $expectedTarget);
900+
901+
$found = self::$phpcsFile->findStartOfStatement($testToken);
902+
903+
$this->assertSame($expected, $found);
904+
905+
}//end testFindStartInsideShortArrayNestedWithinMatch()
906+
907+
908+
/**
909+
* Data provider.
910+
*
911+
* @return array<string, array<string, int|string>>
912+
*/
913+
public static function dataFindStartInsideShortArrayNestedWithinMatch()
914+
{
915+
return [
916+
'Array item itself should be start for first array item' => [
917+
'testMarker' => '/* test437NestedShortArrayWithinMatch */',
918+
'targets' => T_LNUMBER,
919+
'expectedTarget' => T_LNUMBER,
920+
],
921+
'Array item itself should be start for second array item' => [
922+
'testMarker' => '/* test437NestedShortArrayWithinMatch */',
923+
'targets' => T_DNUMBER,
924+
'expectedTarget' => T_DNUMBER,
925+
],
926+
'Array item itself should be start for third array item' => [
927+
'testMarker' => '/* test437NestedShortArrayWithinMatch */',
928+
'targets' => T_VARIABLE,
929+
'expectedTarget' => T_VARIABLE,
930+
],
931+
];
932+
933+
}//end dataFindStartInsideShortArrayNestedWithinMatch()
934+
935+
883936
}//end class

0 commit comments

Comments
 (0)