Performance/CacheValueOverride: bug fix - false positives for variables in different scopes

As things were, the searching for the "next use of the variable to which the cache was assigned" did not limit itself to the current variable scope.

In effect, that meant that "function local" variables could be mistaken for global variables and visa versa, as demonstrated by the new tests.

Fixed now.

Includes tests.

Author: jrfnl

WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php

@@ -10,6 +10,8 @@
 namespace WordPressVIPMinimum\Sniffs\Performance;
 
 use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Tokens\Collections;
+use PHPCSUtils\Utils\Conditions;
 use WordPressCS\WordPress\AbstractFunctionRestrictionsSniff;
 
 /**
@@ -71,16 +73,40 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content
 		$variableToken = $this->tokens[ $variablePos ];
 		$variableName  = $variableToken['content'];
 
-		$nextVariableOccurrence = $this->phpcsFile->findNext( T_VARIABLE, $closeBracket + 1, null, false, $variableName );
+		// Figure out the scope we need to search in.
+		$searchEnd   = $this->phpcsFile->numTokens;
+		$functionPtr = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, [ T_FUNCTION, T_CLOSURE ] );
+		if ( $functionPtr !== false && isset( $this->tokens[ $functionPtr ]['scope_closer'] ) ) {
+			$searchEnd = $this->tokens[ $functionPtr ]['scope_closer'];
+		}
+
+		$nextVariableOccurrence = false;
+		for ( $i = $closeBracket + 1; $i < $searchEnd; $i++ ) {
+			if ( $this->tokens[ $i ]['code'] === T_VARIABLE && $this->tokens[ $i ]['content'] === $variableName ) {
+				$nextVariableOccurrence = $i;
+				break;
+			}
+
+			// Skip over any and all closed scopes.
+			if ( isset( Collections::closedScopes()[ $this->tokens[ $i ]['code'] ] ) ) {
+				if ( isset( $this->tokens[ $i ]['scope_closer'] ) ) {
+					$i = $this->tokens[ $i ]['scope_closer'];
+				}
+			}
+		}
+
+		if ( $nextVariableOccurrence === false ) {
+			return;
+		}
 
-		$rightAfterNextVariableOccurence = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextVariableOccurrence + 1, null, true, null, true );
+		$rightAfterNextVariableOccurence = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextVariableOccurrence + 1, $searchEnd, true, null, true );
 
 		if ( $this->tokens[ $rightAfterNextVariableOccurence ]['code'] !== T_EQUAL ) {
 			// Not a value override.
 			return;
 		}
 
-		$valueAfterEqualSign = $this->phpcsFile->findNext( Tokens::$emptyTokens, $rightAfterNextVariableOccurence + 1, null, true, null, true );
+		$valueAfterEqualSign = $this->phpcsFile->findNext( Tokens::$emptyTokens, $rightAfterNextVariableOccurence + 1, $searchEnd, true, null, true );
 
 		if ( $this->tokens[ $valueAfterEqualSign ]['code'] === T_FALSE ) {
 			$message = 'Obtained cached value in `%s` is being overridden. Disabling caching?';

WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.1.inc

@@ -89,3 +89,39 @@ function firstClassCallable() {
 	$callable = wp_cache_get(...);
 	$callable = false; // OK.
 }
+
+// Bug fix - the variable re-assignment must be in the same scope.
+function controlStructuresAreNotClosedScopes() {
+	if (false === ($cache = \wp_cache_get( $key, $group ))) {
+		// Create new cache.
+	}
+	$cache = false; // Bad.
+}
+
+
+function closedScopeA() {
+	$myCache = \wp_cache_get();
+}
+
+function closedScopeB($myCache = false) {} // OK.
+
+function closedScopeC() {
+	$yourCache = wp_cache_get();
+
+	$closure = function () {
+		$yourCache = false; // OK.
+	};
+}
+
+$globalScope = wp_cache_get();
+function closedScopeD() {
+	$globalScope = false; // OK.
+}
+
+$globalScope = wp_cache_get();
+function closedScopeE($globalScope = false;) {} // OK.
+
+$globalScope = wp_cache_get();
+class FooBar {
+	function method($globalScope = false) {} // OK.
+}

WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.php

@@ -31,6 +31,7 @@ public function getErrorList( $testFile = '' ) {
 					68 => 1,
 					78 => 1,
 					83 => 1,
+					98 => 1,
 				];
 
 			default: