Skip to content

Fix false positive of access to static private property of parent class#1989

Merged
ondrejmirtes merged 6 commits intophpstan:1.9.xfrom
staabm:static
Nov 18, 2022
Merged

Fix false positive of access to static private property of parent class#1989
ondrejmirtes merged 6 commits intophpstan:1.9.xfrom
staabm:static

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Nov 9, 2022

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Nov 9, 2022

just realized that this rule also missed a separate error case, which theAccessPropertiesRule contained in

phpstan-src/src/Rules/Properties/AccessPropertiesRule.php

Lines 124 to 135 in 80ba5a0

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(),
];
}

added the missing check and a test

@staabm staabm marked this pull request as ready for review November 9, 2022 22:59
@phpstan-bot
Copy link
Copy Markdown
Collaborator

phpstan-bot commented Nov 9, 2022

This pull request has been marked as ready for review.

ondrejmirtes
ondrejmirtes requested changes Nov 10, 2022
View reviewed changes
src/Rules/Properties/AccessStaticPropertiesRule.php Outdated
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. typeForDescribe is just for error message description
  2. instanceof *Type is always wrong

Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 10, 2022

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();
}
}

ondrejmirtes
ondrejmirtes requested changes Nov 10, 2022
View reviewed changes
src/Rules/Properties/AccessStaticPropertiesRule.php Outdated
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Nov 10, 2022

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.

Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 10, 2022

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

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Nov 10, 2022

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.

Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 11, 2022

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

Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 11, 2022

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

@ondrejmirtes ondrejmirtes merged commit 173091d into phpstan:1.9.x Nov 18, 2022
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Nov 18, 2022

Thank you!

@staabm staabm deleted the static branch June 21, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive: Access to private property $root of parent class Foo.

3 participants

@staabm @phpstan-bot @ondrejmirtes