From aca4d9fb2994b9238badad39ed6249b7ca73c50a Mon Sep 17 00:00:00 2001 From: Greg Sherwood Date: Tue, 17 Apr 2018 11:09:54 +1000 Subject: [PATCH] Added support for nullable return types (ref #1527) --- package.xml | 8 +- src/Files/File.php | 42 ++-- src/Tokenizers/PHP.php | 12 +- tests/Core/File/GetMethodPropertiesTest.inc | 9 + tests/Core/File/GetMethodPropertiesTest.php | 220 +++++++++++++------- 5 files changed, 198 insertions(+), 93 deletions(-) diff --git a/package.xml b/package.xml index 0d7aa1c6f0..9b029524df 100644 --- a/package.xml +++ b/package.xml @@ -36,7 +36,7 @@ http://pear.php.net/dtd/package-2.0.xsd"> - The current method for setting array properties in ruleset files has been deprecated and will be removed in version 4 -- Currently, setting an array value uses the string syntax "print=>echo,create_function=>null" -- Now, invididual array elements are specifed using a new "element" tag with "key" and "value" attributes - -- For example, element key="print" value="echo" + --- For example, element key="print" value="echo" -- Thanks to Michał Bundyra for the patch - Config values set using --runtime-set now override any config values set in rulesets or the CodeSniffer.conf file @@ -50,7 +50,7 @@ http://pear.php.net/dtd/package-2.0.xsd"> - PHPCS annotations now remain as T_PHPCS_* tokens instead of reverting to comment tokens when --ignore-annotations is used -- This stops sniffs (especially commenting sniffs) from generating a large number of false errors when ignoring -- Any custom sniffs that are using the T_PHPCS_* tokens to detect annotations may need to be changed to ignore them - -- Check $phpcsFile->config->annotations to see if annotations are enabled and ignore when false + --- Check $phpcsFile->config->annotations to see if annotations are enabled and ignore when false - Function return types are now tokenized as T_RETURN_TYPE in all cases -- Previously, only simple types were tokenized in this way and namespaces were ignored -- Custom sniffs looking for T_NS_SEPERATOR inside return types will now need to look for T_RETURN_TYPE and check the value directly @@ -65,8 +65,10 @@ http://pear.php.net/dtd/package-2.0.xsd"> -- Thanks to Timo Schinkel for the patch - The return value of File::getMethodProperties() now contains a "return_type" array index -- This contains the return type of the function or closer, or a blank string if not specified + -- If the return type is nullable, the return type will contain the leading ? + --- A nullable_return_type array index in the return value will also be set to true -- If the return type contains namespace information, it will be cleaned of whitespace and comments - -- To access the original return value string, use the main tokens array + --- To access the original return value string, use the main tokens array - The key used for caching PHPCS runs now includes all set config values -- This fixes a problem where changing config values (e.g., via --runtime-set) used an incorrect cache file - The "Function opening brace placement" metric has been separated into function and closure metrics in the info report diff --git a/src/Files/File.php b/src/Files/File.php index 54df386aac..87f7b5b88f 100644 --- a/src/Files/File.php +++ b/src/Files/File.php @@ -1427,12 +1427,13 @@ public function getMethodParameters($stackPtr) * The format of the array is: * * array( - * 'scope' => 'public', // public protected or protected - * 'return_type' => '', // the return type of the method. - * 'scope_specified' => true, // true is scope keyword was found. - * 'is_abstract' => false, // true if the abstract keyword was found. - * 'is_final' => false, // true if the final keyword was found. - * 'is_static' => false, // true if the static keyword was found. + * 'scope' => 'public', // public protected or protected + * 'scope_specified' => true, // true is scope keyword was found. + * 'return_type' => '', // the return type of the method. + * 'nullable_return_type' => false, // true if the return type is nullable. + * 'is_abstract' => false, // true if the abstract keyword was found. + * 'is_final' => false, // true if the final keyword was found. + * 'is_static' => false, // true if the static keyword was found. * ); * * @@ -1474,10 +1475,11 @@ public function getMethodProperties($stackPtr) $scope = 'public'; $scopeSpecified = false; - $isAbstract = false; - $isFinal = false; - $isStatic = false; $returnType = ''; + $nullableReturnType = false; + $isAbstract = false; + $isFinal = false; + $isStatic = false; for ($i = ($stackPtr - 1); $i > 0; $i--) { if (isset($valid[$this->tokens[$i]['code']]) === false) { @@ -1523,26 +1525,34 @@ public function getMethodProperties($stackPtr) break; } + if ($this->tokens[$i]['code'] === T_NULLABLE) { + $nullableReturnType = true; + } + while ($this->tokens[$i]['code'] === T_RETURN_TYPE) { $returnType .= $this->tokens[$i]['content']; $i++; } } - } + }//end if if ($returnType !== '') { // Cleanup. $returnType = preg_replace('/\s+/', '', $returnType); $returnType = preg_replace('/\/\*.*?\*\//', '', $returnType); + if ($nullableReturnType === true) { + $returnType = '?'.$returnType; + } } return [ - 'scope' => $scope, - 'return_type' => $returnType, - 'scope_specified' => $scopeSpecified, - 'is_abstract' => $isAbstract, - 'is_final' => $isFinal, - 'is_static' => $isStatic, + 'scope' => $scope, + 'scope_specified' => $scopeSpecified, + 'return_type' => $returnType, + 'nullable_return_type' => $nullableReturnType, + 'is_abstract' => $isAbstract, + 'is_final' => $isFinal, + 'is_static' => $isStatic, ]; }//end getMethodProperties() diff --git a/src/Tokenizers/PHP.php b/src/Tokenizers/PHP.php index 99797c81f7..2c1aa42c19 100644 --- a/src/Tokenizers/PHP.php +++ b/src/Tokenizers/PHP.php @@ -1057,7 +1057,7 @@ protected function tokenize($string) ) { // Non-empty content. if (is_array($tokens[$x]) === true && $tokens[$x][0] === T_USE) { - // Search ahead for the closing parenthesis. + // Found a use statements, so search ahead for the closing parenthesis. for ($x = ($x + 1); $x < $numTokens; $x++) { if (is_array($tokens[$x]) === false && $tokens[$x] === ')') { continue(2); @@ -1090,7 +1090,15 @@ protected function tokenize($string) && is_array($tokens[$x]) === true && isset(Util\Tokens::$emptyTokens[$tokens[$x][0]]) === true ) { - // We haven't found the start of the type hint yet. + // Whitespace or coments before the type hint. + continue; + } + + if ($typeHintStart === null + && is_array($tokens[$x]) === false + && $tokens[$x] === '?' + ) { + // Found a nullable operator, so skip it. continue; } diff --git a/tests/Core/File/GetMethodPropertiesTest.inc b/tests/Core/File/GetMethodPropertiesTest.inc index 2ca4e4deb1..2aad32dfe3 100644 --- a/tests/Core/File/GetMethodPropertiesTest.inc +++ b/tests/Core/File/GetMethodPropertiesTest.inc @@ -28,6 +28,15 @@ class MyClass { /* testPublicReturnMethod */ public function myFunction(): array {} + /* testNullableReturnMethod */ + public function myFunction(): ?array {} + + /* testMessyNullableReturnMethod */ + public function myFunction() /* comment + */ : + /* comment */ ? //comment + array {} + /* testReturnNamespace */ function myFunction(): \MyNamespace\MyClass {} diff --git a/tests/Core/File/GetMethodPropertiesTest.php b/tests/Core/File/GetMethodPropertiesTest.php index 57d03095ae..8f4d010309 100644 --- a/tests/Core/File/GetMethodPropertiesTest.php +++ b/tests/Core/File/GetMethodPropertiesTest.php @@ -67,12 +67,13 @@ public function tearDown() public function testBasicFunction() { $expected = [ - 'scope' => 'public', - 'return_type' => '', - 'scope_specified' => false, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -98,12 +99,13 @@ public function testBasicFunction() public function testReturnFunction() { $expected = [ - 'scope' => 'public', - 'return_type' => 'array', - 'scope_specified' => false, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => 'array', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -129,12 +131,13 @@ public function testReturnFunction() public function testNestedClosure() { $expected = [ - 'scope' => 'public', - 'return_type' => 'int', - 'scope_specified' => false, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => 'int', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -160,12 +163,13 @@ public function testNestedClosure() public function testBasicMethod() { $expected = [ - 'scope' => 'public', - 'return_type' => '', - 'scope_specified' => false, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -191,12 +195,13 @@ public function testBasicMethod() public function testPrivateStaticMethod() { $expected = [ - 'scope' => 'private', - 'return_type' => '', - 'scope_specified' => true, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => true, + 'scope' => 'private', + 'scope_specified' => true, + 'return_type' => '', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => true, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -222,12 +227,13 @@ public function testPrivateStaticMethod() public function testFinalMethod() { $expected = [ - 'scope' => 'public', - 'return_type' => '', - 'scope_specified' => true, - 'is_abstract' => false, - 'is_final' => true, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => true, + 'return_type' => '', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => true, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -253,12 +259,13 @@ public function testFinalMethod() public function testProtectedReturnMethod() { $expected = [ - 'scope' => 'protected', - 'return_type' => 'int', - 'scope_specified' => true, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'protected', + 'scope_specified' => true, + 'return_type' => 'int', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -284,12 +291,13 @@ public function testProtectedReturnMethod() public function testPublicReturnMethod() { $expected = [ - 'scope' => 'public', - 'return_type' => 'array', - 'scope_specified' => true, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => true, + 'return_type' => 'array', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -307,6 +315,70 @@ public function testPublicReturnMethod() }//end testPublicReturnMethod() + /** + * Test a public method with a nullable return type. + * + * @return void + */ + public function testNullableReturnMethod() + { + $expected = [ + 'scope' => 'public', + 'scope_specified' => true, + 'return_type' => '?array', + 'nullable_return_type' => true, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + ]; + + $start = ($this->phpcsFile->numTokens - 1); + $function = $this->phpcsFile->findPrevious( + T_COMMENT, + $start, + null, + false, + '/* testNullableReturnMethod */' + ); + + $found = $this->phpcsFile->getMethodProperties(($function + 5)); + $this->assertSame($expected, $found); + + }//end testNullableReturnMethod() + + + /** + * Test a public method with a nullable return type. + * + * @return void + */ + public function testMessyNullableReturnMethod() + { + $expected = [ + 'scope' => 'public', + 'scope_specified' => true, + 'return_type' => '?array', + 'nullable_return_type' => true, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + ]; + + $start = ($this->phpcsFile->numTokens - 1); + $function = $this->phpcsFile->findPrevious( + T_COMMENT, + $start, + null, + false, + '/* testMessyNullableReturnMethod */' + ); + + $found = $this->phpcsFile->getMethodProperties(($function + 5)); + $this->assertSame($expected, $found); + + }//end testMessyNullableReturnMethod() + + /** * Test a method with a namespaced return type. * @@ -315,12 +387,13 @@ public function testPublicReturnMethod() public function testReturnNamespace() { $expected = [ - 'scope' => 'public', - 'return_type' => '\MyNamespace\MyClass', - 'scope_specified' => false, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '\MyNamespace\MyClass', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -346,12 +419,13 @@ public function testReturnNamespace() public function testReturnMultilineNamespace() { $expected = [ - 'scope' => 'public', - 'return_type' => '\MyNamespace\MyClass\Foo', - 'scope_specified' => false, - 'is_abstract' => false, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '\MyNamespace\MyClass\Foo', + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -377,12 +451,13 @@ public function testReturnMultilineNamespace() public function testAbstractMethod() { $expected = [ - 'scope' => 'public', - 'return_type' => '', - 'scope_specified' => false, - 'is_abstract' => true, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '', + 'nullable_return_type' => false, + 'is_abstract' => true, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1); @@ -408,12 +483,13 @@ public function testAbstractMethod() public function testAbstractReturnMethod() { $expected = [ - 'scope' => 'protected', - 'return_type' => 'bool', - 'scope_specified' => true, - 'is_abstract' => true, - 'is_final' => false, - 'is_static' => false, + 'scope' => 'protected', + 'scope_specified' => true, + 'return_type' => 'bool', + 'nullable_return_type' => false, + 'is_abstract' => true, + 'is_final' => false, + 'is_static' => false, ]; $start = ($this->phpcsFile->numTokens - 1);