From abae22bc6d118c473ddb5cd0abfd3f67a7402683 Mon Sep 17 00:00:00 2001 From: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:57:17 +0000 Subject: [PATCH 1/3] Fix phpstan/phpstan#14398: Incorrect line number for method visibility errors - Added optional $line parameter to MethodVisibilityComparisonHelper::compare() - Pass method name start line from OverridingMethodRule and ConsistentConstructorRule - This ensures visibility errors point to the function declaration line, not the first attribute line - New regression test in tests/PHPStan/Rules/Methods/data/bug-14398.php --- .../Methods/ConsistentConstructorRule.php | 2 +- .../MethodVisibilityComparisonHelper.php | 20 +++++++++++------- src/Rules/Methods/OverridingMethodRule.php | 2 +- .../Methods/OverridingMethodRuleTest.php | 12 +++++++++++ .../PHPStan/Rules/Methods/data/bug-14398.php | 21 +++++++++++++++++++ 5 files changed, 48 insertions(+), 9 deletions(-) create mode 100644 tests/PHPStan/Rules/Methods/data/bug-14398.php diff --git a/src/Rules/Methods/ConsistentConstructorRule.php b/src/Rules/Methods/ConsistentConstructorRule.php index 5eace25f3a6..df07da54254 100644 --- a/src/Rules/Methods/ConsistentConstructorRule.php +++ b/src/Rules/Methods/ConsistentConstructorRule.php @@ -48,7 +48,7 @@ public function processNode(Node $node, Scope $scope): array return array_merge( $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true), - $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method), + $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, $node->getOriginalNode()->name->getStartLine()), ); } diff --git a/src/Rules/Methods/MethodVisibilityComparisonHelper.php b/src/Rules/Methods/MethodVisibilityComparisonHelper.php index f0e922deece..6944fc4f701 100755 --- a/src/Rules/Methods/MethodVisibilityComparisonHelper.php +++ b/src/Rules/Methods/MethodVisibilityComparisonHelper.php @@ -15,14 +15,14 @@ final class MethodVisibilityComparisonHelper { /** @return list */ - public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method): array + public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method, ?int $line = null): array { /** @var list $messages */ $messages = []; if ($prototype->isPublic()) { if (!$method->isPublic()) { - $messages[] = RuleErrorBuilder::message(sprintf( + $errorBuilder = RuleErrorBuilder::message(sprintf( '%s method %s::%s() overriding public method %s::%s() should also be public.', $method->isPrivate() ? 'Private' : 'Protected', $method->getDeclaringClass()->getDisplayName(), @@ -31,11 +31,14 @@ public function compare(ExtendedMethodReflection $prototype, ClassReflection $pr $prototype->getName(), )) ->nonIgnorable() - ->identifier('method.visibility') - ->build(); + ->identifier('method.visibility'); + if ($line !== null) { + $errorBuilder->line($line); + } + $messages[] = $errorBuilder->build(); } } elseif ($method->isPrivate()) { - $messages[] = RuleErrorBuilder::message(sprintf( + $errorBuilder = RuleErrorBuilder::message(sprintf( 'Private method %s::%s() overriding protected method %s::%s() should be protected or public.', $method->getDeclaringClass()->getDisplayName(), $method->getName(), @@ -43,8 +46,11 @@ public function compare(ExtendedMethodReflection $prototype, ClassReflection $pr $prototype->getName(), )) ->nonIgnorable() - ->identifier('method.visibility') - ->build(); + ->identifier('method.visibility'); + if ($line !== null) { + $errorBuilder->line($line); + } + $messages[] = $errorBuilder->build(); } return $messages; diff --git a/src/Rules/Methods/OverridingMethodRule.php b/src/Rules/Methods/OverridingMethodRule.php index 2058b64931f..f16d329bdb5 100644 --- a/src/Rules/Methods/OverridingMethodRule.php +++ b/src/Rules/Methods/OverridingMethodRule.php @@ -189,7 +189,7 @@ public function processNode(Node $node, Scope&NodeCallbackInvoker $scope): array } if ($checkVisibility) { - $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method)); + $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method, $node->getOriginalNode()->name->getStartLine())); } $prototypeVariants = $prototype->getVariants(); diff --git a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php index 55979eeb77d..07d47bb48ad 100644 --- a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php +++ b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php @@ -853,4 +853,16 @@ public function testFixWithTabs(): void $this->fix(__DIR__ . '/data/fix-with-tabs.php', __DIR__ . '/data/fix-with-tabs.php.fixed'); } + #[RequiresPhp('>= 8.0')] + public function testBug14398(): void + { + $this->phpVersionId = PHP_VERSION_ID; + $this->analyse([__DIR__ . '/data/bug-14398.php'], [ + [ + 'Private method Bug14398\ChildPrivateOverridesProtected::calculate() overriding protected method Bug14398\BaseProtectedMethod::calculate() should be protected or public.', + 17, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Methods/data/bug-14398.php b/tests/PHPStan/Rules/Methods/data/bug-14398.php new file mode 100644 index 00000000000..f9fbae4dd22 --- /dev/null +++ b/tests/PHPStan/Rules/Methods/data/bug-14398.php @@ -0,0 +1,21 @@ += 8.0 + +namespace Bug14398; + +class BaseProtectedMethod +{ + protected function calculate(): int + { + return 42; + } +} + +class ChildPrivateOverridesProtected extends BaseProtectedMethod +{ + // PHPStan: Private method … overriding protected method … should be protected or public. + #[Override] + private function calculate(): int + { + return 99; + } +} From ce8177189e2c3557774526d44fe98e3b31aea1f6 Mon Sep 17 00:00:00 2001 From: phpstan-bot Date: Thu, 2 Apr 2026 15:29:01 +0000 Subject: [PATCH 2/3] Change MethodVisibilityComparisonHelper::compare() to accept Node instead of optional line number Address review feedback: replace optional `?int $line = null` parameter with a required `Node $node` parameter, calling `$node->getStartLine()` inside the helper. This simplifies callers and makes the API more explicit. Co-Authored-By: Claude Opus 4.6 --- .../Methods/ConsistentConstructorRule.php | 2 +- .../MethodVisibilityComparisonHelper.php | 23 ++++++++----------- src/Rules/Methods/OverridingMethodRule.php | 2 +- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/Rules/Methods/ConsistentConstructorRule.php b/src/Rules/Methods/ConsistentConstructorRule.php index df07da54254..69de69c0ac2 100644 --- a/src/Rules/Methods/ConsistentConstructorRule.php +++ b/src/Rules/Methods/ConsistentConstructorRule.php @@ -48,7 +48,7 @@ public function processNode(Node $node, Scope $scope): array return array_merge( $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true), - $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, $node->getOriginalNode()->name->getStartLine()), + $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, $node->getOriginalNode()->name), ); } diff --git a/src/Rules/Methods/MethodVisibilityComparisonHelper.php b/src/Rules/Methods/MethodVisibilityComparisonHelper.php index 6944fc4f701..71cbfcb9a6e 100755 --- a/src/Rules/Methods/MethodVisibilityComparisonHelper.php +++ b/src/Rules/Methods/MethodVisibilityComparisonHelper.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Methods; +use PhpParser\Node; use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Reflection\ClassReflection; use PHPStan\Reflection\ExtendedMethodReflection; @@ -15,14 +16,14 @@ final class MethodVisibilityComparisonHelper { /** @return list */ - public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method, ?int $line = null): array + public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method, Node $node): array { /** @var list $messages */ $messages = []; if ($prototype->isPublic()) { if (!$method->isPublic()) { - $errorBuilder = RuleErrorBuilder::message(sprintf( + $messages[] = RuleErrorBuilder::message(sprintf( '%s method %s::%s() overriding public method %s::%s() should also be public.', $method->isPrivate() ? 'Private' : 'Protected', $method->getDeclaringClass()->getDisplayName(), @@ -31,14 +32,12 @@ public function compare(ExtendedMethodReflection $prototype, ClassReflection $pr $prototype->getName(), )) ->nonIgnorable() - ->identifier('method.visibility'); - if ($line !== null) { - $errorBuilder->line($line); - } - $messages[] = $errorBuilder->build(); + ->identifier('method.visibility') + ->line($node->getStartLine()) + ->build(); } } elseif ($method->isPrivate()) { - $errorBuilder = RuleErrorBuilder::message(sprintf( + $messages[] = RuleErrorBuilder::message(sprintf( 'Private method %s::%s() overriding protected method %s::%s() should be protected or public.', $method->getDeclaringClass()->getDisplayName(), $method->getName(), @@ -46,11 +45,9 @@ public function compare(ExtendedMethodReflection $prototype, ClassReflection $pr $prototype->getName(), )) ->nonIgnorable() - ->identifier('method.visibility'); - if ($line !== null) { - $errorBuilder->line($line); - } - $messages[] = $errorBuilder->build(); + ->identifier('method.visibility') + ->line($node->getStartLine()) + ->build(); } return $messages; diff --git a/src/Rules/Methods/OverridingMethodRule.php b/src/Rules/Methods/OverridingMethodRule.php index f16d329bdb5..fa83f7dcc2d 100644 --- a/src/Rules/Methods/OverridingMethodRule.php +++ b/src/Rules/Methods/OverridingMethodRule.php @@ -189,7 +189,7 @@ public function processNode(Node $node, Scope&NodeCallbackInvoker $scope): array } if ($checkVisibility) { - $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method, $node->getOriginalNode()->name->getStartLine())); + $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method, $node->getOriginalNode()->name)); } $prototypeVariants = $prototype->getVariants(); From c6432edafe0d4fe6945593fb5079bc6654981840 Mon Sep 17 00:00:00 2001 From: phpstan-bot Date: Thu, 2 Apr 2026 15:56:19 +0000 Subject: [PATCH 3/3] Fix InClassMethodNode startLine for all rules with attributes Instead of fixing individual rules, adjust InClassMethodNode to use the method name's start line when attributes shift the node's start line. This fixes incorrect error line numbers for ALL rules processing InClassMethodNode when methods have attributes (e.g. #[Override]). The range check ensures trait-aliased methods (where the name identifier points to the alias location) keep their original line number. Also reverts MethodVisibilityComparisonHelper to its simpler signature since the line fix is now handled at the node level. Co-Authored-By: Claude Opus 4.6 --- src/Node/InClassMethodNode.php | 7 ++++++- src/Rules/Methods/ConsistentConstructorRule.php | 2 +- src/Rules/Methods/MethodVisibilityComparisonHelper.php | 5 +---- src/Rules/Methods/OverridingMethodRule.php | 2 +- ...edAttributePhpFunctionFromParserReflectionRuleTest.php | 6 +++--- .../Reflection/AttributeReflectionFromNodeRuleTest.php | 2 +- tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php | 6 +++--- tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php | 8 ++++---- 8 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/Node/InClassMethodNode.php b/src/Node/InClassMethodNode.php index 6547b7a12a9..329f657b017 100644 --- a/src/Node/InClassMethodNode.php +++ b/src/Node/InClassMethodNode.php @@ -19,7 +19,12 @@ public function __construct( private Node\Stmt\ClassMethod $originalNode, ) { - parent::__construct($originalNode->getAttributes()); + $attributes = $originalNode->getAttributes(); + $nameStartLine = $originalNode->name->getStartLine(); + if ($nameStartLine >= $originalNode->getStartLine() && $nameStartLine <= $originalNode->getEndLine()) { + $attributes['startLine'] = $nameStartLine; + } + parent::__construct($attributes); } public function getClassReflection(): ClassReflection diff --git a/src/Rules/Methods/ConsistentConstructorRule.php b/src/Rules/Methods/ConsistentConstructorRule.php index 69de69c0ac2..5eace25f3a6 100644 --- a/src/Rules/Methods/ConsistentConstructorRule.php +++ b/src/Rules/Methods/ConsistentConstructorRule.php @@ -48,7 +48,7 @@ public function processNode(Node $node, Scope $scope): array return array_merge( $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true), - $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, $node->getOriginalNode()->name), + $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method), ); } diff --git a/src/Rules/Methods/MethodVisibilityComparisonHelper.php b/src/Rules/Methods/MethodVisibilityComparisonHelper.php index 71cbfcb9a6e..f0e922deece 100755 --- a/src/Rules/Methods/MethodVisibilityComparisonHelper.php +++ b/src/Rules/Methods/MethodVisibilityComparisonHelper.php @@ -2,7 +2,6 @@ namespace PHPStan\Rules\Methods; -use PhpParser\Node; use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Reflection\ClassReflection; use PHPStan\Reflection\ExtendedMethodReflection; @@ -16,7 +15,7 @@ final class MethodVisibilityComparisonHelper { /** @return list */ - public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method, Node $node): array + public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method): array { /** @var list $messages */ $messages = []; @@ -33,7 +32,6 @@ public function compare(ExtendedMethodReflection $prototype, ClassReflection $pr )) ->nonIgnorable() ->identifier('method.visibility') - ->line($node->getStartLine()) ->build(); } } elseif ($method->isPrivate()) { @@ -46,7 +44,6 @@ public function compare(ExtendedMethodReflection $prototype, ClassReflection $pr )) ->nonIgnorable() ->identifier('method.visibility') - ->line($node->getStartLine()) ->build(); } diff --git a/src/Rules/Methods/OverridingMethodRule.php b/src/Rules/Methods/OverridingMethodRule.php index fa83f7dcc2d..2058b64931f 100644 --- a/src/Rules/Methods/OverridingMethodRule.php +++ b/src/Rules/Methods/OverridingMethodRule.php @@ -189,7 +189,7 @@ public function processNode(Node $node, Scope&NodeCallbackInvoker $scope): array } if ($checkVisibility) { - $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method, $node->getOriginalNode()->name)); + $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method)); } $prototypeVariants = $prototype->getVariants(); diff --git a/tests/PHPStan/Reflection/Annotations/DeprecatedAttributePhpFunctionFromParserReflectionRuleTest.php b/tests/PHPStan/Reflection/Annotations/DeprecatedAttributePhpFunctionFromParserReflectionRuleTest.php index aef982a7d73..6fc35b859da 100644 --- a/tests/PHPStan/Reflection/Annotations/DeprecatedAttributePhpFunctionFromParserReflectionRuleTest.php +++ b/tests/PHPStan/Reflection/Annotations/DeprecatedAttributePhpFunctionFromParserReflectionRuleTest.php @@ -99,15 +99,15 @@ public function testMethodRule(): void ], [ 'Deprecated', - 15, + 16, ], [ 'Deprecated: msg', - 21, + 22, ], [ 'Deprecated: msg2', - 27, + 28, ], ]); } diff --git a/tests/PHPStan/Reflection/AttributeReflectionFromNodeRuleTest.php b/tests/PHPStan/Reflection/AttributeReflectionFromNodeRuleTest.php index a07cbead1b2..f79880e9779 100644 --- a/tests/PHPStan/Reflection/AttributeReflectionFromNodeRuleTest.php +++ b/tests/PHPStan/Reflection/AttributeReflectionFromNodeRuleTest.php @@ -86,7 +86,7 @@ public function testRule(): void $this->analyse([__DIR__ . '/data/attribute-reflection.php'], [ [ '#[AttributeReflectionTest\MyAttr(one: 7, two: 8)], $test: #[AttributeReflectionTest\MyAttr(one: 9, two: 10)]', - 28, + 29, ], [ '#[AttributeReflectionTest\MyAttr()]', diff --git a/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php b/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php index 87319aa44dd..d8bca55b6fc 100644 --- a/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php +++ b/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php @@ -221,15 +221,15 @@ public function testBug3997(): void $this->analyse([__DIR__ . '/data/bug-3997.php'], [ [ 'Return type (int) of method Bug3997\Baz::count() should be covariant with return type (int<0, max>) of method Countable::count()', - 35, + 36, ], [ 'Return type (int) of method Bug3997\Lorem::count() should be covariant with return type (int<0, max>) of method Countable::count()', - 49, + 50, ], [ 'Return type (string) of method Bug3997\Ipsum::count() should be compatible with return type (int<0, max>) of method Countable::count()', - 63, + 64, ], ]); } diff --git a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php index 07d47bb48ad..9603b2ee52e 100644 --- a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php +++ b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php @@ -659,7 +659,7 @@ public function testBug10101(): void $this->analyse([__DIR__ . '/data/bug-10101.php'], [ [ 'Return type mixed of method Bug10101\B::next() is not covariant with return type void of method Bug10101\A::next().', - 10, + 11, ], ]); } @@ -690,7 +690,7 @@ public function testBug10149(): void $errors = [ [ 'Method Bug10149\StdSat::__get() has #[\Override] attribute but does not override any method.', - 10, + 11, ], ]; $this->analyse([__DIR__ . '/data/bug-10149.php'], $errors); @@ -729,11 +729,11 @@ public function testOverrideAttribute(): void $this->analyse([__DIR__ . '/data/override-attribute.php'], [ [ 'Method OverrideAttribute\Bar::test2() has #[\Override] attribute but does not override any method.', - 24, + 25, ], [ 'Method OverrideAttribute\ChildOfParentWithConstructor::__construct() has #[\Override] attribute but does not override any method.', - 42, + 43, ], ]); }