Performance/CacheValueOverride: various improvements

WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php

@@ -10,46 +10,61 @@
 namespace WordPressVIPMinimum\Sniffs\Performance;
 
 use PHP_CodeSniffer\Util\Tokens;
-use WordPressVIPMinimum\Sniffs\Sniff;
+use PHPCSUtils\Tokens\Collections;
+use PHPCSUtils\Utils\Conditions;
+use WordPressCS\WordPress\AbstractFunctionRestrictionsSniff;
 
 /**
  * This sniff check whether a cached value is being overridden.
  */
-class CacheValueOverrideSniff extends Sniff {
+class CacheValueOverrideSniff extends AbstractFunctionRestrictionsSniff {
 
 	/**
-	 * Returns the token types that this sniff is interested in.
+	 * Groups of functions to restrict.
 	 *
-	 * @return array<int|string>
+	 * @return array<string, array<string, array<string>>>
 	 */
-	public function register() {
-		return [ T_STRING ];
+	public function getGroups() {
+		return [
+			'wp_cache_get' => [
+				'functions' => [ 'wp_cache_get' ],
+			],
+		];
 	}
 
-
 	/**
-	 * Processes the tokens that this sniff is interested in.
+	 * Process a matched token.
 	 *
-	 * @param int $stackPtr The position in the stack where the token was found.
+	 * @param int    $stackPtr        The position of the current token in the stack.
+	 * @param string $group_name      The name of the group which was matched.
+	 * @param string $matched_content The token content (function name) which was matched
+	 *                                in lowercase.
 	 *
 	 * @return void
 	 */
-	public function process_token( $stackPtr ) {
-
-		$functionName = $this->tokens[ $stackPtr ]['content'];
+	public function process_matched_token( $stackPtr, $group_name, $matched_content ) {
+		$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+		if ( $openBracket === false || isset( $this->tokens[ $openBracket ]['parenthesis_closer'] ) === false ) {
+			// Import use statement for function or parse error/live coding. Ignore.
+			return;
+		}
 
-		if ( $functionName !== 'wp_cache_get' ) {
-			// Not a function we are looking for.
+		$closeBracket  = $this->tokens[ $openBracket ]['parenthesis_closer'];
+		$firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $openBracket + 1 ), null, true );
+		$nextNonEmpty  = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $firstNonEmpty + 1 ), null, true );
+		if ( $nextNonEmpty === false ) {
+			// Parse error/live coding. Ignore.
 			return;
 		}
 
-		if ( $this->isFunctionCall( $stackPtr ) === false ) {
-			// Not a function call.
+		if ( $this->tokens[ $firstNonEmpty ]['code'] === T_ELLIPSIS
+			&& $nextNonEmpty === $closeBracket
+		) {
+			// First class callable. Ignore.
 			return;
 		}
 
 		$variablePos = $this->isVariableAssignment( $stackPtr );
-
 		if ( $variablePos === false ) {
 			// Not a variable assignment.
 			return;
@@ -58,56 +73,50 @@ public function process_token( $stackPtr ) {
 		$variableToken = $this->tokens[ $variablePos ];
 		$variableName  = $variableToken['content'];
 
-		// Find the next non-empty token.
-		$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
+		// 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'];
+		}
 
-		// Find the closing bracket.
-		$closeBracket = $this->tokens[ $openBracket ]['parenthesis_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'];
+				}
+			}
+		}
 
-		$nextVariableOccurrence = $this->phpcsFile->findNext( T_VARIABLE, $closeBracket + 1, null, false, $variableName );
+		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 ) {
+		if ( $rightAfterNextVariableOccurence === false
+			|| $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 ) {
+		if ( $valueAfterEqualSign !== false && $this->tokens[ $valueAfterEqualSign ]['code'] === T_FALSE ) {
 			$message = 'Obtained cached value in `%s` is being overridden. Disabling caching?';
 			$data    = [ $variableName ];
 			$this->phpcsFile->addError( $message, $nextVariableOccurrence, 'CacheValueOverride', $data );
 		}
 	}
 
-	/**
-	 * Check whether the examined code is a function call.
-	 *
-	 * @param int $stackPtr The position of the current token in the stack.
-	 *
-	 * @return bool
-	 */
-	private function isFunctionCall( $stackPtr ) {
-
-		// Find the next non-empty token.
-		$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
-
-		if ( $this->tokens[ $openBracket ]['code'] !== T_OPEN_PARENTHESIS ) {
-			// Not a function call.
-			return false;
-		}
-
-		// Find the previous non-empty token.
-		$search   = Tokens::$emptyTokens;
-		$search[] = T_BITWISE_AND;
-		$previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true );
-
-		// It's a function definition, not a function call, so return false.
-		return ! ( $this->tokens[ $previous ]['code'] === T_FUNCTION );
-	}
-
 	/**
 	 * Check whether the examined code is a variable assignment.
 	 *
@@ -117,9 +126,10 @@ private function isFunctionCall( $stackPtr ) {
 	 */
 	private function isVariableAssignment( $stackPtr ) {
 
-		// Find the previous non-empty token.
+		// Find the previous non-empty token, but allow for FQN function calls.
 		$search   = Tokens::$emptyTokens;
 		$search[] = T_BITWISE_AND;
+		$search[] = T_NS_SEPARATOR;
 		$previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true );
 
 		if ( $this->tokens[ $previous ]['code'] !== T_EQUAL ) {

WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.1.inc

@@ -0,0 +1,127 @@
+<?php
+
+/*
+ * Not the sniff target.
+ */
+use function wp_cache_get;
+
+$partiallyQualifiedFunction = my\ns\wp_cache_get(); $partiallyQualifiedFunction = false;
+$methodCall = $this->wp_cache_get(); $methodCall = false;
+$nullsafeMethodCall = $this?->wp_cache_get(); $nullsafeMethodCall = false;
+$staticMethodCall = MyClass::wp_cache_get(); $staticMethodCall = false;
+echo WP_CACHE_GET;
+$namespacedFunction = namespace\wp_cache_get(); $namespacedFunction = false;
+
+function &wp_cache_get() {}
+
+// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
+#[WP_Cache_Get('text')]
+function foo() {}
+
+
+/*
+ * These should all be okay.
+ */
+$good_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP );
+
+if ( empty( $good_wp_users ) ) {
+	$good_wp_users = false;
+	$good_wp_users = get_users();
+}
+
+function NoAssignment() {
+	$unrelated_var = false;
+	\wp_cache_get( $key, $group ) === false && do_something_to_create_new_cache();
+	$unrelated_var = false;
+}
+
+function functionCallsAreNotCaseSensitiveGood() {
+	$retrieved = WP_Cache_Get( ...$params );
+	if ( empty( $retrieved ) ) {
+		$retrieved = false;
+	}
+}
+
+function fqnFunctionCallGood() {
+	$no_params_not_our_concern = \wp_cache_get();
+	if ( empty( $no_params_not_our_concern ) ) {
+		$no_params_not_our_concern = false;
+	}
+}
+
+function cacheOverrideDoesntAssignFalse() {
+	$not_false = wp_cache_get( ...$params );
+	$not_false = true;
+}
+
+
+function cacheOverrideAssignsUnknownValue( $unknown ) {
+	$not_false = wp_cache_get( ...$params );
+	$not_false = $unknown;
+}
+
+
+/*
+ * These should all be flagged.
+ */
+$bad_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP );
+$bad_wp_users = false; // Bad.
+
+if ( empty( $bad_wp_users ) ) {
+	$bad_wp_users = get_users();
+}
+
+function functionCallsAreNotCaseSensitiveBad() {
+	$cache_retrieved = WP_Cache_Get( ...$params );
+	// Comment.
+	do_something_else();
+	$cache_retrieved = false; // Bad.
+}
+
+function fqnFunctionCallBad() {
+	$shouldBeCaught = \wp_cache_get();
+	$shouldBeCaught = false; // Bad.
+}
+
+// Ignore PHP 8.1 first class callable.
+// Ignore as the assignment is not for the return value of the function, but for the callable.
+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.2.inc

