Require every if/else path to return in AlwaysReturnInFilter

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<int> 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;
 	}
 
 	/**

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() {

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,
 		];
 	}