Skip to content
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

Hooked properties cannot be unset() #3842

Merged
merged 8 commits into from
Mar 2, 2025
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Feb 28, 2025

@staabm staabm marked this pull request as ready for review February 28, 2025 09:37
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

One more thing - if we're on PHP 8.4+ (phpVersion::supportsPropertyHooks), and the property is not final, and the declaring class is not final, and the property is not private, report a similar error even if the property does not have hooks. Preferably with a different explaining error message. Because a subclass might create a hooked declaration for the same property.

We're already doing a similar thing here (accessing/writing a property that might be hooked in a subclass can throw any exception):

$declaringClass = $propertyReflection->getDeclaringClass();
if (!$propertyReflection->hasHook($hookName)) {
if (
$propertyReflection->isPrivate()
|| $propertyReflection->isFinal()->yes()
|| $declaringClass->isFinal()
) {
return [];
}
if ($this->implicitThrows) {
return [ThrowPoint::createImplicit($scope, $propertyFetch)];
}
return [];

staabm and others added 4 commits March 1, 2025 18:09

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@staabm
Copy link
Contributor Author

staabm commented Mar 1, 2025

One more thing - if we're on PHP 8.4+ (phpVersion::supportsPropertyHooks), and the property is not final, and the declaring class is not final, and the property is not private, report a similar error even if the property does not have hooks. Preferably with a different explaining error message. Because a subclass might create a hooked declaration for the same property.

this sounds like a edge case which should only be reported via a opt-in flag..?

@staabm
Copy link
Contributor Author

staabm commented Mar 1, 2025

while testing I found https://phpstan.org/r/0299facc-d23d-4ed8-9567-e1c7c8c30536 which I think should not report a error on line 9 on PHP 8.4, right?

https://3v4l.org/ZD4oE

ondrejmirtes and others added 3 commits March 2, 2025 09:44
Comment on lines +121 to +125
if (
!$propertyReflection->isPrivate()
&& !$propertyReflection->isFinal()->yes()
&& !$propertyReflection->getDeclaringClass()->isFinal()
) {
Copy link
Contributor Author

@staabm staabm Mar 2, 2025

Choose a reason for hiding this comment

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

I also though about adding

public function isHookedInSubclass(): TrinaryLogic {
		if (
			!$this->isPrivate()
			&& !$this->isFinal()->yes()
			&& !$this->getDeclaringClass()->isFinal()
		) {
			return TrinaryLogic::createMaybe();
		}

		return TrinaryLogic::createNo();
	}

to PhpPropertyReflection. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Not needed right now :) I'd have to this about that more.

@ondrejmirtes ondrejmirtes merged commit b143449 into phpstan:2.1.x Mar 2, 2025
414 of 417 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the hook-unset branch March 2, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants