Skip to content

Commit 24bad88

Browse files
klausiklausi
authored
fix(FunctionComment): Ignore return statements in closures (#3427690)
1 parent 496c130 commit 24bad88

File tree

4 files changed

+193
-146
lines changed

4 files changed

+193
-146
lines changed

‎coder_sniffer/Drupal/Sniffs/Commenting/FunctionCommentSniff.php‎

Lines changed: 148 additions & 136 deletions
Original file line numberDiff line numberDiff line change
@@ -190,22 +190,9 @@ public function process(File $phpcsFile, $stackPtr)
190190
protected function processReturn(File $phpcsFile, $stackPtr, $commentStart)
191191
{
192192
$tokens = $phpcsFile->getTokens();
193-
194-
// Skip constructor and destructor.
195-
$className = '';
196-
foreach ($tokens[$stackPtr]['conditions'] as $condPtr => $condition) {
197-
if ($condition === T_CLASS || $condition === T_INTERFACE) {
198-
$className = $phpcsFile->getDeclarationName($condPtr);
199-
$className = strtolower(ltrim($className, '_'));
200-
}
201-
}
202-
203-
$methodName = $phpcsFile->getDeclarationName($stackPtr);
204-
$isSpecialMethod = ($methodName === '__construct' || $methodName === '__destruct');
205-
$methodName = strtolower(ltrim($methodName, '_'));
206-
207193
$return = null;
208194
$end = $stackPtr;
195+
209196
foreach ($tokens[$commentStart]['comment_tags'] as $pos => $tag) {
210197
if ($tokens[$tag]['content'] === '@return') {
211198
if ($return !== null) {
@@ -235,147 +222,172 @@ protected function processReturn(File $phpcsFile, $stackPtr, $commentStart)
235222
}//end if
236223
}//end foreach
237224

238-
$type = null;
239-
if ($isSpecialMethod === false) {
240-
if ($return !== null) {
241-
$type = trim($tokens[($return + 2)]['content']);
242-
if (empty($type) === true || $tokens[($return + 2)]['code'] !== T_DOC_COMMENT_STRING) {
243-
$error = 'Return type missing for @return tag in function comment';
244-
$phpcsFile->addError($error, $return, 'MissingReturnType');
245-
} else if (strpos($type, ' ') === false) {
246-
// Check return type (can be multiple, separated by '|').
247-
$typeNames = explode('|', $type);
248-
$suggestedNames = [];
249-
$hasNull = false;
250-
251-
foreach ($typeNames as $i => $typeName) {
252-
if (strtolower($typeName) === 'null') {
253-
$hasNull = true;
254-
}
225+
if ($return !== null) {
226+
$returnType = trim($tokens[($return + 2)]['content']);
227+
if (empty($returnType) === true || $tokens[($return + 2)]['code'] !== T_DOC_COMMENT_STRING) {
228+
$error = 'Return type missing for @return tag in function comment';
229+
$phpcsFile->addError($error, $return, 'MissingReturnType');
230+
} else if (strpos($returnType, ' ') === false) {
231+
// Check return type (can be multiple, separated by '|').
232+
$typeNames = explode('|', $returnType);
233+
$suggestedNames = [];
234+
$hasNull = false;
235+
foreach ($typeNames as $i => $typeName) {
236+
if (strtolower($typeName) === 'null') {
237+
$hasNull = true;
238+
}
255239

256-
$suggestedName = static::suggestType($typeName);
257-
if (in_array($suggestedName, $suggestedNames) === false) {
258-
$suggestedNames[] = $suggestedName;
259-
}
240+
$suggestedName = $this->suggestType($typeName);
241+
if (in_array($suggestedName, $suggestedNames, true) === false) {
242+
$suggestedNames[] = $suggestedName;
260243
}
244+
}
261245

262-
$suggestedType = implode('|', $suggestedNames);
263-
if ($type !== $suggestedType) {
264-
$error = 'Expected "%s" but found "%s" for function return type';
265-
$data = [
266-
$suggestedType,
267-
$type,
268-
];
269-
$fix = $phpcsFile->addFixableError($error, $return, 'InvalidReturn', $data);
270-
if ($fix === true) {
271-
$content = $suggestedType;
272-
$phpcsFile->fixer->replaceToken(($return + 2), $content);
246+
$suggestedType = implode('|', $suggestedNames);
247+
if ($returnType !== $suggestedType) {
248+
$error = 'Expected "%s" but found "%s" for function return type';
249+
$data = [
250+
$suggestedType,
251+
$returnType,
252+
];
253+
$fix = $phpcsFile->addFixableError($error, $return, 'InvalidReturn', $data);
254+
if ($fix === true) {
255+
$replacement = $suggestedType;
256+
257+
$phpcsFile->fixer->replaceToken(($return + 2), $replacement);
258+
unset($replacement);
259+
}
260+
}
261+
262+
// If the return type is void, make sure there is
263+
// no return statement in the function.
264+
if ($returnType === 'void') {
265+
if (isset($tokens[$stackPtr]['scope_closer']) === true) {
266+
$endToken = $tokens[$stackPtr]['scope_closer'];
267+
for ($returnToken = $stackPtr; $returnToken < $endToken; $returnToken++) {
268+
if ($tokens[$returnToken]['code'] === T_CLOSURE
269+
|| $tokens[$returnToken]['code'] === T_ANON_CLASS
270+
) {
271+
$returnToken = $tokens[$returnToken]['scope_closer'];
272+
continue;
273+
}
274+
275+
if ($tokens[$returnToken]['code'] === T_RETURN
276+
|| $tokens[$returnToken]['code'] === T_YIELD
277+
|| $tokens[$returnToken]['code'] === T_YIELD_FROM
278+
) {
279+
break;
280+
}
273281
}
274-
}//end if
275282

276-
if ($type !== 'mixed' && $type !== 'void') {
277-
// If return type is not void, there needs to be a return statement
278-
// somewhere in the function that returns something.
279-
if (isset($tokens[$stackPtr]['scope_closer']) === true) {
280-
$endToken = $tokens[$stackPtr]['scope_closer'];
281-
$foundReturnToken = false;
282-
$searchStart = $stackPtr;
283-
$foundNonVoidReturn = false;
284-
do {
285-
$returnToken = $phpcsFile->findNext([T_RETURN, T_YIELD, T_YIELD_FROM], $searchStart, $endToken);
286-
if ($returnToken === false && $foundReturnToken === false) {
287-
$error = '@return doc comment specified, but function has no return statement';
288-
$phpcsFile->addError($error, $return, 'InvalidNoReturn');
289-
} else {
290-
// Check for return token as the last loop after the last return
291-
// in the function will enter this else condition
292-
// but without the returnToken.
293-
if ($returnToken !== false) {
294-
$foundReturnToken = true;
295-
$semicolon = $phpcsFile->findNext(T_WHITESPACE, ($returnToken + 1), null, true);
296-
if ($tokens[$semicolon]['code'] === T_SEMICOLON) {
297-
// Void return is allowed if the @return type has null in it.
298-
if ($hasNull === false) {
299-
$error = 'Function return type is not void, but function is returning void here';
300-
$phpcsFile->addError($error, $returnToken, 'InvalidReturnNotVoid');
301-
}
302-
} else {
303-
$foundNonVoidReturn = true;
304-
}//end if
305-
306-
$searchStart = ($returnToken + 1);
307-
}//end if
308-
}//end if
309-
} while ($returnToken !== false);
310-
311-
if ($foundNonVoidReturn === false && $foundReturnToken === true) {
312-
$error = 'Function return type is not void, but function does not have a non-void return statement';
313-
$phpcsFile->addError($error, $return, 'InvalidReturnNotVoid');
283+
if ($returnToken !== $endToken) {
284+
// If the function is not returning anything, just
285+
// exiting, then there is no problem.
286+
$semicolon = $phpcsFile->findNext(T_WHITESPACE, ($returnToken + 1), null, true);
287+
if ($tokens[$semicolon]['code'] !== T_SEMICOLON) {
288+
$error = 'Function return type is void, but function contains return statement';
289+
$phpcsFile->addError($error, $return, 'InvalidReturnVoid');
314290
}
315-
}//end if
291+
}
316292
}//end if
317-
}//end if
293+
} else if ($returnType !== 'mixed'
294+
&& $returnType !== 'never'
295+
&& in_array('void', $typeNames, true) === false
296+
) {
297+
// If return type is not void, never, or mixed, there needs to be a
298+
// return statement somewhere in the function that returns something.
299+
if (isset($tokens[$stackPtr]['scope_closer']) === true) {
300+
$endToken = $tokens[$stackPtr]['scope_closer'];
301+
for ($returnToken = $stackPtr; $returnToken < $endToken; $returnToken++) {
302+
if ($tokens[$returnToken]['code'] === T_CLOSURE
303+
|| $tokens[$returnToken]['code'] === T_ANON_CLASS
304+
) {
305+
$returnToken = $tokens[$returnToken]['scope_closer'];
306+
continue;
307+
}
318308

319-
$comment = '';
320-
for ($i = ($return + 3); $i < $end; $i++) {
321-
if ($tokens[$i]['code'] === T_DOC_COMMENT_STRING) {
322-
$indent = 0;
323-
if ($tokens[($i - 1)]['code'] === T_DOC_COMMENT_WHITESPACE) {
324-
$indent = strlen($tokens[($i - 1)]['content']);
309+
if ($tokens[$returnToken]['code'] === T_RETURN
310+
|| $tokens[$returnToken]['code'] === T_YIELD
311+
|| $tokens[$returnToken]['code'] === T_YIELD_FROM
312+
) {
313+
break;
314+
}
325315
}
326316

327-
$comment .= ' '.$tokens[$i]['content'];
328-
$commentLines[] = [
329-
'comment' => $tokens[$i]['content'],
330-
'token' => $i,
331-
'indent' => $indent,
332-
];
333-
if ($indent < 3) {
334-
$error = 'Return comment indentation must be 3 spaces, found %s spaces';
335-
$fix = $phpcsFile->addFixableError($error, $i, 'ReturnCommentIndentation', [$indent]);
336-
if ($fix === true) {
337-
$phpcsFile->fixer->replaceToken(($i - 1), ' ');
317+
if ($returnToken === $endToken) {
318+
$error = 'Function return type is not void, but function has no return statement';
319+
$phpcsFile->addError($error, $return, 'InvalidNoReturn');
320+
} else {
321+
$semicolon = $phpcsFile->findNext(T_WHITESPACE, ($returnToken + 1), null, true);
322+
// Void return is allowed if the @return type has null in it.
323+
if ($tokens[$semicolon]['code'] === T_SEMICOLON && $hasNull === false) {
324+
$error = 'Function return type is not void, but function is returning void here';
325+
$phpcsFile->addError($error, $returnToken, 'InvalidReturnNotVoid');
338326
}
339327
}
340-
}
341-
}//end for
342-
343-
// The first line of the comment must be indented no more than 3
344-
// spaces, the following lines can be more so we only check the first
345-
// line.
346-
if (empty($commentLines[0]['indent']) === false && $commentLines[0]['indent'] > 3) {
347-
$error = 'Return comment indentation must be 3 spaces, found %s spaces';
348-
$fix = $phpcsFile->addFixableError($error, ($commentLines[0]['token'] - 1), 'ReturnCommentIndentation', [$commentLines[0]['indent']]);
349-
if ($fix === true) {
350-
$phpcsFile->fixer->replaceToken(($commentLines[0]['token'] - 1), ' ');
351-
}
352-
}
328+
}//end if
329+
}//end if
330+
}//end if
353331

354-
if ($comment === '' && $type !== '$this' && $type !== 'static') {
355-
if (strpos($type, ' ') !== false) {
356-
$error = 'Description for the @return value must be on the next line';
357-
} else {
358-
$error = 'Description for the @return value is missing';
332+
$comment = '';
333+
for ($i = ($return + 3); $i < $end; $i++) {
334+
if ($tokens[$i]['code'] === T_DOC_COMMENT_STRING) {
335+
$indent = 0;
336+
if ($tokens[($i - 1)]['code'] === T_DOC_COMMENT_WHITESPACE) {
337+
$indent = strlen($tokens[($i - 1)]['content']);
359338
}
360339

361-
$phpcsFile->addError($error, $return, 'MissingReturnComment');
362-
} else if (strpos($type, ' ') !== false) {
363-
if (preg_match('/^([^\s]+)[\s]+(\$[^\s]+)[\s]*$/', $type, $matches) === 1) {
364-
$error = 'Return type must not contain variable name "%s"';
365-
$data = [$matches[2]];
366-
$fix = $phpcsFile->addFixableError($error, ($return + 2), 'ReturnVarName', $data);
340+
$comment .= ' '.$tokens[$i]['content'];
341+
$commentLines[] = [
342+
'comment' => $tokens[$i]['content'],
343+
'token' => $i,
344+
'indent' => $indent,
345+
];
346+
if ($indent < 3) {
347+
$error = 'Return comment indentation must be 3 spaces, found %s spaces';
348+
$fix = $phpcsFile->addFixableError($error, $i, 'ReturnCommentIndentation', [$indent]);
367349
if ($fix === true) {
368-
$phpcsFile->fixer->replaceToken(($return + 2), $matches[1]);
350+
$phpcsFile->fixer->replaceToken(($i - 1), ' ');
369351
}
352+
}
353+
}
354+
}//end for
355+
356+
// The first line of the comment must be indented no more than 3
357+
// spaces, the following lines can be more so we only check the first
358+
// line.
359+
if (empty($commentLines[0]['indent']) === false && $commentLines[0]['indent'] > 3) {
360+
$error = 'Return comment indentation must be 3 spaces, found %s spaces';
361+
$fix = $phpcsFile->addFixableError($error, ($commentLines[0]['token'] - 1), 'ReturnCommentIndentation', [$commentLines[0]['indent']]);
362+
if ($fix === true) {
363+
$phpcsFile->fixer->replaceToken(($commentLines[0]['token'] - 1), ' ');
364+
}
365+
}
366+
367+
if ($comment === '' && $returnType !== '$this' && $returnType !== 'static') {
368+
if (strpos($returnType, ' ') !== false) {
369+
$error = 'Description for the @return value must be on the next line';
370+
} else {
371+
$error = 'Description for the @return value is missing';
372+
}
370373

371-
// Do not check PHPStan types that contain any kind of brackets.
372-
// See https://phpstan.org/writing-php-code/phpdoc-types#general-arrays .
373-
} else if (preg_match('/[<\[\{\(]/', $type) === 0) {
374-
$error = 'Return type "%s" must not contain spaces';
375-
$data = [$type];
376-
$phpcsFile->addError($error, $return, 'ReturnTypeSpaces', $data);
374+
$phpcsFile->addError($error, $return, 'MissingReturnComment');
375+
} else if (strpos($returnType, ' ') !== false) {
376+
if (preg_match('/^([^\s]+)[\s]+(\$[^\s]+)[\s]*$/', $returnType, $matches) === 1) {
377+
$error = 'Return type must not contain variable name "%s"';
378+
$data = [$matches[2]];
379+
$fix = $phpcsFile->addFixableError($error, ($return + 2), 'ReturnVarName', $data);
380+
if ($fix === true) {
381+
$phpcsFile->fixer->replaceToken(($return + 2), $matches[1]);
377382
}
378-
}//end if
383+
384+
// Do not check PHPStan types that contain any kind of brackets.
385+
// See https://phpstan.org/writing-php-code/phpdoc-types#general-arrays .
386+
} else if (preg_match('/[<\[\{\(]/', $returnType) === 0) {
387+
$error = 'Return type "%s" must not contain spaces';
388+
$data = [$returnType];
389+
$phpcsFile->addError($error, $return, 'ReturnTypeSpaces', $data);
390+
}
379391
}//end if
380392
}//end if
381393

‎tests/Drupal/Commenting/FunctionCommentUnitTest.inc‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -276,11 +276,11 @@ function test23() {
276276
}
277277

278278
/**
279-
* Even when null is given in @return there must be at least 1 valid return.
279+
* Don't test if the return type was actually fulfilling all options.
280280
*
281-
* When null is a potential return value it should be allowed to have return
282-
* statements with void return. This does not mean that all returns can be void.
283-
* There must be at least one non-void return.
281+
* Even if there is bool specified we don't care if it is actually used or not.
282+
* Other tools such as PHPStan should be used to validate the return types.
283+
* This is fine!
284284
*
285285
* @return bool|null
286286
* This implies that the return value can be NULL, a boolean, or empty.
@@ -957,4 +957,22 @@ class Test43 {
957957
public function __construct() {
958958
}
959959

960-
}
960+
}
961+
962+
/**
963+
* Closure return statements with different types are allowed.
964+
*
965+
* @return array
966+
* Some array.
967+
*/
968+
function return_some_array(): array {
969+
do_something_with_a_callback(function () {
970+
if ($some_condition) {
971+
// Early return.
972+
return;
973+
}
974+
975+
// Do work.
976+
});
977+
return [];
978+
}

0 commit comments

Comments
 (0)