Fix phpstan/phpstan#14398: Incorrect line number for "Private method overriding public method should be public"#5381
Closed
phpstan-bot wants to merge 3 commits intophpstan:2.1.xfrom
Closed
Conversation
…y 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
Contributor
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Just add a non optional Node argument directly to the method.
…tead 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 <noreply@anthropic.com>
Collaborator
Author
|
Full test suite passed as well. All changes are committed and pushed. Both |
Contributor
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Check for other rule related to InClassMethodNode::class;
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 <noreply@anthropic.com>
Collaborator
Author
|
The background |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a method has attributes (like
#[Override]), the "Private method overriding public/protected method" error was reported on the first attribute line instead of the actual method declaration line. This fix ensures the error points to the correct line.Changes
$lineparameter toMethodVisibilityComparisonHelper::compare()insrc/Rules/Methods/MethodVisibilityComparisonHelper.phpOverridingMethodRule::processNode()insrc/Rules/Methods/OverridingMethodRule.phpto pass$node->getOriginalNode()->name->getStartLine()as the line numberConsistentConstructorRule::processNode()insrc/Rules/Methods/ConsistentConstructorRule.phpsimilarlytests/PHPStan/Rules/Methods/data/bug-14398.phptestBug14398()intests/PHPStan/Rules/Methods/OverridingMethodRuleTest.phpRoot cause
InClassMethodNodeinherits its start line from the underlyingClassMethodAST node, which includes attribute lines. When rules return errors without an explicit.line()call,RuleErrorTransformeruses$node->getStartLine()as the default, which points to the first attribute rather than thefunctionkeyword. The fix explicitly sets the line on visibility errors to the method name's start line.Test
The regression test defines a class with a
#[Override]attribute on a private method that overrides a protected parent method. It verifies the error is reported on line 17 (theprivate functiondeclaration) rather than line 16 (the#[Override]attribute).Fixes phpstan/phpstan#14398