From 6e1f05461f72210f54fc90ecb1aa4ac70d10a805 Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 13 Apr 2024 10:23:47 +0200 Subject: [PATCH] fix(FunctionComment): Ignore return statements in closures --- .../Commenting/FunctionCommentSniff.php | 287 +++++++++--------- .../Commenting/FunctionCommentUnitTest.inc | 28 +- .../FunctionCommentUnitTest.inc.fixed | 26 +- .../Commenting/FunctionCommentUnitTest.php | 1 - 4 files changed, 194 insertions(+), 148 deletions(-) diff --git a/coder_sniffer/Drupal/Sniffs/Commenting/FunctionCommentSniff.php b/coder_sniffer/Drupal/Sniffs/Commenting/FunctionCommentSniff.php index 9c7acb47..58737bc4 100644 --- a/coder_sniffer/Drupal/Sniffs/Commenting/FunctionCommentSniff.php +++ b/coder_sniffer/Drupal/Sniffs/Commenting/FunctionCommentSniff.php @@ -190,22 +190,9 @@ public function process(File $phpcsFile, $stackPtr) protected function processReturn(File $phpcsFile, $stackPtr, $commentStart) { $tokens = $phpcsFile->getTokens(); - - // Skip constructor and destructor. - $className = ''; - foreach ($tokens[$stackPtr]['conditions'] as $condPtr => $condition) { - if ($condition === T_CLASS || $condition === T_INTERFACE) { - $className = $phpcsFile->getDeclarationName($condPtr); - $className = strtolower(ltrim($className, '_')); - } - } - - $methodName = $phpcsFile->getDeclarationName($stackPtr); - $isSpecialMethod = ($methodName === '__construct' || $methodName === '__destruct'); - $methodName = strtolower(ltrim($methodName, '_')); - $return = null; $end = $stackPtr; + foreach ($tokens[$commentStart]['comment_tags'] as $pos => $tag) { if ($tokens[$tag]['content'] === '@return') { if ($return !== null) { @@ -232,150 +219,174 @@ protected function processReturn(File $phpcsFile, $stackPtr, $commentStart) } else { $end = $tokens[$commentStart]['comment_closer']; } - }//end if - }//end foreach + } + } - $type = null; - if ($isSpecialMethod === false) { - if ($return !== null) { - $type = trim($tokens[($return + 2)]['content']); - if (empty($type) === true || $tokens[($return + 2)]['code'] !== T_DOC_COMMENT_STRING) { - $error = 'Return type missing for @return tag in function comment'; - $phpcsFile->addError($error, $return, 'MissingReturnType'); - } else if (strpos($type, ' ') === false) { - // Check return type (can be multiple, separated by '|'). - $typeNames = explode('|', $type); - $suggestedNames = []; - $hasNull = false; - - foreach ($typeNames as $i => $typeName) { - if (strtolower($typeName) === 'null') { - $hasNull = true; - } + if ($return !== null) { + $returnType = trim($tokens[($return + 2)]['content']); + if (empty($returnType) === true || $tokens[($return + 2)]['code'] !== T_DOC_COMMENT_STRING) { + $error = 'Return type missing for @return tag in function comment'; + $phpcsFile->addError($error, $return, 'MissingReturnType'); + } else if (strpos($returnType, ' ') === false) { + // Check return type (can be multiple, separated by '|'). + $typeNames = explode('|', $returnType); + $suggestedNames = []; + $hasNull = false; + foreach ($typeNames as $i => $typeName) { + if (strtolower($typeName) === 'null') { + $hasNull = true; + } - $suggestedName = static::suggestType($typeName); - if (in_array($suggestedName, $suggestedNames) === false) { - $suggestedNames[] = $suggestedName; - } + $suggestedName = $this->suggestType($typeName); + if (in_array($suggestedName, $suggestedNames, true) === false) { + $suggestedNames[] = $suggestedName; } + } - $suggestedType = implode('|', $suggestedNames); - if ($type !== $suggestedType) { - $error = 'Expected "%s" but found "%s" for function return type'; - $data = [ - $suggestedType, - $type, - ]; - $fix = $phpcsFile->addFixableError($error, $return, 'InvalidReturn', $data); - if ($fix === true) { - $content = $suggestedType; - $phpcsFile->fixer->replaceToken(($return + 2), $content); + $suggestedType = implode('|', $suggestedNames); + if ($returnType !== $suggestedType) { + $error = 'Expected "%s" but found "%s" for function return type'; + $data = [ + $suggestedType, + $returnType, + ]; + $fix = $phpcsFile->addFixableError($error, $return, 'InvalidReturn', $data); + if ($fix === true) { + $replacement = $suggestedType; + + $phpcsFile->fixer->replaceToken(($return + 2), $replacement); + unset($replacement); + } + } + + // If the return type is void, make sure there is + // no return statement in the function. + if ($returnType === 'void') { + if (isset($tokens[$stackPtr]['scope_closer']) === true) { + $endToken = $tokens[$stackPtr]['scope_closer']; + for ($returnToken = $stackPtr; $returnToken < $endToken; $returnToken++) { + if ($tokens[$returnToken]['code'] === T_CLOSURE + || $tokens[$returnToken]['code'] === T_ANON_CLASS + ) { + $returnToken = $tokens[$returnToken]['scope_closer']; + continue; + } + + if ($tokens[$returnToken]['code'] === T_RETURN + || $tokens[$returnToken]['code'] === T_YIELD + || $tokens[$returnToken]['code'] === T_YIELD_FROM + ) { + break; + } } - }//end if - if ($type !== 'mixed' && $type !== 'void') { - // If return type is not void, there needs to be a return statement - // somewhere in the function that returns something. - if (isset($tokens[$stackPtr]['scope_closer']) === true) { - $endToken = $tokens[$stackPtr]['scope_closer']; - $foundReturnToken = false; - $searchStart = $stackPtr; - $foundNonVoidReturn = false; - do { - $returnToken = $phpcsFile->findNext([T_RETURN, T_YIELD, T_YIELD_FROM], $searchStart, $endToken); - if ($returnToken === false && $foundReturnToken === false) { - $error = '@return doc comment specified, but function has no return statement'; - $phpcsFile->addError($error, $return, 'InvalidNoReturn'); - } else { - // Check for return token as the last loop after the last return - // in the function will enter this else condition - // but without the returnToken. - if ($returnToken !== false) { - $foundReturnToken = true; - $semicolon = $phpcsFile->findNext(T_WHITESPACE, ($returnToken + 1), null, true); - if ($tokens[$semicolon]['code'] === T_SEMICOLON) { - // Void return is allowed if the @return type has null in it. - if ($hasNull === false) { - $error = 'Function return type is not void, but function is returning void here'; - $phpcsFile->addError($error, $returnToken, 'InvalidReturnNotVoid'); - } - } else { - $foundNonVoidReturn = true; - }//end if - - $searchStart = ($returnToken + 1); - }//end if - }//end if - } while ($returnToken !== false); - - if ($foundNonVoidReturn === false && $foundReturnToken === true) { - $error = 'Function return type is not void, but function does not have a non-void return statement'; - $phpcsFile->addError($error, $return, 'InvalidReturnNotVoid'); + if ($returnToken !== $endToken) { + // If the function is not returning anything, just + // exiting, then there is no problem. + $semicolon = $phpcsFile->findNext(T_WHITESPACE, ($returnToken + 1), null, true); + if ($tokens[$semicolon]['code'] !== T_SEMICOLON) { + $error = 'Function return type is void, but function contains return statement'; + $phpcsFile->addError($error, $return, 'InvalidReturnVoid'); } - }//end if + } }//end if - }//end if + } else if ($returnType !== 'mixed' + && $returnType !== 'never' + && in_array('void', $typeNames, true) === false + ) { + // If return type is not void, never, or mixed, there needs to be a + // return statement somewhere in the function that returns something. + if (isset($tokens[$stackPtr]['scope_closer']) === true) { + $endToken = $tokens[$stackPtr]['scope_closer']; + for ($returnToken = $stackPtr; $returnToken < $endToken; $returnToken++) { + if ($tokens[$returnToken]['code'] === T_CLOSURE + || $tokens[$returnToken]['code'] === T_ANON_CLASS + ) { + $returnToken = $tokens[$returnToken]['scope_closer']; + continue; + } - $comment = ''; - for ($i = ($return + 3); $i < $end; $i++) { - if ($tokens[$i]['code'] === T_DOC_COMMENT_STRING) { - $indent = 0; - if ($tokens[($i - 1)]['code'] === T_DOC_COMMENT_WHITESPACE) { - $indent = strlen($tokens[($i - 1)]['content']); + if ($tokens[$returnToken]['code'] === T_RETURN + || $tokens[$returnToken]['code'] === T_YIELD + || $tokens[$returnToken]['code'] === T_YIELD_FROM + ) { + break; + } } - $comment .= ' '.$tokens[$i]['content']; - $commentLines[] = [ - 'comment' => $tokens[$i]['content'], - 'token' => $i, - 'indent' => $indent, - ]; - if ($indent < 3) { - $error = 'Return comment indentation must be 3 spaces, found %s spaces'; - $fix = $phpcsFile->addFixableError($error, $i, 'ReturnCommentIndentation', [$indent]); - if ($fix === true) { - $phpcsFile->fixer->replaceToken(($i - 1), ' '); + if ($returnToken === $endToken) { + $error = 'Function return type is not void, but function has no return statement'; + $phpcsFile->addError($error, $return, 'InvalidNoReturn'); + } else { + $semicolon = $phpcsFile->findNext(T_WHITESPACE, ($returnToken + 1), null, true); + // Void return is allowed if the @return type has null in it. + if ($tokens[$semicolon]['code'] === T_SEMICOLON && $hasNull === false) { + $error = 'Function return type is not void, but function is returning void here'; + $phpcsFile->addError($error, $returnToken, 'InvalidReturnNotVoid'); } } + }//end if + }//end if + }//end if + $comment = ''; + for ($i = ($return + 3); $i < $end; $i++) { + if ($tokens[$i]['code'] === T_DOC_COMMENT_STRING) { + $indent = 0; + if ($tokens[($i - 1)]['code'] === T_DOC_COMMENT_WHITESPACE) { + $indent = strlen($tokens[($i - 1)]['content']); } - }//end for - // The first line of the comment must be indented no more than 3 - // spaces, the following lines can be more so we only check the first - // line. - if (empty($commentLines[0]['indent']) === false && $commentLines[0]['indent'] > 3) { - $error = 'Return comment indentation must be 3 spaces, found %s spaces'; - $fix = $phpcsFile->addFixableError($error, ($commentLines[0]['token'] - 1), 'ReturnCommentIndentation', [$commentLines[0]['indent']]); - if ($fix === true) { - $phpcsFile->fixer->replaceToken(($commentLines[0]['token'] - 1), ' '); + $comment .= ' '.$tokens[$i]['content']; + $commentLines[] = [ + 'comment' => $tokens[$i]['content'], + 'token' => $i, + 'indent' => $indent, + ]; + if ($indent < 3) { + $error = 'Return comment indentation must be 3 spaces, found %s spaces'; + $fix = $phpcsFile->addFixableError($error, $i, 'ReturnCommentIndentation', [$indent]); + if ($fix === true) { + $phpcsFile->fixer->replaceToken(($i - 1), ' '); + } } } + }//end for + + // The first line of the comment must be indented no more than 3 + // spaces, the following lines can be more so we only check the first + // line. + if (empty($commentLines[0]['indent']) === false && $commentLines[0]['indent'] > 3) { + $error = 'Return comment indentation must be 3 spaces, found %s spaces'; + $fix = $phpcsFile->addFixableError($error, ($commentLines[0]['token'] - 1), 'ReturnCommentIndentation', [$commentLines[0]['indent']]); + if ($fix === true) { + $phpcsFile->fixer->replaceToken(($commentLines[0]['token'] - 1), ' '); + } + } - if ($comment === '' && $type !== '$this' && $type !== 'static') { - if (strpos($type, ' ') !== false) { - $error = 'Description for the @return value must be on the next line'; - } else { - $error = 'Description for the @return value is missing'; - } - - $phpcsFile->addError($error, $return, 'MissingReturnComment'); - } else if (strpos($type, ' ') !== false) { - if (preg_match('/^([^\s]+)[\s]+(\$[^\s]+)[\s]*$/', $type, $matches) === 1) { - $error = 'Return type must not contain variable name "%s"'; - $data = [$matches[2]]; - $fix = $phpcsFile->addFixableError($error, ($return + 2), 'ReturnVarName', $data); - if ($fix === true) { - $phpcsFile->fixer->replaceToken(($return + 2), $matches[1]); - } + if ($comment === '' && $returnType !== '$this' && $returnType !== 'static') { + if (strpos($returnType, ' ') !== false) { + $error = 'Description for the @return value must be on the next line'; + } else { + $error = 'Description for the @return value is missing'; + } - // Do not check PHPStan types that contain any kind of brackets. - // See https://phpstan.org/writing-php-code/phpdoc-types#general-arrays . - } else if (preg_match('/[<\[\{\(]/', $type) === 0) { - $error = 'Return type "%s" must not contain spaces'; - $data = [$type]; - $phpcsFile->addError($error, $return, 'ReturnTypeSpaces', $data); + $phpcsFile->addError($error, $return, 'MissingReturnComment'); + } else if (strpos($returnType, ' ') !== false) { + if (preg_match('/^([^\s]+)[\s]+(\$[^\s]+)[\s]*$/', $returnType, $matches) === 1) { + $error = 'Return type must not contain variable name "%s"'; + $data = [$matches[2]]; + $fix = $phpcsFile->addFixableError($error, ($return + 2), 'ReturnVarName', $data); + if ($fix === true) { + $phpcsFile->fixer->replaceToken(($return + 2), $matches[1]); } - }//end if + + // Do not check PHPStan types that contain any kind of brackets. + // See https://phpstan.org/writing-php-code/phpdoc-types#general-arrays . + } else if (preg_match('/[<\[\{\(]/', $returnType) === 0) { + $error = 'Return type "%s" must not contain spaces'; + $data = [$returnType]; + $phpcsFile->addError($error, $return, 'ReturnTypeSpaces', $data); + } }//end if }//end if diff --git a/tests/Drupal/Commenting/FunctionCommentUnitTest.inc b/tests/Drupal/Commenting/FunctionCommentUnitTest.inc index 2db543b5..b005dd4c 100644 --- a/tests/Drupal/Commenting/FunctionCommentUnitTest.inc +++ b/tests/Drupal/Commenting/FunctionCommentUnitTest.inc @@ -276,11 +276,11 @@ function test23() { } /** - * Even when null is given in @return there must be at least 1 valid return. + * Don't test if the return type was actually fulfilling all options. * - * When null is a potential return value it should be allowed to have return - * statements with void return. This does not mean that all returns can be void. - * There must be at least one non-void return. + * Even if there is bool specified we don't care if it is actually used or not. + * Other tools such as PHPStan should be used to validate the return types. + * This is fine! * * @return bool|null * This implies that the return value can be NULL, a boolean, or empty. @@ -957,4 +957,22 @@ class Test43 { public function __construct() { } -} \ No newline at end of file +} + +/** + * Closure return statements with different types are allowed. + * + * @return array + * Some array. + */ +function return_some_array(): array { + do_something_with_a_callback(function () { + if ($some_condition) { + // Early return. + return; + } + + // Do work. + }); + return []; +} diff --git a/tests/Drupal/Commenting/FunctionCommentUnitTest.inc.fixed b/tests/Drupal/Commenting/FunctionCommentUnitTest.inc.fixed index da234c6c..817400ca 100644 --- a/tests/Drupal/Commenting/FunctionCommentUnitTest.inc.fixed +++ b/tests/Drupal/Commenting/FunctionCommentUnitTest.inc.fixed @@ -288,11 +288,11 @@ function test23() { } /** - * Even when null is given in @return there must be at least 1 valid return. + * Don't test if the return type was actually fulfilling all options. * - * When null is a potential return value it should be allowed to have return - * statements with void return. This does not mean that all returns can be void. - * There must be at least one non-void return. + * Even if there is bool specified we don't care if it is actually used or not. + * Other tools such as PHPStan should be used to validate the return types. + * This is fine! * * @return bool|null * This implies that the return value can be NULL, a boolean, or empty. @@ -984,3 +984,21 @@ class Test43 { } } + +/** + * Closure return statements with different types are allowed. + * + * @return array + * Some array. + */ +function return_some_array(): array { + do_something_with_a_callback(function () { + if ($some_condition) { + // Early return. + return; + } + + // Do work. + }); + return []; +} diff --git a/tests/Drupal/Commenting/FunctionCommentUnitTest.php b/tests/Drupal/Commenting/FunctionCommentUnitTest.php index 7dbf8269..b32119ce 100644 --- a/tests/Drupal/Commenting/FunctionCommentUnitTest.php +++ b/tests/Drupal/Commenting/FunctionCommentUnitTest.php @@ -52,7 +52,6 @@ protected function getErrorList(string $testFile): array 252 => 1, 254 => 1, 256 => 1, - 285 => 1, 298 => 1, 308 => 1, 311 => 1,