Skip to content

Commit 467d284

Browse files
jrfnljrfnl
committed
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.
1 parent 808dff8 commit 467d284

File tree

3 files changed

+134
-37
lines changed

3 files changed

+134
-37
lines changed

‎src/Files/File.php‎

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -2435,51 +2435,56 @@ public function findStartOfStatement($start, $ignore=null)
24352435
// If the start token is inside the case part of a match expression,
24362436
// find the start of the condition. If it's in the statement part, find
24372437
// the token that comes after the match arrow.
2438-
$matchExpression = $this->getCondition($start, T_MATCH);
2439-
if ($matchExpression !== false) {
2440-
for ($prevMatch = $start; $prevMatch > $this->tokens[$matchExpression]['scope_opener']; $prevMatch--) {
2441-
if ($prevMatch !== $start
2442-
&& ($this->tokens[$prevMatch]['code'] === T_MATCH_ARROW
2443-
|| $this->tokens[$prevMatch]['code'] === T_COMMA)
2444-
) {
2445-
break;
2446-
}
2447-
2448-
// Skip nested statements.
2449-
if (isset($this->tokens[$prevMatch]['bracket_opener']) === true
2450-
&& $prevMatch === $this->tokens[$prevMatch]['bracket_closer']
2451-
) {
2452-
$prevMatch = $this->tokens[$prevMatch]['bracket_opener'];
2453-
} else if (isset($this->tokens[$prevMatch]['parenthesis_opener']) === true
2454-
&& $prevMatch === $this->tokens[$prevMatch]['parenthesis_closer']
2455-
) {
2456-
$prevMatch = $this->tokens[$prevMatch]['parenthesis_opener'];
2457-
}
2458-
}
2438+
if (empty($this->tokens[$start]['conditions']) === false) {
2439+
$conditions = $this->tokens[$start]['conditions'];
2440+
$lastConditionOwner = end($conditions);
2441+
2442+
if ($lastConditionOwner === T_MATCH) {
2443+
$matchExpression = key($conditions);
2444+
for ($prevMatch = $start; $prevMatch > $this->tokens[$matchExpression]['scope_opener']; $prevMatch--) {
2445+
if ($prevMatch !== $start
2446+
&& ($this->tokens[$prevMatch]['code'] === T_MATCH_ARROW
2447+
|| $this->tokens[$prevMatch]['code'] === T_COMMA)
2448+
) {
2449+
break;
2450+
}
24592451

2460-
if ($prevMatch <= $this->tokens[$matchExpression]['scope_opener']) {
2461-
// We're before the arrow in the first case.
2462-
$next = $this->findNext(Tokens::$emptyTokens, ($this->tokens[$matchExpression]['scope_opener'] + 1), null, true);
2463-
if ($next === false) {
2464-
return $start;
2452+
// Skip nested statements.
2453+
if (isset($this->tokens[$prevMatch]['bracket_opener']) === true
2454+
&& $prevMatch === $this->tokens[$prevMatch]['bracket_closer']
2455+
) {
2456+
$prevMatch = $this->tokens[$prevMatch]['bracket_opener'];
2457+
} else if (isset($this->tokens[$prevMatch]['parenthesis_opener']) === true
2458+
&& $prevMatch === $this->tokens[$prevMatch]['parenthesis_closer']
2459+
) {
2460+
$prevMatch = $this->tokens[$prevMatch]['parenthesis_opener'];
2461+
}
24652462
}
24662463

2467-
return $next;
2468-
}
2469-
2470-
if ($this->tokens[$prevMatch]['code'] === T_COMMA) {
2471-
// We're before the arrow, but not in the first case.
2472-
$prevMatchArrow = $this->findPrevious(T_MATCH_ARROW, ($prevMatch - 1), $this->tokens[$matchExpression]['scope_opener']);
2473-
if ($prevMatchArrow === false) {
2464+
if ($prevMatch <= $this->tokens[$matchExpression]['scope_opener']) {
24742465
// We're before the arrow in the first case.
24752466
$next = $this->findNext(Tokens::$emptyTokens, ($this->tokens[$matchExpression]['scope_opener'] + 1), null, true);
2467+
if ($next === false) {
2468+
return $start;
2469+
}
2470+
24762471
return $next;
24772472
}
24782473

2479-
$end = $this->findEndOfStatement($prevMatchArrow);
2480-
$next = $this->findNext(Tokens::$emptyTokens, ($end + 1), null, true);
2481-
return $next;
2482-
}
2474+
if ($this->tokens[$prevMatch]['code'] === T_COMMA) {
2475+
// We're before the arrow, but not in the first case.
2476+
$prevMatchArrow = $this->findPrevious(T_MATCH_ARROW, ($prevMatch - 1), $this->tokens[$matchExpression]['scope_opener']);
2477+
if ($prevMatchArrow === false) {
2478+
// We're before the arrow in the first case.
2479+
$next = $this->findNext(Tokens::$emptyTokens, ($this->tokens[$matchExpression]['scope_opener'] + 1), null, true);
2480+
return $next;
2481+
}
2482+
2483+
$end = $this->findEndOfStatement($prevMatchArrow);
2484+
$next = $this->findNext(Tokens::$emptyTokens, ($end + 1), null, true);
2485+
return $next;
2486+
}
2487+
}//end if
24832488
}//end if
24842489

24852490
$lastNotEmpty = $start;

‎tests/Core/File/FindStartOfStatementTest.inc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,3 +162,13 @@ switch ($foo) {
162162
/* testInsideDefaultContinueStatement */
163163
continue $var;
164164
}
165+
166+
match ($var) {
167+
true =>
168+
/* test437ClosureDeclaration */
169+
function ($var) {
170+
/* test437EchoNestedWithinClosureWithinMatch */
171+
echo $var, 'text', PHP_EOL;
172+
},
173+
default => false
174+
};

‎tests/Core/File/FindStartOfStatementTest.php‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -637,4 +637,86 @@ public static function dataFindStartInsideSwitchCaseDefaultStatements()
637637
}//end dataFindStartInsideSwitchCaseDefaultStatements()
638638

639639

640+
/**
641+
* Test finding the start of a statement inside a closed scope nested within a match expressions.
642+
*
643+
* @param string $testMarker The comment which prefaces the target token in the test file.
644+
* @param int|string $target The token to search for after the test marker.
645+
* @param int|string $expectedTarget Token code of the expected start of statement stack pointer.
646+
*
647+
* @link https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/437
648+
*
649+
* @dataProvider dataFindStartInsideClosedScopeNestedWithinMatch
650+
*
651+
* @return void
652+
*/
653+
public function testFindStartInsideClosedScopeNestedWithinMatch($testMarker, $target, $expectedTarget)
654+
{
655+
$testToken = $this->getTargetToken($testMarker, $target);
656+
$expected = $this->getTargetToken($testMarker, $expectedTarget);
657+
658+
$found = self::$phpcsFile->findStartOfStatement($testToken);
659+
660+
$this->assertSame($expected, $found);
661+
662+
}//end testFindStartInsideClosedScopeNestedWithinMatch()
663+
664+
665+
/**
666+
* Data provider.
667+
*
668+
* @return array<string, array<string, int|string>>
669+
*/
670+
public static function dataFindStartInsideClosedScopeNestedWithinMatch()
671+
{
672+
return [
673+
// These were already working correctly.
674+
'Closure function keyword should be start of closure - closure keyword' => [
675+
'testMarker' => '/* test437ClosureDeclaration */',
676+
'targets' => T_CLOSURE,
677+
'expectedTarget' => T_CLOSURE,
678+
],
679+
'Open curly is a statement/expression opener - open curly' => [
680+
'testMarker' => '/* test437ClosureDeclaration */',
681+
'targets' => T_OPEN_CURLY_BRACKET,
682+
'expectedTarget' => T_OPEN_CURLY_BRACKET,
683+
],
684+
685+
'Echo should be start for expression - echo keyword' => [
686+
'testMarker' => '/* test437EchoNestedWithinClosureWithinMatch */',
687+
'targets' => T_ECHO,
688+
'expectedTarget' => T_ECHO,
689+
],
690+
'Echo should be start for expression - variable' => [
691+
'testMarker' => '/* test437EchoNestedWithinClosureWithinMatch */',
692+
'targets' => T_VARIABLE,
693+
'expectedTarget' => T_ECHO,
694+
],
695+
'Echo should be start for expression - comma' => [
696+
'testMarker' => '/* test437EchoNestedWithinClosureWithinMatch */',
697+
'targets' => T_COMMA,
698+
'expectedTarget' => T_ECHO,
699+
],
700+
701+
// These were not working correctly and would previously return the close curly of the match expression.
702+
'First token after comma in echo expression should be start for expression - text string' => [
703+
'testMarker' => '/* test437EchoNestedWithinClosureWithinMatch */',
704+
'targets' => T_CONSTANT_ENCAPSED_STRING,
705+
'expectedTarget' => T_CONSTANT_ENCAPSED_STRING,
706+
],
707+
'First token after comma in echo expression - PHP_EOL constant' => [
708+
'testMarker' => '/* test437EchoNestedWithinClosureWithinMatch */',
709+
'targets' => T_STRING,
710+
'expectedTarget' => T_STRING,
711+
],
712+
'First token after comma in echo expression - semicolon' => [
713+
'testMarker' => '/* test437EchoNestedWithinClosureWithinMatch */',
714+
'targets' => T_SEMICOLON,
715+
'expectedTarget' => T_STRING,
716+
],
717+
];
718+
719+
}//end dataFindStartInsideClosedScopeNestedWithinMatch()
720+
721+
640722
}//end class

0 commit comments

Comments
 (0)