From 2cd700142fb4756650f96a6227e4bb35eb3b5ef0 Mon Sep 17 00:00:00 2001
From: Ondrej Mirtes <ondrej@mirtes.cz>
Date: Sat, 10 Oct 2020 13:52:30 +0200
Subject: [PATCH] Fix reporting overriden variadics

---
 src/Rules/Methods/OverridingMethodRule.php    | 77 ++++++++++++++-----
 .../Methods/OverridingMethodRuleTest.php      | 27 +++++++
 .../Methods/data/overriding-variadics.php     | 69 +++++++++++++++++
 3 files changed, 153 insertions(+), 20 deletions(-)
 create mode 100644 tests/PHPStan/Rules/Methods/data/overriding-variadics.php

diff --git a/src/Rules/Methods/OverridingMethodRule.php b/src/Rules/Methods/OverridingMethodRule.php
index cf2969f0da..2f0237a534 100644
--- a/src/Rules/Methods/OverridingMethodRule.php
+++ b/src/Rules/Methods/OverridingMethodRule.php
@@ -146,6 +146,7 @@ public function processNode(Node $node, Scope $scope): array
 		$methodVariant = ParametersAcceptorSelector::selectSingle($method->getVariants());
 		$methodParameters = $methodVariant->getParameters();
 
