Improve impure tip

Author: ondrejmirtes

src/Analyser/MutatingScope.php

@@ -787,8 +787,9 @@ public function getMaybeDefinedVariables(): array
 	public function findPossiblyImpureCallDescriptions(Expr $expr): array
 	{
 		$nodeFinder = new NodeFinder();
-		$descriptions = [];
+		$callExprDescriptions = [];
 		$foundCallExprMatch = false;
+		$matchedCallExprKeys = [];
 		foreach ($this->expressionTypes as $holder) {
 			$holderExpr = $holder->getExpr();
 			if (!$holderExpr instanceof PossiblyImpureCallExpr) {
@@ -810,6 +811,7 @@ public function findPossiblyImpureCallDescriptions(Expr $expr): array
 			}
 
 			$foundCallExprMatch = true;
+			$matchedCallExprKeys[$callExprKey] = true;
 
 			// Only show the tip when the scope's type for the call expression
 			// differs from the declared return type, meaning control flow
@@ -821,58 +823,70 @@ public function findPossiblyImpureCallDescriptions(Expr $expr): array
 				continue;
 			}
 
-			$descriptions[] = $holderExpr->getCallDescription();
-		}
-
-		if (count($descriptions) > 0) {
-			return array_values(array_unique($descriptions));
+			$callExprDescriptions[] = $holderExpr->getCallDescription();
 		}
 
 		// If the first pass found a callExpr in the error expression but
 		// filtered it out (return type wasn't narrowed), the error is
 		// explained by the return type alone - skip the fallback.
-		if ($foundCallExprMatch) {
+		if ($foundCallExprMatch && count($callExprDescriptions) === 0) {
 			return [];
 		}
 
-		// Fallback: match by impactedExpr for cases where a maybe-impure method
+		// Second pass: match by impactedExpr for cases where a maybe-impure method
 		// on an object didn't invalidate it, but a different method's return
 		// value was narrowed on that object.
 		// Skip when the expression itself is a direct method/static call -
 		// those are passed by ImpossibleCheckType rules where the error is
 		// about the call's arguments, not about object state.
-		if ($expr instanceof Expr\MethodCall || $expr instanceof Expr\StaticCall) {
-			return [];
-		}
-		foreach ($this->expressionTypes as $holder) {
-			$holderExpr = $holder->getExpr();
-			if (!$holderExpr instanceof PossiblyImpureCallExpr) {
-				continue;
-			}
+		if (!($expr instanceof Expr\MethodCall || $expr instanceof Expr\StaticCall)) {
+			$impactedExprDescriptions = [];
+			foreach ($this->expressionTypes as $holder) {
+				$holderExpr = $holder->getExpr();
+				if (!$holderExpr instanceof PossiblyImpureCallExpr) {
+					continue;
+				}
 
-			$impactedExprKey = $this->getNodeKey($holderExpr->impactedExpr);
+				$impactedExprKey = $this->getNodeKey($holderExpr->impactedExpr);
 
-			// Skip if impactedExpr is the same as callExpr (function calls)
-			if ($impactedExprKey === $this->getNodeKey($holderExpr->callExpr)) {
-				continue;
-			}
+				// Skip if impactedExpr is the same as callExpr (function calls)
+				if ($impactedExprKey === $this->getNodeKey($holderExpr->callExpr)) {
+					continue;
+				}
 
-			$found = $nodeFinder->findFirst([$expr], function (Node $node) use ($impactedExprKey): bool {
-				if (!$node instanceof Expr) {
-					return false;
+				// Skip if this entry's callExpr was already matched in the first pass
+				$callExprKey = $this->getNodeKey($holderExpr->callExpr);
+				if (isset($matchedCallExprKeys[$callExprKey])) {
+					continue;
 				}
 
-				return $this->getNodeKey($node) === $impactedExprKey;
-			});
+				$found = $nodeFinder->findFirst([$expr], function (Node $node) use ($impactedExprKey): bool {
+					if (!$node instanceof Expr) {
+						return false;
+					}
 
-			if ($found === null) {
-				continue;
+					return $this->getNodeKey($node) === $impactedExprKey;
+				});
+
+				if ($found === null) {
+					continue;
+				}
+
+				$impactedExprDescriptions[] = $holderExpr->getCallDescription();
 			}
 
-			$descriptions[] = $holderExpr->getCallDescription();
+			// Prefer impactedExpr matches (intermediate calls that could have
+			// invalidated the object) over callExpr matches
+			if (count($impactedExprDescriptions) > 0) {
+				return array_values(array_unique($impactedExprDescriptions));
+			}
+		}
+
+		if (count($callExprDescriptions) > 0) {
+			return array_values(array_unique($callExprDescriptions));
 		}
 
-		return array_values(array_unique($descriptions));
+		return [];
 	}
 
 	private function isGlobalVariable(string $variableName): bool

tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php

@@ -1125,14 +1125,27 @@ public function testPossiblyImpureTip(): void
 			// Object invalidation: @phpstan-impure intermediate - no error ($this invalidated)
 			// Object invalidation: void intermediate - no error ($this invalidated)
 
+			// Intermediate maybe-impure call takes priority over direct call
+			[
+				'Strict comparison using === between 1 and 2 will always evaluate to false.',
+				294,
+				'If PossiblyImpureTip\IntermediateCallPriority::next() is impure, add <fg=cyan>@phpstan-impure</> PHPDoc tag above its declaration.',
+			],
+			// No intermediate call: tip points to fetch() itself
+			[
+				'Strict comparison using === between 1 and 2 will always evaluate to false.',
+				303,
+				'If PossiblyImpureTip\IntermediateCallPriority::fetch() is impure, add <fg=cyan>@phpstan-impure</> PHPDoc tag above its declaration.',
+			],
+
 			// No tip when return type alone explains the error
 			[
 				'Strict comparison using === between string and null will always evaluate to false.',
-				287,
+				324,
 			],
 			[
 				'Strict comparison using !== between string and null will always evaluate to true.',
-				291,
+				328,
 			],
 		]);
 	}

tests/PHPStan/Rules/Comparison/data/possibly-impure-tip.php

@@ -270,6 +270,43 @@ public function testVoidIntermediate(): void
 
 }
 
+// --- Intermediate maybe-impure call takes priority over direct call ---
+
+class IntermediateCallPriority
+{
+
+	public function fetch(): int
+	{
+		return rand(1, 100);
+	}
+
+	public function next(): bool
+	{
+		return true;
+	}
+
+	public function testIntermediateCallTipPriority(): void
+	{
+		// fetch() narrowed to 1, next() is intermediate maybe-impure call
+		// tip should point to next(), not fetch()
+		if ($this->fetch() === 1) {
+			$this->next();
+			if ($this->fetch() === 2) { // always false, tip for next()
+			}
+		}
+	}
+
+	public function testNoIntermediateCall(): void
+	{
+		// No intermediate call: tip should point to fetch() itself
+		if ($this->fetch() === 1) {
+			if ($this->fetch() === 2) { // always false, tip for fetch()
+			}
+		}
+	}
+
+}
+
 // --- No tip when return type alone explains the error ---
 
 class NoTipWhenReturnTypeExplains