@@ -0,0 +1,5 @@
+<?php
+
+// Parse error/live coding test.
+// This should be the only test in the file.
+$liveCoding = wp_cache_get()

WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.3.inc

@@ -0,0 +1,6 @@
+<?php
+
+// Parse error/live coding test.
+// This should be the only test in the file.
+$liveCoding = wp_cache_get();
+$liveCoding

WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.4.inc

@@ -0,0 +1,6 @@
+<?php
+
+// Parse error/live coding test.
+// This should be the only test in the file.
+$liveCoding = wp_cache_get();
+$liveCoding =

WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.php

@@ -19,12 +19,24 @@ class CacheValueOverrideUnitTest extends AbstractSniffUnitTest {
 	/**
 	 * Returns the lines where errors should occur.
 	 *
+	 * @param string $testFile The name of the file being tested.
+	 *
 	 * @return array<int, int> Key is the line number, value is the number of expected errors.
 	 */
-	public function getErrorList() {
-		return [
-			5 => 1,
-		];
+	public function getErrorList( $testFile = '' ) {
+
+		switch ( $testFile ) {
+			case 'CacheValueOverrideUnitTest.1.inc':
+				return [
+					68 => 1,
+					78 => 1,
+					83 => 1,
+					98 => 1,
+				];
+
+			default:
+				return [];
+		}
 	}
 
 	/**