+		$prototypeAfterVariadic = false;
 		foreach ($prototypeVariant->getParameters() as $i => $prototypeParameter) {
 			if (!array_key_exists($i, $methodParameters)) {
 				$messages[] = RuleErrorBuilder::message(sprintf(
@@ -190,19 +191,41 @@ public function processNode(Node $node, Scope $scope): array
 			}
 
 			if ($prototypeParameter->isVariadic()) {
+				$prototypeAfterVariadic = true;
 				if (!$methodParameter->isVariadic()) {
-					$messages[] = RuleErrorBuilder::message(sprintf(
-						'Parameter #%d $%s of method %s::%s() is not variadic but parameter #%d $%s of method %s::%s() is variadic.',
-						$i + 1,
-						$methodParameter->getName(),
-						$method->getDeclaringClass()->getDisplayName(),
-						$method->getName(),
-						$i + 1,
-						$prototypeParameter->getName(),
-						$prototype->getDeclaringClass()->getDisplayName(),
-						$prototype->getName()
-					))->nonIgnorable()->build();
-					continue;
+					if (!$methodParameter->isOptional()) {
+						if (count($methodParameters) !== $i + 1) {
+							$messages[] = RuleErrorBuilder::message(sprintf(
+								'Parameter #%d $%s of method %s::%s() is not optional.',
+								$i + 1,
+								$methodParameter->getName(),
+								$method->getDeclaringClass()->getDisplayName(),
+								$method->getName()
+							))->nonIgnorable()->build();
+							continue;
+						}
+
+						$messages[] = RuleErrorBuilder::message(sprintf(
+							'Parameter #%d $%s of method %s::%s() is not variadic but parameter #%d $%s of method %s::%s() is variadic.',
+							$i + 1,
+							$methodParameter->getName(),
+							$method->getDeclaringClass()->getDisplayName(),
+							$method->getName(),
+							$i + 1,
+							$prototypeParameter->getName(),
+							$prototype->getDeclaringClass()->getDisplayName(),
+							$prototype->getName()
+						))->nonIgnorable()->build();
+						continue;
+					} elseif (count($methodParameters) === $i + 1) {
+						$messages[] = RuleErrorBuilder::message(sprintf(
+							'Parameter #%d $%s of method %s::%s() is not variadic.',
+							$i + 1,
+							$methodParameter->getName(),
+							$method->getDeclaringClass()->getDisplayName(),
+							$method->getName(),
+						))->nonIgnorable()->build();
+					}
 				}
 			} elseif ($methodParameter->isVariadic()) {
 				$messages[] = RuleErrorBuilder::message(sprintf(
@@ -285,17 +308,31 @@ public function processNode(Node $node, Scope $scope): array
 				continue;
 			}
 
-			if ($methodParameter->isOptional()) {
+			if (
+				$j === count($methodParameters) - 1
+				&& $prototypeAfterVariadic
+				&& !$methodParameter->isVariadic()
+			) {
+				$messages[] = RuleErrorBuilder::message(sprintf(
+					'Parameter #%d $%s of method %s::%s() is not variadic.',
+					$j + 1,
+					$methodParameter->getName(),
+					$method->getDeclaringClass()->getDisplayName(),
+					$method->getName()
+				))->nonIgnorable()->build();
 				continue;
 			}
 
-			$messages[] = RuleErrorBuilder::message(sprintf(
-				'Parameter #%d $%s of method %s::%s() is not optional.',
-				$j + 1,
-				$methodParameter->getName(),
-				$method->getDeclaringClass()->getDisplayName(),
-				$method->getName()
-			))->nonIgnorable()->build();
+			if (!$methodParameter->isOptional()) {
+				$messages[] = RuleErrorBuilder::message(sprintf(
+					'Parameter #%d $%s of method %s::%s() is not optional.',
+					$j + 1,
+					$methodParameter->getName(),
+					$method->getDeclaringClass()->getDisplayName(),
+					$method->getName()
+				))->nonIgnorable()->build();
+				continue;
+			}
 		}
 
 		$methodReturnType = $methodVariant->getNativeReturnType();
diff --git a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php
index 9cc005e1ea..0e79f2c536 100644
--- a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php
+++ b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php
@@ -355,4 +355,31 @@ public function testBug3629(): void
 		$this->analyse([__DIR__ . '/data/bug-3629.php'], []);
 	}
 
+	public function testVariadics(): void
+	{
+		if (!self::$useStaticReflectionProvider) {
+			$this->markTestSkipped('Test requires static reflection.');
+		}
+
+		$this->phpVersionId = PHP_VERSION_ID;
+		$this->analyse([__DIR__ . '/data/overriding-variadics.php'], [
+			[
+				'Parameter #2 $lang of method OverridingVariadics\OtherTranslator::translate() is not optional.',
+				34,
+			],
+			[
+				'Parameter #2 $lang of method OverridingVariadics\AnotherTranslator::translate() is not optional.',
+				44,
+			],
+			[
+				'Parameter #3 $parameters of method OverridingVariadics\AnotherTranslator::translate() is not variadic.',
+				44,
+			],
+			[
+				'Parameter #2 $lang of method OverridingVariadics\YetAnotherTranslator::translate() is not variadic.',
+				54,
+			],
+		]);
+	}
+
 }
diff --git a/tests/PHPStan/Rules/Methods/data/overriding-variadics.php b/tests/PHPStan/Rules/Methods/data/overriding-variadics.php
new file mode 100644
index 0000000000..b91bb38579
--- /dev/null
+++ b/tests/PHPStan/Rules/Methods/data/overriding-variadics.php
@@ -0,0 +1,69 @@
+<?php
+
+namespace OverridingVariadics;
+
+interface ITranslator
+{
+
+	/**
+	 * Translates the given string.
+	 * @param  mixed  $message
+	 * @param  string  ...$parameters
+	 */
+	function translate($message, string ...$parameters): string;
+
+}
+
+class Translator implements ITranslator
+{
+
+	/**
+	 * @param string $message
+	 * @param string ...$parameters
+	 */
+	public function translate($message, $lang = 'cs', string ...$parameters): string
+	{
+
+	}
+
+}
+
+class OtherTranslator implements ITranslator
+{
+
+	public function translate($message, $lang, string ...$parameters): string
+	{
+
+	}
+
+}
+
+class AnotherTranslator implements ITranslator
+{
+
+	public function translate($message, $lang = 'cs', string $parameters): string
+	{
+
+	}
+
+}
+
+class YetAnotherTranslator implements ITranslator
+{
+
+	public function translate($message, $lang = 'cs'): string
+	{
+
+	}
+
+}
+
+class ReflectionClass extends \ReflectionClass
+{
+
+	public function newInstance($arg = null, ...$args)
+	{
+
+	}
+
+}