diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index bc354b56..3e72f1f2 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -10,6 +10,7 @@ namespace WordPressVIPMinimum\Sniffs\Hooks; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\Arrays; use PHPCSUtils\Utils\Conditions; use PHPCSUtils\Utils\FunctionDeclarations; @@ -228,14 +229,9 @@ private function processFunctionBody( $stackPtr ): void { $functionBodyScopeEnd ); - $outsideConditionalReturn = 0; - while ( $returnTokenPtr ) { // A return in a nested closure or function does not return from the callback. if ( Conditions::getLastCondition( $this->phpcsFile, $returnTokenPtr, [ T_FUNCTION, T_CLOSURE ] ) === $stackPtr ) { - if ( $this->isInsideIfConditonal( $returnTokenPtr, $stackPtr ) === false ) { - ++$outsideConditionalReturn; - } if ( $this->isReturningVoid( $returnTokenPtr ) ) { $message = 'Please, make sure that a callback to `%s` filter is returning void intentionally.'; $data = [ $filterName ]; @@ -249,7 +245,7 @@ private function processFunctionBody( $stackPtr ): void { ); } - if ( $outsideConditionalReturn === 0 ) { + if ( $this->alwaysReturns( $functionBodyScopeStart, $functionBodyScopeEnd ) === false ) { if ( $this->hasTerminatingStatement( $functionBodyScopeStart, $functionBodyScopeEnd ) ) { $message = 'The callback for the `%s` filter uses a terminating statement (`exit`, `die`, or `throw`) instead of returning a value. Filter callbacks should always return a value.'; $data = [ $filterName ]; @@ -263,26 +259,123 @@ private function processFunctionBody( $stackPtr ): void { } /** - * Is the current token inside a conditional within the function? + * Does every path through the code between the scope opener and closer reach a return? * - * @param int $stackPtr The position in the stack where the token was found. - * @param int $functionPtr The position of the function the token belongs to. + * Only if, elseif and else are followed as separate paths. A return inside any other + * control structure, such as a loop, switch or try, counts as reaching a return. + * + * @param int $scopeOpener The scope opener. + * @param int $scopeCloser The scope closer. * * @return bool */ - private function isInsideIfConditonal( $stackPtr, $functionPtr ): bool { + private function alwaysReturns( $scopeOpener, $scopeCloser ): bool { - // Only look inside the function, so a class or a function wrapped inside - // a conditional as a whole does not count. Eg.: inside `function_exists`. - foreach ( $this->tokens[ $stackPtr ]['conditions'] as $conditionPtr => $conditionCode ) { - if ( $conditionPtr > $functionPtr && $conditionCode === T_IF ) { + for ( $i = $scopeOpener + 1; $i < $scopeCloser; $i++ ) { + if ( $this->tokens[ $i ]['code'] === T_RETURN ) { return true; } + + if ( isset( $this->tokens[ $i ]['scope_condition'], $this->tokens[ $i ]['scope_opener'], $this->tokens[ $i ]['scope_closer'] ) === false + || $this->tokens[ $i ]['scope_condition'] !== $i + ) { + continue; + } + + if ( $this->isNestedScope( $i ) ) { + // A return in a nested function or class does not return from the callback. + $i = $this->tokens[ $i ]['scope_closer']; + continue; + } + + if ( $this->tokens[ $i ]['code'] === T_IF ) { + // An if chain only returns on every path when it has an else and every branch returns. + $branches = $this->getIfChainBranches( $i ); + $lastBranch = end( $branches ); + $returns = $this->tokens[ $lastBranch ]['code'] === T_ELSE; + foreach ( $branches as $branch ) { + $returns = $returns && $this->alwaysReturns( $this->tokens[ $branch ]['scope_opener'], $this->tokens[ $branch ]['scope_closer'] ); + } + + if ( $returns ) { + return true; + } + + $i = $this->tokens[ $lastBranch ]['scope_closer']; + continue; + } + + if ( $this->alwaysReturns( $this->tokens[ $i ]['scope_opener'], $this->tokens[ $i ]['scope_closer'] ) ) { + return true; + } + + $i = $this->tokens[ $i ]['scope_closer']; } return false; } + /** + * Get the if, elseif and else branches of an if chain. + * + * A branch without braces ends the chain, as it has no scope to follow. + * + * @param int $ifPtr The position of the if. + * + * @return array The positions of the branches, in order. + */ + private function getIfChainBranches( $ifPtr ): array { + + $branches = []; + $branch = $ifPtr; + while ( isset( $this->tokens[ $branch ]['scope_opener'], $this->tokens[ $branch ]['scope_closer'] ) ) { + $branches[] = $branch; + if ( $this->tokens[ $branch ]['code'] === T_ELSE ) { + break; + } + + // With the alternative syntax, a branch closes on the next elseif or else. + $next = $this->tokens[ $branch ]['scope_closer']; + if ( $this->tokens[ $next ]['code'] !== T_ELSEIF && $this->tokens[ $next ]['code'] !== T_ELSE ) { + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next + 1, null, true ); + } + + if ( $next !== false + && $this->tokens[ $next ]['code'] === T_ELSE + && isset( $this->tokens[ $next ]['scope_opener'] ) === false + ) { + // An `else if` is an else without braces, followed by an if. + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next + 1, null, true ); + if ( $next === false || $this->tokens[ $next ]['code'] !== T_IF ) { + break; + } + } elseif ( $next === false + || ( $this->tokens[ $next ]['code'] !== T_ELSEIF && $this->tokens[ $next ]['code'] !== T_ELSE ) + ) { + break; + } + + $branch = $next; + } + + return $branches; + } + + /** + * Is the token a nested function or class, whose code does not belong to the callback? + * + * @param int $stackPtr The position in the stack where the token was found. + * + * @return bool + */ + private function isNestedScope( $stackPtr ): bool { + + $code = $this->tokens[ $stackPtr ]['code']; + + return isset( $this->tokens[ $stackPtr ]['scope_closer'] ) + && ( isset( Collections::functionDeclarationTokens()[ $code ] ) || isset( Tokens::$ooScopeTokens[ $code ] ) ); + } + /** * Check whether the function body contains an exit, die, or throw statement. * @@ -293,13 +386,19 @@ private function isInsideIfConditonal( $stackPtr, $functionPtr ): bool { */ private function hasTerminatingStatement( $scopeStart, $scopeEnd ): bool { - $terminatingPtr = $this->phpcsFile->findNext( - [ T_EXIT, T_THROW ], - $scopeStart + 1, - $scopeEnd - ); + for ( $i = $scopeStart + 1; $i < $scopeEnd; $i++ ) { + if ( $this->isNestedScope( $i ) ) { + // An exit or throw in a nested function or class does not end the callback. + $i = $this->tokens[ $i ]['scope_closer']; + continue; + } + + if ( $this->tokens[ $i ]['code'] === T_EXIT || $this->tokens[ $i ]['code'] === T_THROW ) { + return true; + } + } - return $terminatingPtr !== false; + return false; } /** diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc index b3eb4e5e..44e0d764 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc @@ -276,6 +276,90 @@ add_filter( 'nested_class_return', function( $input ) { // Error. }; } ); +// A return in an else or elseif is only reached on some paths. +add_filter( 'else_only_return', function( $input ) { // Error. + if ( is_admin() ) { + $input = 'admin'; + } else { + return $input; + } +} ); + +add_filter( 'elseif_without_else', function( $input ) { // Error. + if ( is_admin() ) { + return 'admin'; + } elseif ( is_feed() ) { + return 'feed'; + } +} ); + +add_filter( 'every_branch_returns', function( $input ) { // Ok. + if ( is_admin() ) { + return 'admin'; + } elseif ( is_feed() ) { + return 'feed'; + } else { + return $input; + } +} ); + +add_filter( 'every_branch_returns_else_if', function( $input ) { // Ok. + if ( is_admin() ) { + return 'admin'; + } else if ( is_feed() ) { + return 'feed'; + } else { + return $input; + } +} ); + +add_filter( 'every_branch_returns_alternative_syntax', function( $input ) { // Ok. + if ( is_admin() ) : + return 'admin'; + elseif ( is_feed() ) : + return 'feed'; + else : + return $input; + endif; +} ); + +add_filter( 'alternative_syntax_missing_else', function( $input ) { // Error. + if ( is_admin() ) : + return 'admin'; + elseif ( is_feed() ) : + return 'feed'; + endif; +} ); + +add_filter( 'nested_chain_in_else', function( $input ) { // Ok. + if ( is_admin() ) { + return 'admin'; + } else { + if ( is_feed() ) { + return 'feed'; + } else { + return $input; + } + } +} ); + +// An exit or throw in a nested closure does not end the callback. +add_filter( 'nested_closure_throw', function( $input ) { // Error. + if ( is_admin() ) { + return $input; + } + add_action( 'shutdown', function() { + throw new \RuntimeException( 'Failed' ); + } ); +} ); + +add_filter( 'nested_arrow_function_throw', function( $input ) { // Error. + if ( is_admin() ) { + return $input; + } + $fail = fn() => throw new \RuntimeException( 'Failed' ); +} ); + // Intentional parse error. This has to be the last test in the file! class parse_error_test { public function __construct() { diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php index c954f095..e2eaad9b 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php @@ -35,6 +35,11 @@ public function getErrorList() { 241 => 1, 258 => 1, 271 => 1, + 280 => 1, + 288 => 1, + 326 => 1, + 347 => 1, + 356 => 1, ]; }