Skip to content

Commit

Permalink
fix(FunctionComment): Ignore return statements in closures
Browse files Browse the repository at this point in the history
  • Loading branch information
@klausi
klausi committed Apr 13, 2024
1 parent 496c130 commit 6e1f054
Show file tree
Hide file tree
Showing 4 changed files with 194 additions and 148 deletions.
287 changes: 149 additions & 138 deletions coder_sniffer/Drupal/Sniffs/Commenting/FunctionCommentSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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

Expand Down
28 changes: 23 additions & 5 deletions tests/Drupal/Commenting/FunctionCommentUnitTest.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -957,4 +957,22 @@ class Test43 {
public function __construct() {
}

}
}

/**
* 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 [];
}
Loading

0 comments on commit 6e1f054

Please sign in to comment.