-
Notifications
You must be signed in to change notification settings - Fork 529
Fix false positive of access to static private property of parent class #1989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
just realized that this rule also missed a separate error case, which the phpstan-src/src/Rules/Properties/AccessPropertiesRule.php Lines 124 to 135 in 80ba5a0
added the missing check and a test |
This pull request has been marked as ready for review. |
@@ -184,6 +185,26 @@ private function processSingleProperty(Scope $scope, StaticPropertyFetch $node, | |||
return $messages; | |||
} | |||
|
|||
if ($typeForDescribe instanceof TypeWithClassName && $typeForDescribe->getClassReflection() !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- typeForDescribe is just for error message description
instanceof *Type
is always wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored it to make it more like
phpstan-src/src/Rules/Properties/AccessPropertiesRule.php
Lines 119 to 139 in 582a9cb
if (count($classNames) === 1) { | |
$referencedClass = $typeResult->getReferencedClasses()[0]; | |
$propertyClassReflection = $this->reflectionProvider->getClass($referencedClass); | |
$parentClassReflection = $propertyClassReflection->getParentClass(); | |
while ($parentClassReflection !== null) { | |
if ($parentClassReflection->hasProperty($name)) { | |
if ($scope->canAccessProperty($parentClassReflection->getProperty($name, $scope))) { | |
return []; | |
} | |
return [ | |
RuleErrorBuilder::message(sprintf( | |
'Access to private property $%s of parent class %s.', | |
$name, | |
$parentClassReflection->getDisplayName(), | |
))->build(), | |
]; | |
} | |
$parentClassReflection = $parentClassReflection->getParentClass(); | |
} | |
} |
foreach ($typeForDescribe->getClassReflection()->getParents() as $parentClassReflection) { | ||
if (!$parentClassReflection->hasProperty($name)) { | ||
continue; | ||
$classNames = $classType->getReferencedClasses(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also wrong. getReferencedClasses
will return nested classes too. Which means that if you get Foo<Bar>
, getReferencedClasses will contain both Foo and Bar.
I think that in context of this PR, you need TypeUtils::getDirectClassNames
. Feel free to fix the other PR too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this point I just did what you said. I don't know what I am doing here right now.
Feel free to fix the other PR too.
not sure which one you mean.
I will be in a meeting for the rest of the day. would be great in case you already know what needs to be done, if you could finish it.
thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said: refactored it to make it more like
phpstan-src/src/Rules/Properties/AccessPropertiesRule.php
Lines 119 to 139 in 582a9cb
if (count($classNames) === 1) { | |
$referencedClass = $typeResult->getReferencedClasses()[0]; | |
$propertyClassReflection = $this->reflectionProvider->getClass($referencedClass); | |
$parentClassReflection = $propertyClassReflection->getParentClass(); | |
while ($parentClassReflection !== null) { | |
if ($parentClassReflection->hasProperty($name)) { | |
if ($scope->canAccessProperty($parentClassReflection->getProperty($name, $scope))) { | |
return []; | |
} | |
return [ | |
RuleErrorBuilder::message(sprintf( | |
'Access to private property $%s of parent class %s.', | |
$name, | |
$parentClassReflection->getDisplayName(), | |
))->build(), | |
]; | |
} | |
$parentClassReflection = $parentClassReflection->getParentClass(); | |
} | |
} |
But that other rule is wrong too, not sure for how long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. I pushed a fix. hopefully thats what you had in mind.
thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that other rule is wrong too, not sure for how long.
btw: I found the commit staabm@455fb7b
Thank you! |
closes phpstan/phpstan#8333