From de41b2a5bae8560de2671717ec36776ceb059f40 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaroslav=20Hansl=C3=ADk?= <kukulich@kukulich.cz>
Date: Mon, 22 Jun 2020 12:36:13 +0200
Subject: [PATCH] UselessIfConditionWithReturnSniff: Don't remove comments
 automatically

---
 .../UselessIfConditionWithReturnSniff.php     | 28 ++++++++++++-------
 .../UselessIfConditionWithReturnSniffTest.php |  5 +++-
 ...elessIfConditionWithReturnErrors.fixed.php | 27 ++++++++++++++++++
 .../uselessIfConditionWithReturnErrors.php    | 27 ++++++++++++++++++
 4 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/SlevomatCodingStandard/Sniffs/ControlStructures/UselessIfConditionWithReturnSniff.php b/SlevomatCodingStandard/Sniffs/ControlStructures/UselessIfConditionWithReturnSniff.php
index d07ae7025..34f5b75d9 100644
--- a/SlevomatCodingStandard/Sniffs/ControlStructures/UselessIfConditionWithReturnSniff.php
+++ b/SlevomatCodingStandard/Sniffs/ControlStructures/UselessIfConditionWithReturnSniff.php
@@ -4,6 +4,7 @@
 
 use PHP_CodeSniffer\Files\File;
 use PHP_CodeSniffer\Sniffs\Sniff;
+use PHP_CodeSniffer\Util\Tokens;
 use SlevomatCodingStandard\Helpers\ConditionHelper;
 use SlevomatCodingStandard\Helpers\TokenHelper;
 use function array_key_exists;
@@ -60,14 +61,6 @@ public function process(File $phpcsFile, $ifPointer): void
 				: ConditionHelper::getNegativeCondition($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'] + 1, $tokens[$ifPointer]['parenthesis_closer'] - 1);
 		};
 
-		$isFixable = function (int $ifPointer) use ($phpcsFile, $tokens): bool {
-			if ($this->assumeAllConditionExpressionsAreAlreadyBoolean) {
-				return true;
-			}
-
-			return ConditionHelper::conditionReturnsBoolean($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'] + 1, $tokens[$ifPointer]['parenthesis_closer'] - 1);
-		};
-
 		$elsePointer = TokenHelper::findNextEffective($phpcsFile, $tokens[$ifPointer]['scope_closer'] + 1);
 
 		$errorParameters = [
@@ -90,7 +83,7 @@ public function process(File $phpcsFile, $ifPointer): void
 				return;
 			}
 
-			if (!$isFixable($ifPointer)) {
+			if (!$this->isFixable($phpcsFile, $ifPointer, $tokens[$elsePointer]['scope_closer'])) {
 				$phpcsFile->addError(...$errorParameters);
 				return;
 			}
@@ -119,7 +112,7 @@ public function process(File $phpcsFile, $ifPointer): void
 				return;
 			}
 
-			if (!$isFixable($ifPointer)) {
+			if (!$this->isFixable($phpcsFile, $ifPointer, $semicolonPointer)) {
 				$phpcsFile->addError(...$errorParameters);
 				return;
 			}
@@ -139,6 +132,21 @@ public function process(File $phpcsFile, $ifPointer): void
 		}
 	}
 
+	private function isFixable(File $phpcsFile, int $ifPointer, int $endPointer): bool
+	{
+		$tokens = $phpcsFile->getTokens();
+
+		if (TokenHelper::findNext($phpcsFile, Tokens::$commentTokens, $ifPointer + 1, $endPointer) !== null) {
+			return false;
+		}
+
+		if ($this->assumeAllConditionExpressionsAreAlreadyBoolean) {
+			return true;
+		}
+
+		return ConditionHelper::conditionReturnsBoolean($phpcsFile, $tokens[$ifPointer]['parenthesis_opener'] + 1, $tokens[$ifPointer]['parenthesis_closer'] - 1);
+	}
+
 	private function findBooleanAfterReturnInScope(File $phpcsFile, int $scopeOpenerPointer): ?int
 	{
 		$tokens = $phpcsFile->getTokens();
diff --git a/tests/Sniffs/ControlStructures/UselessIfConditionWithReturnSniffTest.php b/tests/Sniffs/ControlStructures/UselessIfConditionWithReturnSniffTest.php
index 6bfe23646..a9254d226 100644
--- a/tests/Sniffs/ControlStructures/UselessIfConditionWithReturnSniffTest.php
+++ b/tests/Sniffs/ControlStructures/UselessIfConditionWithReturnSniffTest.php
@@ -17,7 +17,7 @@ public function testErrors(): void
 	{
 		$report = self::checkFile(__DIR__ . '/data/uselessIfConditionWithReturnErrors.php');
 
-		self::assertSame(9, $report->getErrorCount());
+		self::assertSame(12, $report->getErrorCount());
 
 		self::assertSniffError($report, 4, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
 		self::assertSniffError($report, 12, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
@@ -28,6 +28,9 @@ public function testErrors(): void
 		self::assertSniffError($report, 52, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
 		self::assertSniffError($report, 61, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
 		self::assertSniffError($report, 70, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
+		self::assertSniffError($report, 82, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
+		self::assertSniffError($report, 91, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
+		self::assertSniffError($report, 100, UselessIfConditionWithReturnSniff::CODE_USELESS_IF_CONDITION);
 
 		self::assertAllFixedInFile($report);
 	}
diff --git a/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.fixed.php b/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.fixed.php
index 064d6de4e..9369d5364 100644
--- a/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.fixed.php
+++ b/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.fixed.php
@@ -49,3 +49,30 @@ function () {
 		empty($day['from']) || !empty($day['to'])
 	);
 };
+
+function () {
+	if (true) {
+		// Comment in if
+		return true;
+	}
+
+	return false;
+};
+
+function () {
+	if (true) {
+		return false;
+	} else {
+		// Comment in else
+		return true;
+	}
+};
+
+function () {
+	if (true) {
+		return true;
+	}
+
+	// Comment before return
+	return false;
+};
diff --git a/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.php b/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.php
index 99e702f10..23ccc3dd5 100644
--- a/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.php
+++ b/tests/Sniffs/ControlStructures/data/uselessIfConditionWithReturnErrors.php
@@ -77,3 +77,30 @@ function () {
 
 	return true;
 };
+
+function () {
+	if (true) {
+		// Comment in if
+		return true;
+	}
+
+	return false;
+};
+
+function () {
+	if (true) {
+		return false;
+	} else {
+		// Comment in else
+		return true;
+	}
+};
+
+function () {
+	if (true) {
+		return true;
+	}
+
+	// Comment before return
+	return false;
+};