Check for impure property hooks

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

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

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,

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[]
 	 */

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()
 					)
 				) {

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) {
@@ -112,7 +115,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 +149,7 @@ public function processPropertyHooks(
 				$gatheredReturnStatements,
 				$statementResult,
 				$executionEnds,
-				array_merge($statementResult->getImpurePoints(), $methodImpurePoints),
+				array_merge($statementResult->getImpurePoints(), $hookImpurePoints),
 				$classReflection,
 				$hookReflection,
 				$propertyReflection,

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

src/Rules/Pure/PurePropertyHookRule.php

@@ -0,0 +1,57 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Pure;
+
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\DependencyInjection\RegisteredRule;
+use PHPStan\Node\PropertyHookReturnStatementsNode;
+use PHPStan\Rules\Rule;
+use PHPStan\ShouldNotHappenException;
+use function sprintf;
+use function ucfirst;
+
+/**
+ * @implements Rule<PropertyHookReturnStatementsNode>
+ */
+#[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,
+		);
+	}
+
+}

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

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
 	{

tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php

@@ -0,0 +1,55 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Pure;
+
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+use PHPUnit\Framework\Attributes\RequiresPhp;
+
+/**
+ * @extends RuleTestCase<PurePropertyHookRule>
+ */
+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,
+			],
+		]);
+	}
+
+}