EarlyExitSniff: fixed fixer for complicated logical conditions

Author: kukulich

SlevomatCodingStandard/Sniffs/ControlStructures/EarlyExitSniff.php

@@ -124,13 +124,13 @@ private function processElse(\PHP_CodeSniffer\Files\File $phpcsFile, int $elsePo
 
 			$ifCode = $this->getScopeCode($phpcsFile, $ifPointer);
 			$elseCode = $this->getScopeCode($phpcsFile, $elsePointer);
-			$negativeIfCondition = $this->getNegativeCondition($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'] + 1, $tokens[$ifPointer]['parenthesis_closer'] - 1);
+			$negativeIfCondition = $this->getNegativeCondition($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'], $tokens[$ifPointer]['parenthesis_closer']);
 			$afterIfCode = $this->fixIndentation($ifCode, $phpcsFile->eolChar, $this->getIndentation($phpcsFile, $ifPointer));
 
 			$phpcsFile->fixer->addContent(
 				$ifPointer,
 				sprintf(
-					'if (%s) {%s}%s%s',
+					'if %s {%s}%s%s',
 					$negativeIfCondition,
 					$elseCode,
 					$phpcsFile->eolChar,
@@ -225,7 +225,7 @@ private function processIf(\PHP_CodeSniffer\Files\File $phpcsFile, int $ifPointe
 		$earlyExitCode = $this->getEarlyExitCode($tokens[$scopePointer]['code']);
 		$earlyExitCodeIndentation = $this->prepareIndentation($ifIndentation);
 
-		$negativeIfCondition = $this->getNegativeCondition($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'] + 1, $tokens[$ifPointer]['parenthesis_closer'] - 1);
+		$negativeIfCondition = $this->getNegativeCondition($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'], $tokens[$ifPointer]['parenthesis_closer']);
 
 		$phpcsFile->fixer->beginChangeset();
 
@@ -238,7 +238,7 @@ private function processIf(\PHP_CodeSniffer\Files\File $phpcsFile, int $ifPointe
 		$phpcsFile->fixer->addContent(
 			$ifPointer,
 			sprintf(
-				'if (%s) {%s%s%s;%s%s}%s%s',
+				'if %s {%s%s%s;%s%s}%s%s',
 				$negativeIfCondition,
 				$phpcsFile->eolChar,
 				$earlyExitCodeIndentation,
@@ -291,7 +291,7 @@ private function getEarlyExitCode($code): string
 		return 'return';
 	}
 
-	private function getNegativeCondition(\PHP_CodeSniffer\Files\File $phpcsFile, int $conditionBoundaryStartPointer, int $conditionBoundaryEndPointer, bool $nested = false): string
+	private function getNegativeConditionPart(\PHP_CodeSniffer\Files\File $phpcsFile, int $conditionBoundaryStartPointer, int $conditionBoundaryEndPointer, bool $nested): string
 	{
 		$tokens = $phpcsFile->getTokens();
 
@@ -314,7 +314,7 @@ private function getNegativeCondition(\PHP_CodeSniffer\Files\File $phpcsFile, in
 			}
 		}
 
-		if (!$nested && count($booleanPointers) > 0) {
+		if (count($booleanPointers) > 0) {
 			return $this->getNegativeLogicalCondition($phpcsFile, $conditionBoundaryStartPointer, $conditionBoundaryEndPointer);
 		}
 
@@ -405,30 +405,38 @@ private function getNegativeLogicalCondition(\PHP_CodeSniffer\Files\File $phpcsF
 				continue;
 			}
 
-			$negativeCondition .= $this->getNegativeNestedCondition($phpcsFile, $nestedConditionStartPointer, $actualPointer - 1);
+			$negativeCondition .= $this->getNegativeCondition($phpcsFile, $nestedConditionStartPointer, $actualPointer - 1, true);
 			$negativeCondition .= $booleanOperatorReplacements[$tokens[$actualPointer]['code']];
 
 			$nestedConditionStartPointer = $actualPointer + 1;
 			$actualPointer++;
 
 		} while (true);
 
-		$negativeCondition .= $this->getNegativeNestedCondition($phpcsFile, $nestedConditionStartPointer, $conditionBoundaryEndPointer);
+		$negativeCondition .= $this->getNegativeCondition($phpcsFile, $nestedConditionStartPointer, $conditionBoundaryEndPointer, true);
 
 		return $negativeCondition;
 	}
 
-	private function getNegativeNestedCondition(\PHP_CodeSniffer\Files\File $phpcsFile, int $conditionBoundaryStartPointer, int $conditionBoundaryEndPointer): string
+	private function getNegativeCondition(\PHP_CodeSniffer\Files\File $phpcsFile, int $conditionBoundaryStartPointer, int $conditionBoundaryEndPointer, bool $nested = false): string
 	{
 		/** @var int $conditionStartPointer */
 		$conditionStartPointer = TokenHelper::findNextEffective($phpcsFile, $conditionBoundaryStartPointer);
 		/** @var int $conditionEndPointer */
 		$conditionEndPointer = TokenHelper::findPreviousEffective($phpcsFile, $conditionBoundaryEndPointer);
 
+		$tokens = $phpcsFile->getTokens();
+		if ($tokens[$conditionStartPointer]['code'] === T_OPEN_PARENTHESIS && $tokens[$conditionStartPointer]['parenthesis_closer'] === $conditionEndPointer) {
+			/** @var int $conditionStartPointer */
+			$conditionStartPointer = TokenHelper::findNextEffective($phpcsFile, $conditionStartPointer + 1);
+			/** @var int $conditionEndPointer */
+			$conditionEndPointer = TokenHelper::findPreviousEffective($phpcsFile, $conditionEndPointer - 1);
+		}
+
 		return sprintf(
 			'%s%s%s',
 			$conditionBoundaryStartPointer !== $conditionStartPointer ? TokenHelper::getContent($phpcsFile, $conditionBoundaryStartPointer, $conditionStartPointer - 1) : '',
-			$this->getNegativeCondition($phpcsFile, $conditionStartPointer, $conditionEndPointer, true),
+			$this->getNegativeConditionPart($phpcsFile, $conditionStartPointer, $conditionEndPointer, $nested),
 			$conditionBoundaryEndPointer !== $conditionEndPointer ? TokenHelper::getContent($phpcsFile, $conditionEndPointer + 1, $conditionBoundaryEndPointer) : ''
 		);
 	}

tests/Sniffs/ControlStructures/EarlyExitSniffTest.php

@@ -16,13 +16,13 @@ public function testErrors(): void
 	{
 		$report = self::checkFile(__DIR__ . '/data/earlyExitErrors.php');
 
-		self::assertSame(46, $report->getErrorCount());
+		self::assertSame(48, $report->getErrorCount());
 
 		foreach ([6, 15, 24, 33, 42, 50, 58, 66, 74, 82, 90, 98, 108, 149, 157, 165, 191, 199, 207] as $line) {
 			self::assertSniffError($report, $line, EarlyExitSniff::CODE_EARLY_EXIT_NOT_USED, 'Use early exit instead of else.');
 		}
 
-		foreach ([115, 122, 129, 135, 141, 213, 222, 229, 235, 241, 247, 256, 262, 271, 287, 305] as $line) {
+		foreach ([115, 122, 129, 135, 141, 213, 222, 229, 235, 241, 247, 256, 262, 271, 287, 305, 361, 368] as $line) {
 			self::assertSniffError($report, $line, EarlyExitSniff::CODE_EARLY_EXIT_NOT_USED, 'Use early exit to reduce code nesting.');
 		}
 

tests/Sniffs/ControlStructures/data/earlyExitErrors.fixed.php

@@ -206,7 +206,7 @@ function inlineCommentAfterIf() {
 }
 
 function logicalCombinedCondition() {
-	if (!(true && true) && !false) {
+	if ((!true || !true) && !false) {
 		return;
 	}
 
@@ -403,3 +403,21 @@ function ifElseInElse() {
 
 	doSomething();
 }
+
+function logicalCombinedComplicatedCondition() {
+	foreach ($records as $record) {
+		if (!in_array($record['type'], ['A', 'C']) || (strpos($record['name'], 'www.') !== 0 && strpos($record['name'], 'ftp.') !== 0)) {
+			continue;
+		}
+
+		echo 'You should see this echo twice!'.PHP_EOL;
+	}
+}
+
+function logicalVeryComplicatedCondition() {
+	if ((!true) || ((!false && !true) && (!false || (!true)))) {
+		return;
+	}
+
+	doSomething();
+}

tests/Sniffs/ControlStructures/data/earlyExitErrors.php

@@ -355,3 +355,17 @@ function ifElseInElse() {
 		}
 	}
 }
+
+function logicalCombinedComplicatedCondition() {
+	foreach ($records as $record) {
+		if (in_array($record['type'], ['A', 'C']) && (strpos($record['name'], 'www.') === 0 || strpos($record['name'], 'ftp.') === 0)) {
+			echo 'You should see this echo twice!'.PHP_EOL;
+		}
+	}
+}
+
+function logicalVeryComplicatedCondition() {
+	if ((true) && ((false || true) || (false && (true)))) {
+		doSomething();
+	}
+}