From 46b883dfc3693aafe3759c9fbcfe0c5b25235326 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 18 Apr 2024 05:39:11 +0200 Subject: [PATCH 1/2] BackfillFnTokenTest: use data providers when appropriate These two tests were testing multiple test cases, but not using a data provider, which means that if one of the test cases would fail, the ones after the failing test case wouldn't even be tested. By using a data provider, that issue is avoided. It also allows for simplifying some of the test code for the `testKeywordReturnTypes()` test which duplicated the logic for the `scopePositionTestHelper()` just to allow for custom error messages mentioning the test marker. This is no longer needed when each marker is tested individually via the data provider. --- tests/Core/Tokenizer/BackfillFnTokenTest.php | 100 ++++++++++++------- 1 file changed, 66 insertions(+), 34 deletions(-) diff --git a/tests/Core/Tokenizer/BackfillFnTokenTest.php b/tests/Core/Tokenizer/BackfillFnTokenTest.php index 8245b18756..d6d77c7681 100644 --- a/tests/Core/Tokenizer/BackfillFnTokenTest.php +++ b/tests/Core/Tokenizer/BackfillFnTokenTest.php @@ -16,21 +16,43 @@ final class BackfillFnTokenTest extends AbstractTokenizerTestCase /** * Test simple arrow functions. * - * @covers PHP_CodeSniffer\Tokenizers\PHP::processAdditional + * @param string $testMarker The comment prefacing the target token. + * + * @dataProvider dataSimple + * @covers PHP_CodeSniffer\Tokenizers\PHP::processAdditional * * @return void */ - public function testSimple() + public function testSimple($testMarker) { - foreach (['/* testStandard */', '/* testMixedCase */'] as $comment) { - $token = $this->getTargetToken($comment, T_FN); - $this->backfillHelper($token); - $this->scopePositionTestHelper($token, 5, 12); - } + $token = $this->getTargetToken($testMarker, T_FN); + $this->backfillHelper($token); + $this->scopePositionTestHelper($token, 5, 12); }//end testSimple() + /** + * Data provider. + * + * @see testSimple() + * + * @return array> + */ + public static function dataSimple() + { + return [ + 'standard' => [ + 'testMarker' => '/* testStandard */', + ], + 'mixed case' => [ + 'testMarker' => '/* testMixedCase */', + ], + ]; + + }//end dataSimple() + + /** * Test whitespace inside arrow function definitions. * @@ -370,44 +392,54 @@ public function testNamespaceOperatorInTypes() /** - * Test arrow functions that use self/parent/callable/array/static return types. + * Test arrow functions that use keyword return types. * - * @covers PHP_CodeSniffer\Tokenizers\PHP::processAdditional + * @param string $testMarker The comment prefacing the target token. + * + * @dataProvider dataKeywordReturnTypes + * @covers PHP_CodeSniffer\Tokenizers\PHP::processAdditional * * @return void */ - public function testKeywordReturnTypes() + public function testKeywordReturnTypes($testMarker) { $tokens = $this->phpcsFile->getTokens(); - $testMarkers = [ - 'Self', - 'Parent', - 'Callable', - 'Array', - 'Static', - ]; - - foreach ($testMarkers as $marker) { - $token = $this->getTargetToken('/* test'.$marker.'ReturnType */', T_FN); - $this->backfillHelper($token); - - $expectedScopeOpener = ($token + 11); - $expectedScopeCloser = ($token + 14); + $token = $this->getTargetToken($testMarker, T_FN); + $this->backfillHelper($token); + $this->scopePositionTestHelper($token, 11, 14); - $this->assertSame($expectedScopeOpener, $tokens[$token]['scope_opener'], "Scope opener is not the arrow token (for $marker)"); - $this->assertSame($expectedScopeCloser, $tokens[$token]['scope_closer'], "Scope closer is not the semicolon token(for $marker)"); + }//end testKeywordReturnTypes() - $opener = $tokens[$token]['scope_opener']; - $this->assertSame($expectedScopeOpener, $tokens[$opener]['scope_opener'], "Opener scope opener is not the arrow token(for $marker)"); - $this->assertSame($expectedScopeCloser, $tokens[$opener]['scope_closer'], "Opener scope closer is not the semicolon token(for $marker)"); - $closer = $tokens[$token]['scope_closer']; - $this->assertSame($expectedScopeOpener, $tokens[$closer]['scope_opener'], "Closer scope opener is not the arrow token(for $marker)"); - $this->assertSame($expectedScopeCloser, $tokens[$closer]['scope_closer'], "Closer scope closer is not the semicolon token(for $marker)"); - } + /** + * Data provider. + * + * @see testKeywordReturnTypes() + * + * @return array> + */ + public static function dataKeywordReturnTypes() + { + return [ + 'self' => [ + 'testMarker' => '/* testSelfReturnType */', + ], + 'parent' => [ + 'testMarker' => '/* testParentReturnType */', + ], + 'callable' => [ + 'testMarker' => '/* testCallableReturnType */', + ], + 'array' => [ + 'testMarker' => '/* testArrayReturnType */', + ], + 'static' => [ + 'testMarker' => '/* testStaticReturnType */', + ], + ]; - }//end testKeywordReturnTypes() + }//end dataKeywordReturnTypes() /** From bd6356c3281db766ac61437becfbb14f4c79cfff Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 18 Apr 2024 06:01:58 +0200 Subject: [PATCH 2/2] Tokenizer/PHP: arrow function tokenization broken when true/false used in return type Since PHP 8.0, `false` and `null` can be included in a union return type. As of PHP 8.2, both `true`, `false` and `null` can be used as a stand-alone return type. The tokenizer layer handling arrow functions did not take this into account correctly. While `null` was handled correctly, `true` and `false` was not and would result in the arrow function `fn` keyword being tokenized as `T_STRING` across all PHP versions. As a result of that, the other typical tokenizer changes related to arrow functions (`=>` as `T_FN_ARROW`, scope/parenthesis owners etc) would also not be executed correctly. In practice, I suspect few people will have run into this bug as, after all, what's the point of declaring an arrow function which will only ever return `true` or `false` ? so in practice, it is likely to only have come into play for people using `true` or `false` as part of an arrow function union type. All the same, PHPCS should handle this correctly. Includes unit tests proving the bug and safeguarding the fix. --- src/Tokenizers/PHP.php | 2 + tests/Core/Tokenizer/BackfillFnTokenTest.inc | 15 +++++++ tests/Core/Tokenizer/BackfillFnTokenTest.php | 41 ++++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/src/Tokenizers/PHP.php b/src/Tokenizers/PHP.php index 6e3ebdb6ae..dded2a5c17 100644 --- a/src/Tokenizers/PHP.php +++ b/src/Tokenizers/PHP.php @@ -2718,6 +2718,8 @@ protected function processAdditional() T_NAMESPACE => T_NAMESPACE, T_NS_SEPARATOR => T_NS_SEPARATOR, T_NULL => T_NULL, + T_TRUE => T_TRUE, + T_FALSE => T_FALSE, T_NULLABLE => T_NULLABLE, T_PARENT => T_PARENT, T_SELF => T_SELF, diff --git a/tests/Core/Tokenizer/BackfillFnTokenTest.inc b/tests/Core/Tokenizer/BackfillFnTokenTest.inc index 13f165b77f..6cf44229cc 100644 --- a/tests/Core/Tokenizer/BackfillFnTokenTest.inc +++ b/tests/Core/Tokenizer/BackfillFnTokenTest.inc @@ -92,12 +92,27 @@ fn(array $a) : array => $a; /* testStaticReturnType */ fn(array $a) : static => $a; +/* testFalseReturnType */ +fn(array $a) : false => $a; + +/* testTrueReturnType */ +fn(array $a) : True => $a; + +/* testNullReturnType */ +fn(array $a) : null => $a; + /* testUnionParamType */ $arrowWithUnionParam = fn(int|float $param) : SomeClass => new SomeClass($param); /* testUnionReturnType */ $arrowWithUnionReturn = fn($param) : int|float => $param | 10; +/* testUnionReturnTypeWithTrue */ +$arrowWithUnionReturn = fn($param) : int|true => $param | 10; + +/* testUnionReturnTypeWithFalse */ +$arrowWithUnionReturn = fn($param) : string|FALSE => $param | 10; + /* testTernary */ $fn = fn($a) => $a ? /* testTernaryThen */ fn() : string => 'a' : /* testTernaryElse */ fn() : string => 'b'; diff --git a/tests/Core/Tokenizer/BackfillFnTokenTest.php b/tests/Core/Tokenizer/BackfillFnTokenTest.php index d6d77c7681..5ad4e1b95a 100644 --- a/tests/Core/Tokenizer/BackfillFnTokenTest.php +++ b/tests/Core/Tokenizer/BackfillFnTokenTest.php @@ -437,6 +437,15 @@ public static function dataKeywordReturnTypes() 'static' => [ 'testMarker' => '/* testStaticReturnType */', ], + 'false' => [ + 'testMarker' => '/* testFalseReturnType */', + ], + 'true' => [ + 'testMarker' => '/* testTrueReturnType */', + ], + 'null' => [ + 'testMarker' => '/* testNullReturnType */', + ], ]; }//end dataKeywordReturnTypes() @@ -474,6 +483,38 @@ public function testUnionReturnType() }//end testUnionReturnType() + /** + * Test arrow function with a union return type. + * + * @covers PHP_CodeSniffer\Tokenizers\PHP::processAdditional + * + * @return void + */ + public function testUnionReturnTypeWithTrue() + { + $token = $this->getTargetToken('/* testUnionReturnTypeWithTrue */', T_FN); + $this->backfillHelper($token); + $this->scopePositionTestHelper($token, 11, 18); + + }//end testUnionReturnTypeWithTrue() + + + /** + * Test arrow function with a union return type. + * + * @covers PHP_CodeSniffer\Tokenizers\PHP::processAdditional + * + * @return void + */ + public function testUnionReturnTypeWithFalse() + { + $token = $this->getTargetToken('/* testUnionReturnTypeWithFalse */', T_FN); + $this->backfillHelper($token); + $this->scopePositionTestHelper($token, 11, 18); + + }//end testUnionReturnTypeWithFalse() + + /** * Test arrow functions used in ternary operators. *