Enforce safe constructor overrides with @phpstan-consistent-constructor

Author: herndlm

conf/config.neon

@@ -986,6 +986,9 @@ services:
 	-
 		class: PHPStan\Rules\Methods\MethodParameterComparisonHelper
 
+	-
+		class: PHPStan\Rules\Methods\MethodVisibilityComparisonHelper
+
 	-
 		class: PHPStan\Rules\MissingTypehintCheck
 		arguments:

src/PhpDoc/StubValidator.php

@@ -68,6 +68,7 @@
 use PHPStan\Rules\Methods\ExistingClassesInTypehintsRule;
 use PHPStan\Rules\Methods\MethodParameterComparisonHelper;
 use PHPStan\Rules\Methods\MethodSignatureRule;
+use PHPStan\Rules\Methods\MethodVisibilityComparisonHelper;
 use PHPStan\Rules\Methods\MissingMethodParameterTypehintRule;
 use PHPStan\Rules\Methods\MissingMethodReturnTypehintRule;
 use PHPStan\Rules\Methods\MissingMethodSelfOutTypeRule;
@@ -209,7 +210,15 @@ private function getRuleRegistry(Container $container): RuleRegistry
 			new ExistingClassesInTypehintsRule($functionDefinitionCheck),
 			new \PHPStan\Rules\Functions\ExistingClassesInTypehintsRule($functionDefinitionCheck),
 			new ExistingClassesInPropertiesRule($reflectionProvider, $classNameCheck, $unresolvableTypeHelper, $phpVersion, true, false),
-			new OverridingMethodRule($phpVersion, new MethodSignatureRule($phpClassReflectionExtension, true, true), true, new MethodParameterComparisonHelper($phpVersion), $phpClassReflectionExtension, $container->getParameter('checkMissingOverrideMethodAttribute')),
+			new OverridingMethodRule(
+				$phpVersion,
+				new MethodSignatureRule($phpClassReflectionExtension, true, true),
+				true,
+				new MethodParameterComparisonHelper($phpVersion),
+				new MethodVisibilityComparisonHelper(),
+				$phpClassReflectionExtension,
+				$container->getParameter('checkMissingOverrideMethodAttribute'),
+			),
 			new DuplicateDeclarationRule(),
 			new LocalTypeAliasesRule($localTypeAliasesCheck),
 			new LocalTypeTraitAliasesRule($localTypeAliasesCheck, $reflectionProvider),

src/Rules/Methods/ConsistentConstructorRule.php

@@ -7,6 +7,7 @@
 use PHPStan\Node\InClassMethodNode;
 use PHPStan\Reflection\Dummy\DummyConstructorReflection;
 use PHPStan\Rules\Rule;
+use function array_merge;
 use function strtolower;
 
 /** @implements Rule<InClassMethodNode> */
@@ -15,6 +16,7 @@ final class ConsistentConstructorRule implements Rule
 
 	public function __construct(
 		private MethodParameterComparisonHelper $methodParameterComparisonHelper,
+		private MethodVisibilityComparisonHelper $methodVisibilityComparisonHelper,
 	)
 	{
 	}
@@ -47,7 +49,10 @@ public function processNode(Node $node, Scope $scope): array
 			return [];
 		}
 
-		return $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true);
+		return array_merge(
+			$this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true),
+			$this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method),
+		);
 	}
 
 }

src/Rules/Methods/MethodVisibilityComparisonHelper.php

@@ -0,0 +1,51 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Methods;
+
+use PHPStan\Reflection\ClassReflection;
+use PHPStan\Reflection\ExtendedMethodReflection;
+use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection;
+use PHPStan\Rules\IdentifierRuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+use function sprintf;
+
+final class MethodVisibilityComparisonHelper
+{
+
+	/** @return list<IdentifierRuleError> */
+	public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method): array
+	{
+		/** @var list<IdentifierRuleError> $messages */
+		$messages = [];
+
+		if ($prototype->isPublic()) {
+			if (!$method->isPublic()) {
+				$messages[] = RuleErrorBuilder::message(sprintf(
+					'%s method %s::%s() overriding public method %s::%s() should also be public.',
+					$method->isPrivate() ? 'Private' : 'Protected',
+					$method->getDeclaringClass()->getDisplayName(),
+					$method->getName(),
+					$prototypeDeclaringClass->getDisplayName(true),
+					$prototype->getName(),
+				))
+					->nonIgnorable()
+					->identifier('method.visibility')
+					->build();
+			}
+		} elseif ($method->isPrivate()) {
+			$messages[] = RuleErrorBuilder::message(sprintf(
+				'Private method %s::%s() overriding protected method %s::%s() should be protected or public.',
+				$method->getDeclaringClass()->getDisplayName(),
+				$method->getName(),
+				$prototypeDeclaringClass->getDisplayName(true),
+				$prototype->getName(),
+			))
+				->nonIgnorable()
+				->identifier('method.visibility')
+				->build();
+		}
+
+		return $messages;
+	}
+
+}

