From 849abbd43fc56caaec8400db0b5bc92c5a212a76 Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Sun, 23 Aug 2026 09:23:32 +0200 Subject: [PATCH 1/2] Fix check for pure property hooks --- src/Analyser/PropertyHooksProcessor.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Analyser/PropertyHooksProcessor.php b/src/Analyser/PropertyHooksProcessor.php index 5c38429145..f7a33a4ea8 100644 --- a/src/Analyser/PropertyHooksProcessor.php +++ b/src/Analyser/PropertyHooksProcessor.php @@ -112,7 +112,7 @@ public function processPropertyHooks( $gatheredReturnStatements = []; $executionEnds = []; - $methodImpurePoints = []; + $hookImpurePoints = []; $statementResult = $nodeScopeResolver->processStmtNodesInternal(new PropertyHookStatementNode($hook), $stmts, $hookScope, $storage, new GatheringNodeCallback(static function (Node $node, Scope $scope) use ($hookScope, &$gatheredReturnStatements, &$executionEnds, &$hookImpurePoints): void { if ($scope->getFunction() !== $hookScope->getFunction()) { return; @@ -146,7 +146,7 @@ public function processPropertyHooks( $gatheredReturnStatements, $statementResult, $executionEnds, - array_merge($statementResult->getImpurePoints(), $methodImpurePoints), + array_merge($statementResult->getImpurePoints(), $hookImpurePoints), $classReflection, $hookReflection, $propertyReflection, From be68e36d13327b0b2860659b39c8ebe5e7ecd60b Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Sun, 23 Aug 2026 13:37:51 +0200 Subject: [PATCH 2/2] Check for property hooks being pure/impure --- .../ExprHandler/PropertyFetchHandler.php | 1 + src/Analyser/ImpurePoint.php | 2 +- src/Analyser/MutatingScope.php | 3 +- src/Analyser/NodeScopeResolver.php | 64 +++++++++++++ src/Analyser/PhpDocsResolver.php | 6 +- src/Analyser/PropertyHooksProcessor.php | 7 +- src/Rules/Pure/FunctionPurityCheck.php | 2 +- src/Rules/Pure/PurePropertyHookRule.php | 57 +++++++++++ .../Rules/Pure/PureFunctionRuleTest.php | 19 ++++ .../PHPStan/Rules/Pure/PureMethodRuleTest.php | 12 +++ .../Rules/Pure/PurePropertyHookRuleTest.php | 55 +++++++++++ .../Pure/data/property-hook-impure-point.php | 96 +++++++++++++++++++ .../Rules/Pure/data/pure-property-hook.php | 90 +++++++++++++++++ 13 files changed, 408 insertions(+), 6 deletions(-) create mode 100644 src/Rules/Pure/PurePropertyHookRule.php create mode 100644 tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php create mode 100644 tests/PHPStan/Rules/Pure/data/property-hook-impure-point.php create mode 100644 tests/PHPStan/Rules/Pure/data/pure-property-hook.php diff --git a/src/Analyser/ExprHandler/PropertyFetchHandler.php b/src/Analyser/ExprHandler/PropertyFetchHandler.php index efb6d007bb..7772ef9ad3 100644 --- a/src/Analyser/ExprHandler/PropertyFetchHandler.php +++ b/src/Analyser/ExprHandler/PropertyFetchHandler.php @@ -91,6 +91,7 @@ public function composeResult(NodeScopeResolver $nodeScopeResolver, PropertyFetc if ($propertyDeclaringClass->hasNativeProperty($propertyName)) { $nativeProperty = $propertyDeclaringClass->getNativeProperty($propertyName); $throwPoints = array_merge($throwPoints, $this->propertyHookThrowPointsResolver->getThrowPointsFromPropertyHook($scopeBeforeVar, $expr, $nativeProperty, 'get')); + $impurePoints = array_merge($impurePoints, $nodeScopeResolver->getImpurePointsFromPropertyHook($scopeBeforeVar, $expr, $nativeProperty, 'get')); } } } diff --git a/src/Analyser/ImpurePoint.php b/src/Analyser/ImpurePoint.php index dc9e9a6091..a7d5d6f864 100644 --- a/src/Analyser/ImpurePoint.php +++ b/src/Analyser/ImpurePoint.php @@ -6,7 +6,7 @@ use PHPStan\Node\VirtualNode; /** - * @phpstan-type ImpurePointIdentifier = 'echo'|'die'|'exit'|'propertyAssign'|'propertyAssignByRef'|'propertyUnset'|'methodCall'|'new'|'functionCall'|'include'|'require'|'print'|'eval'|'superglobal'|'yield'|'yieldFrom'|'static'|'global'|'betweenPhpTags'|'staticPropertyAccess' + * @phpstan-type ImpurePointIdentifier = 'echo'|'die'|'exit'|'propertyAssign'|'propertyAssignByRef'|'propertyUnset'|'propertyHookCall'|'methodCall'|'new'|'functionCall'|'include'|'require'|'print'|'eval'|'superglobal'|'yield'|'yieldFrom'|'static'|'global'|'betweenPhpTags'|'staticPropertyAccess' * @api */ final class ImpurePoint diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 716c48a767..244f13ff7a 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -1731,6 +1731,7 @@ public function enterPropertyHook( ?Type $throwType, ?string $deprecatedDescription, bool $isDeprecated, + ?bool $isPure, ?string $phpDocComment, ?ResolvedPhpDocBlock $resolvedPhpDocBlock = null, ): self @@ -1792,7 +1793,7 @@ public function enterPropertyHook( $isDeprecated, false, false, - false, + $isPure, true, Assertions::createEmpty(), null, diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 95bdb58e73..331919c409 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -70,7 +70,9 @@ use PHPStan\Reflection\ParameterReflection; use PHPStan\Reflection\ParametersAcceptor; use PHPStan\Reflection\ParametersAcceptorSelector; +use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection; use PHPStan\Reflection\Php\PhpMethodReflection; +use PHPStan\Reflection\Php\PhpPropertyReflection; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Properties\ReadWritePropertiesExtension; use PHPStan\ShouldNotHappenException; @@ -110,6 +112,7 @@ use function is_int; use function is_string; use function max; +use function sprintf; use function usort; #[AutowiredService] @@ -881,6 +884,67 @@ public function processExprNode( return $expressionResult; } + /** + * Unlike a method call, a property read defaults to pure: only a hook we're + * certain about and that is certainly side-effecting makes the read impure. + * + * The reset is assumed pure as reporting those would make accessing them + * unreasonably annoying. + * + * @param 'get'|'set' $hookName + * @return ImpurePoint[] + */ + public function getImpurePointsFromPropertyHook( + MutatingScope $scope, + PropertyFetch $propertyFetch, + PhpPropertyReflection $propertyReflection, + string $hookName, + ): array + { + if ($this->isPropertyHookBackingValueAccess($scope, $propertyFetch)) { + return []; + } + + if (!$propertyReflection->hasHook($hookName)) { + return []; + } + + if (!$propertyReflection->getHook($hookName)->hasSideEffects()->yes()) { + return []; + } + + return [ + new ImpurePoint( + $scope, + $propertyFetch, + 'propertyHookCall', + sprintf( + 'call to %s hook of property %s::$%s', + $hookName, + $propertyReflection->getDeclaringClass()->getDisplayName(), + $propertyReflection->getName(), + ), + true, + ), + ]; + } + + /** + * Inside a hook of the same property, $this->prop is the backing value, not + * a re-entrant hook call. + */ + private function isPropertyHookBackingValueAccess(MutatingScope $scope, PropertyFetch $propertyFetch): bool + { + $scopeFunction = $scope->getFunction(); + + return $scopeFunction instanceof PhpMethodFromParserNodeReflection + && $scopeFunction->isPropertyHook() + && $propertyFetch->var instanceof Variable + && $propertyFetch->var->name === 'this' + && $propertyFetch->name instanceof Identifier + && $propertyFetch->name->toString() === $scopeFunction->getHookedPropertyName(); + } + /** * @return string[] */ diff --git a/src/Analyser/PhpDocsResolver.php b/src/Analyser/PhpDocsResolver.php index abdbeeb2e5..961cca79d5 100644 --- a/src/Analyser/PhpDocsResolver.php +++ b/src/Analyser/PhpDocsResolver.php @@ -212,12 +212,16 @@ public function getPhpDocs(Scope $scope, Node\FunctionLike|Node\Stmt\Property $n } if ($isPure === null && $node instanceof Node\FunctionLike && $scope->isInClass()) { + // a set hook has no return type node of its own, but it always returns + // void - the class-level @phpstan-pure must not make it pure + $isSetHook = $node instanceof Node\PropertyHook && $node->name->toLowerString() === 'set'; $classResolvedPhpDoc = $scope->getClassReflection()->getResolvedPhpDoc(); if ($classResolvedPhpDoc !== null && $classResolvedPhpDoc->areAllMethodsPure()) { if ( strtolower($functionName ?? '') === '__construct' || ( - ($phpDocReturnType === null || !$phpDocReturnType->isVoid()->yes()) + !$isSetHook + && ($phpDocReturnType === null || !$phpDocReturnType->isVoid()->yes()) && !$scope->getFunctionType($node->getReturnType(), false, false)->isVoid()->yes() ) ) { diff --git a/src/Analyser/PropertyHooksProcessor.php b/src/Analyser/PropertyHooksProcessor.php index f7a33a4ea8..821c51ff7a 100644 --- a/src/Analyser/PropertyHooksProcessor.php +++ b/src/Analyser/PropertyHooksProcessor.php @@ -59,7 +59,7 @@ public function processPropertyHooks( $nodeScopeResolver->callNodeCallback($nodeCallback, $hook, $scope, $storage); $nodeScopeResolver->processAttributeGroups($stmt, $hook->attrGroups, $scope, $storage, $nodeCallback); - [, $phpDocParameterTypes,,,, $phpDocThrowType,,,,,,,, $phpDocComment,,,,,, $resolvedPhpDoc] = $this->phpDocsResolver->getPhpDocs($scope, $hook); + [, $phpDocParameterTypes,,,, $phpDocThrowType,,,,, $isPure,,, $phpDocComment,,,,,, $resolvedPhpDoc] = $this->phpDocsResolver->getPhpDocs($scope, $hook); foreach ($hook->params as $param) { $nodeScopeResolver->processParamNode($stmt, $param, $scope, $storage, $nodeCallback); @@ -76,6 +76,7 @@ public function processPropertyHooks( $phpDocThrowType, $deprecatedDescription, $isDeprecated, + $isPure, $phpDocComment, $resolvedPhpDoc, ); @@ -99,7 +100,9 @@ public function processPropertyHooks( $stmts = $hook->getStmts(); if ($stmts === null) { - return; + // abstract hook - the sibling hook of the same property may still + // have a body, so keep going + continue; } if ($hook->body instanceof Expr) { diff --git a/src/Rules/Pure/FunctionPurityCheck.php b/src/Rules/Pure/FunctionPurityCheck.php index bbe0c7bfdd..be1ec07a5a 100644 --- a/src/Rules/Pure/FunctionPurityCheck.php +++ b/src/Rules/Pure/FunctionPurityCheck.php @@ -31,7 +31,7 @@ final class FunctionPurityCheck { /** - * @param 'Function'|'Method' $identifier + * @param 'Function'|'Method'|'PropertyHook' $identifier * @param ExtendedParameterReflection[] $parameters * @param ImpurePoint[] $impurePoints * @param ThrowPoint[] $throwPoints diff --git a/src/Rules/Pure/PurePropertyHookRule.php b/src/Rules/Pure/PurePropertyHookRule.php new file mode 100644 index 0000000000..abda58798f --- /dev/null +++ b/src/Rules/Pure/PurePropertyHookRule.php @@ -0,0 +1,57 @@ + + */ +#[RegisteredRule(level: 2)] +final class PurePropertyHookRule implements Rule +{ + + public function __construct(private FunctionPurityCheck $check) + { + } + + public function getNodeType(): string + { + return PropertyHookReturnStatementsNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $hookReflection = $node->getHookReflection(); + $hookName = $hookReflection->getPropertyHookName(); + if ($hookName === null) { + throw new ShouldNotHappenException(); + } + + return $this->check->check( + $scope, + sprintf( + '%s hook for property %s::$%s', + ucfirst($hookName), + $hookReflection->getDeclaringClass()->getDisplayName(), + $hookReflection->getHookedPropertyName(), + ), + 'PropertyHook', + $hookReflection, + $hookReflection->getParameters(), + $hookReflection->getReturnType(), + $node->getImpurePoints(), + $node->getStatementResult()->getThrowPoints(), + $node->getPropertyHookNode()->getStmts() ?? [], + false, + ); + } + +} diff --git a/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php b/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php index 1a4d3c3fbb..dac0917d59 100644 --- a/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php +++ b/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php @@ -378,4 +378,23 @@ public function testPureUnlessCallableIsImpurePhp84(): void ]); } + #[RequiresPhp('>= 8.4.0')] + public function testPropertyHookImpurePoint(): void + { + $this->analyse([__DIR__ . '/data/property-hook-impure-point.php'], [ + [ + 'Impure call to get hook of property PropertyHookImpurePoint\\Foo::$impureGet in pure function PropertyHookImpurePoint\\readImpureGet().', + 56, + ], + [ + 'Impure call to get hook of property PropertyHookImpurePoint\\Foo::$impureGet in pure function PropertyHookImpurePoint\\readImpureGetNullsafe().', + 62, + ], + [ + 'Impure call to get hook of property PropertyHookImpurePoint\\Foo::$impureGet in pure function PropertyHookImpurePoint\\readImpureGetInCompoundAssign().', + 69, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php b/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php index a31dcfe0bd..61e32c9d60 100644 --- a/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php +++ b/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php @@ -395,6 +395,18 @@ public function testBug14511(): void $this->analyse([__DIR__ . '/data/bug-14511-method.php'], []); } + #[RequiresPhp('>= 8.4.0')] + public function testPropertyHookImpurePoint(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/property-hook-impure-point.php'], [ + [ + 'Impure call to get hook of property PropertyHookImpurePoint\Foo::$impureGet in pure method PropertyHookImpurePoint\Foo::readOwnImpureGet().', + 42, + ], + ]); + } + #[RequiresPhp('>= 8.1.0')] public function testBug14557(): void { diff --git a/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php b/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php new file mode 100644 index 0000000000..4d8bb6d17a --- /dev/null +++ b/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php @@ -0,0 +1,55 @@ + + */ +class PurePropertyHookRuleTest extends RuleTestCase +{ + + public function getRule(): Rule + { + return new PurePropertyHookRule(new FunctionPurityCheck()); + } + + #[RequiresPhp('>= 8.4.0')] + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/pure-property-hook.php'], [ + [ + 'Impure echo in pure get hook for property PurePropertyHook\Foo::$pureGetWithSideEffect.', + 15, + ], + [ + 'Get hook for property PurePropertyHook\Foo::$impureGetWithoutSideEffect is marked as impure but does not have any side effects.', + 28, + ], + [ + 'Set hook for property PurePropertyHook\Foo::$pureSet is marked as pure but returns void.', + 50, + ], + [ + 'Impure property assignment in pure set hook for property PurePropertyHook\Foo::$pureSet.', + 51, + ], + [ + 'Get hook for property PurePropertyHook\NotFinal::$finalImpureGetWithoutSideEffect is marked as impure but does not have any side effects.', + 74, + ], + [ + 'Set hook for property PurePropertyHook\AbstractGetHookFollowedBySetHook::$mixedHooks is marked as pure but returns void.', + 85, + ], + [ + 'Impure echo in pure set hook for property PurePropertyHook\AbstractGetHookFollowedBySetHook::$mixedHooks.', + 86, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Pure/data/property-hook-impure-point.php b/tests/PHPStan/Rules/Pure/data/property-hook-impure-point.php new file mode 100644 index 0000000000..e48ad23540 --- /dev/null +++ b/tests/PHPStan/Rules/Pure/data/property-hook-impure-point.php @@ -0,0 +1,96 @@ += 8.4 + +declare(strict_types = 1); + +namespace PropertyHookImpurePoint; + +final class Foo +{ + + private int $backing = 1; + + public int $plain = 1; + + public int $impureGet { + /** @phpstan-impure */ + get { + echo 'side effect'; + + return $this->backing; + } + } + + public int $pureGet { + /** @phpstan-pure */ + get => $this->backing; + } + + public int $unannotatedGet { + get => $this->backing; + } + + public int $impureSet { + /** @phpstan-impure */ + set { + $this->impureSet = $value; + } + } + + /** @phpstan-pure */ + public function readOwnImpureGet(): int + { + return $this->impureGet; + } + + /** @phpstan-pure */ + public function readOwnPureGet(): int + { + return $this->pureGet; + } + +} + +/** @phpstan-pure */ +function readImpureGet(Foo $foo): int +{ + return $foo->impureGet; +} + +/** @phpstan-pure */ +function readImpureGetNullsafe(?Foo $foo): ?int +{ + return $foo?->impureGet; +} + +/** @phpstan-pure */ +function readImpureGetInCompoundAssign(Foo $foo): int +{ + $i = 0; + $i += $foo->impureGet; + + return $i; +} + +/** @phpstan-pure */ +function readPureGet(Foo $foo): int +{ + return $foo->pureGet; +} + +/** @phpstan-pure */ +function readUnannotatedGet(Foo $foo): int +{ + return $foo->unannotatedGet; +} + +/** @phpstan-pure */ +function readPlainProperty(Foo $foo): int +{ + return $foo->plain; +} + +/** @phpstan-pure */ +function readUnknownProperty(object $o): mixed +{ + return $o->whatever; +} diff --git a/tests/PHPStan/Rules/Pure/data/pure-property-hook.php b/tests/PHPStan/Rules/Pure/data/pure-property-hook.php new file mode 100644 index 0000000000..24889499b0 --- /dev/null +++ b/tests/PHPStan/Rules/Pure/data/pure-property-hook.php @@ -0,0 +1,90 @@ += 8.4 + +declare(strict_types = 1); + +namespace PurePropertyHook; + +final class Foo +{ + + private int $backing = 1; + + public int $pureGetWithSideEffect { + /** @phpstan-pure */ + get { + echo 'side effect'; + + return $this->backing; + } + } + + public int $pureGet { + /** @phpstan-pure */ + get => $this->backing; + } + + public int $impureGetWithoutSideEffect { + /** @phpstan-impure */ + get => $this->backing; + } + + public int $impureGet { + /** @phpstan-impure */ + get { + echo 'side effect'; + + return $this->backing; + } + } + + public int $unannotatedGet { + get { + echo 'side effect'; + + return $this->backing; + } + } + + public int $pureSet { + /** @phpstan-pure */ + set { + $this->pureSet = $value; + } + } + + public int $impureSet { + /** @phpstan-impure */ + set { + $this->impureSet = $value; + } + } + +} + +class NotFinal +{ + + public int $impureGetWithoutSideEffect { + /** @phpstan-impure */ + get => 1; + } + + public int $finalImpureGetWithoutSideEffect { + /** @phpstan-impure */ + final get => 1; + } + +} + +abstract class AbstractGetHookFollowedBySetHook +{ + + abstract public int $mixedHooks { + get; + /** @phpstan-pure */ + set { + echo 'side effect'; + } + } + +}