Merge pull request #336 from Yoast/JRF/yoastcs-brainmonkeyraceconditi…

…on-various-improvements

Tools/BrainMonkeyRaceCondition: various improvements

Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php

@@ -2,20 +2,25 @@
 
 namespace YoastCS\Yoast\Sniffs\Tools;
 
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Sniffs\Sniff;
 use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Tokens\Collections;
+use PHPCSUtils\Utils\Conditions;
 use PHPCSUtils\Utils\FunctionDeclarations;
 use PHPCSUtils\Utils\PassedParameters;
+use PHPCSUtils\Utils\Scopes;
 use PHPCSUtils\Utils\TextStrings;
-use WordPressCS\WordPress\Sniff;
 
 /**
  * Sniff to detect a particular nasty race condition which can occur in tests using the BrainMonkey utilities.
  *
  * @link https://github.com/Yoast/yoastcs/issues/264
  *
  * @since 2.3.0
+ * @since 3.0.0 This sniff no longer extends the WPCS abstract Sniff class.
  */
-final class BrainMonkeyRaceConditionSniff extends Sniff {
+final class BrainMonkeyRaceConditionSniff implements Sniff {
 
 	/**
 	 * Returns an array of tokens this test wants to listen for.
@@ -27,121 +32,102 @@ public function register() {
 	}
 
 	/**
-	 * Processes a sniff when one of its tokens is encountered.
+	 * Processes this test, when one of its tokens is encountered.
 	 *
-	 * @param int $stackPtr The position of the current token in the stack.
+	 * @param File $phpcsFile The file being scanned.
+	 * @param int  $stackPtr  The position of the current token in the stack passed in $tokens.
 	 *
 	 * @return void
 	 */
-	public function process_token( $stackPtr ) {
-		if ( \strtolower( $this->tokens[ $stackPtr ]['content'] ) !== 'expect' ) {
+	public function process( File $phpcsFile, $stackPtr ) {
+		$tokens = $phpcsFile->getTokens();
+
+		if ( \strtolower( $tokens[ $stackPtr ]['content'] ) !== 'expect' ) {
 			return;
 		}
 
-		$nextNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
-		if ( $nextNonEmpty === false || $this->tokens[ $nextNonEmpty ]['code'] !== \T_OPEN_PARENTHESIS ) {
+		$nextNonEmpty = $phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+		if ( $nextNonEmpty === false || $tokens[ $nextNonEmpty ]['code'] !== \T_OPEN_PARENTHESIS ) {
 			// Definitely not a function call.
 			return;
 		}
 
-		$prevNonEmpty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
+		$prevNonEmpty = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
 		if ( $prevNonEmpty === false
-			|| $this->tokens[ $prevNonEmpty ]['code'] === \T_DOUBLE_COLON
-			|| $this->tokens[ $prevNonEmpty ]['code'] === \T_OBJECT_OPERATOR
-			|| $this->tokens[ $prevNonEmpty ]['type'] === 'T_NULLSAFE_OBJECT_OPERATOR'
-			|| $this->tokens[ $prevNonEmpty ]['code'] === \T_FUNCTION
+			|| isset( Collections::objectOperators()[ $tokens[ $prevNonEmpty ]['code'] ] )
+			|| $tokens[ $prevNonEmpty ]['code'] === \T_FUNCTION
 		) {
 			// Method call or function declaration, not a function call.
 			return;
 		}
 
-		// Make sure the token is in a class method.
-		if ( empty( $this->tokens[ $stackPtr ]['conditions'] ) ) {
+		$functionToken = Conditions::getLastCondition( $phpcsFile, $stackPtr, [ \T_FUNCTION ] );
+		if ( $functionToken === false ) {
 			return;
 		}
 
-		$conditions = $this->tokens[ $stackPtr ]['conditions'];
-		$conditions = \array_reverse( $conditions, true );
-
-		$seenOO        = false;
-		$seenFn        = false;
-		$functionToken = false;
-
-		foreach ( $conditions as $token => $condition ) {
-			if ( $seenFn === true ) {
-				// First condition after the function condition MUST be OO, otherwise it's not a method.
-				$seenOO = isset( Tokens::$ooScopeTokens[ $condition ] );
-				break;
-			}
-
-			if ( $condition === \T_FUNCTION ) {
-				$functionToken = $token;
-				$seenFn        = true;
-			}
-		}
-
-		if ( $seenFn === false || $seenOO === false || $functionToken === false ) {
+		if ( Scopes::isOOMethod( $phpcsFile, $functionToken ) === false ) {
 			return;
 		}
 
 		// Check that this is an expect() for one of the hook functions.
-		$param = PassedParameters::getParameter( $this->phpcsFile, $stackPtr, 1, 'function_name' );
+		$param = PassedParameters::getParameter( $phpcsFile, $stackPtr, 1, 'function_name' );
 		if ( empty( $param ) ) {
 			return;
 		}
 
 		$expected   = Tokens::$emptyTokens;
 		$expected[] = \T_CONSTANT_ENCAPSED_STRING;
 
-		$hasUnexpected = $this->phpcsFile->findNext( $expected, $param['start'], ( $param['end'] + 1 ), true );
+		$hasUnexpected = $phpcsFile->findNext( $expected, $param['start'], ( $param['end'] + 1 ), true );
 		if ( $hasUnexpected !== false ) {
 			return;
 		}
 
-		$text        = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true );
-		$textContent = TextStrings::stripQuotes( $this->tokens[ $text ]['content'] );
+		$text        = $phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true );
+		$textContent = TextStrings::stripQuotes( $tokens[ $text ]['content'] );
 		if ( $textContent !== 'apply_filters' && $textContent !== 'do_action' ) {
 			return;
 		}
 
 		// Now walk the contents of the function declaration to see if we can find the other function call.
-		if ( isset( $this->tokens[ $functionToken ]['scope_opener'], $this->tokens[ $functionToken ]['scope_closer'] ) === false ) {
-			// We don't know the start or end of the function.
-			return;
+		if ( isset( $tokens[ $functionToken ]['scope_opener'], $tokens[ $functionToken ]['scope_closer'] ) === false ) {
+			// We don't know the start or end of the function. Edge case which can't happen under normal circumstances.
+			return; // @codeCoverageIgnore
 		}
 
 		$targetContent = 'expectdone';
 		if ( $textContent === 'apply_filters' ) {
 			$targetContent = 'expectapplied';
 		}
 
-		$start = $this->tokens[ $functionToken ]['scope_opener'];
-		$end   = $this->tokens[ $functionToken ]['scope_closer'];
+		$start = $tokens[ $functionToken ]['scope_opener'];
+		$end   = $tokens[ $functionToken ]['scope_closer'];
 
 		for ( $i = $start; $i < $end; $i++ ) {
-			if ( $this->tokens[ $i ]['code'] !== \T_STRING ) {
+			if ( $tokens[ $i ]['code'] !== \T_STRING ) {
 				continue;
 			}
 
-			if ( \strtolower( $this->tokens[ $i ]['content'] ) !== $targetContent ) {
+			if ( \strtolower( $tokens[ $i ]['content'] ) !== $targetContent ) {
 				continue;
 			}
 
 			// Make sure it is a function call.
-			$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true );
-			if ( $next === false || $this->tokens[ $next ]['code'] !== \T_OPEN_PARENTHESIS ) {
+			$next = $phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true );
+			if ( $next === false || $tokens[ $next ]['code'] !== \T_OPEN_PARENTHESIS ) {
 				continue;
 			}
 
 			// Okay, we have found the race condition. Throw error.
 			$message = 'The %s() test method contains both a call to Monkey\Functions\expect( %s ), as well as a call to %s(). This causes a race condition which will cause the tests to fail. Only use one of these in a test.';
 			$data    = [
-				FunctionDeclarations::getName( $this->phpcsFile, $functionToken ),
-				$this->tokens[ $text ]['content'],
+				FunctionDeclarations::getName( $phpcsFile, $functionToken ),
+				$tokens[ $text ]['content'],
 				( $targetContent === 'expectdone' ) ? 'Monkey\Actions\expectDone' : 'Monkey\Filters\expectApplied',
 			];
 
-			$this->phpcsFile->addError( $message, $functionToken, 'Found', $data );
+			$phpcsFile->addError( $message, $functionToken, 'Found', $data );
 			break;
 		}
 	}

Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.inc

@@ -153,4 +153,41 @@ class BrainMonkeyBasedTest extends TestCase {
 
 		// Do something to satisfy expectations.
 	}
+
+	/*
+	 * Tests involving PHP 8.0+ named parameters.
+	 */
+	public function test_NOT_race_condition_wrong_param_name() {
+		Functions\expect( not_functionname: "apply_filters")->times(5);
+
+		Filters\ExpectDone  ( 'filter_name' );
+
+		// Do something to satisfy expectations.
+	}
+
+	public function test_NOT_race_condition_named_param_not_for_hook() {
+		Functions\expect( function_name: "other_function")->times(5);
+
+		Filters\ExpectDone  ( 'filter_name' );
+
+		// Do something to satisfy expectations.
+	}
+
+	public function test_filter_race_condition_named_param() {
+		Functions\expect( function_name: "apply_filters")->twice();
+
+		Filters\ExpectApplied  ( 'filter_name' )
+			->with( true );
+
+		// Do something to satisfy expectations.
+	}
+
+	public function test_action_race_condition_named_param_unconventional_order() {
+		\Brain\Monkey\Functions\Expect( something: foo, function_name: 'do_action' )->once();
+
+		Actions\expectDone( 'yoast\action_name' )
+			->with( $param );
+
+		// Do something to satisfy expectations.
+	}
 }

Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.php

@@ -24,6 +24,8 @@ public function getErrorList(): array {
 			113 => 1,
 			122 => 1,
 			131 => 1,
+			176 => 1,
+			185 => 1,
 		];
 	}