src/Rules/Methods/OverridingMethodRule.php

@@ -35,6 +35,7 @@ public function __construct(
 		private MethodSignatureRule $methodSignatureRule,
 		private bool $checkPhpDocMethodSignatures,
 		private MethodParameterComparisonHelper $methodParameterComparisonHelper,
+		private MethodVisibilityComparisonHelper $methodVisibilityComparisonHelper,
 		private PhpClassReflectionExtension $phpClassReflectionExtension,
 		private bool $checkMissingOverrideMethodAttribute,
 	)
@@ -165,32 +166,7 @@ public function processNode(Node $node, Scope $scope): array
 		}
 
 		if ($checkVisibility) {
-			if ($prototype->isPublic()) {
-				if (!$method->isPublic()) {
-					$messages[] = RuleErrorBuilder::message(sprintf(
-						'%s method %s::%s() overriding public method %s::%s() should also be public.',
-						$method->isPrivate() ? 'Private' : 'Protected',
-						$method->getDeclaringClass()->getDisplayName(),
-						$method->getName(),
-						$prototypeDeclaringClass->getDisplayName(true),
-						$prototype->getName(),
-					))
-						->nonIgnorable()
-						->identifier('method.visibility')
-						->build();
-				}
-			} elseif ($method->isPrivate()) {
-				$messages[] = RuleErrorBuilder::message(sprintf(
-					'Private method %s::%s() overriding protected method %s::%s() should be protected or public.',
-					$method->getDeclaringClass()->getDisplayName(),
-					$method->getName(),
-					$prototypeDeclaringClass->getDisplayName(true),
-					$prototype->getName(),
-				))
-					->nonIgnorable()
-					->identifier('method.visibility')
-					->build();
-			}
+			$messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method));
 		}
 
 		$prototypeVariants = $prototype->getVariants();

tests/PHPStan/Rules/Methods/ConsistentConstructorRuleTest.php

@@ -12,7 +12,10 @@ class ConsistentConstructorRuleTest extends RuleTestCase
 
 	protected function getRule(): Rule
 	{
-		return new ConsistentConstructorRule(self::getContainer()->getByType(MethodParameterComparisonHelper::class));
+		return new ConsistentConstructorRule(
+			self::getContainer()->getByType(MethodParameterComparisonHelper::class),
+			self::getContainer()->getByType(MethodVisibilityComparisonHelper::class),
+		);
 	}
 
 	public function testRule(): void
@@ -42,4 +45,14 @@ public function testRuleNoErrors(): void
 		$this->analyse([__DIR__ . '/data/consistent-constructor-no-errors.php'], []);
 	}
 
+	public function testBug12137(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-12137.php'], [
+			[
+				'Private method Bug12137\ChildClass::__construct() overriding protected method Bug12137\ParentClass::__construct() should be protected or public.',
+				20,
+			],
+		]);
+	}
+
 }

tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php

@@ -29,6 +29,7 @@ protected function getRule(): Rule
 			new MethodSignatureRule($phpClassReflectionExtension, $this->reportMaybes, $this->reportStatic),
 			true,
 			new MethodParameterComparisonHelper($phpVersion),
+			new MethodVisibilityComparisonHelper(),
 			$phpClassReflectionExtension,
 			false,
 		);

tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php

@@ -31,6 +31,7 @@ protected function getRule(): Rule
 			new MethodSignatureRule($phpClassReflectionExtension, true, true),
 			false,
 			new MethodParameterComparisonHelper($phpVersion),
+			new MethodVisibilityComparisonHelper(),
 			$phpClassReflectionExtension,
 			$this->checkMissingOverrideMethodAttribute,
 		);

tests/PHPStan/Rules/Methods/data/bug-12137.php

@@ -0,0 +1,23 @@
+<?php declare(strict_types = 1);
+
+namespace Bug12137;
+
+/** @phpstan-consistent-constructor */
+abstract class ParentClass
+{
+	protected function __construct()
+	{
+	}
+
+	public static function create(): static
+	{
+		return new static();
+	}
+}
+
+class ChildClass extends ParentClass
+{
+	private function __construct()
+	{
+	}
+}