Invalidate maybe-impure function return values after impure method/static calls#5667
Invalidate maybe-impure function return values after impure method/static calls#5667phpstan-bot wants to merge 9 commits into
Conversation
…atic calls When a maybe-impure function call's return value was narrowed via assert() (e.g. assert(count(MyRecord::find()) === 1)), the narrowed type persisted even after an impure method call like $msg2->insert(). This happened because invalidateExpression() only invalidated expressions containing the specific callee variable, not unrelated maybe-impure expressions whose results could have been affected by the side effects. Add invalidateAllMaybeImpureFunctionReturnValues() to MutatingScope which walks stored expression types and removes any that contain maybe-impure function/method/static calls. Call it from MethodCallHandler and StaticCallHandler when processing calls with definite side effects (hasSideEffects()->yes()), skipping $this-> calls which already invalidate via invalidateExpression(). Closes phpstan/phpstan#13416
VincentLanglet
left a comment
There was a problem hiding this comment.
find is marked as impure
/**
* @return array<int, self>
* @phpstan-impure
*/
public static function find(): array {
return self::$storage;
}
So this fix is wrong ; the real issue is the fact that
count(MyRecord::find())
is a memoized expression even if it contains an impure call.
count($expr) can only be memorize if it does not contains impure $expr.
Maybe the same issue exist for other rememberedExpr ?
Instead of invalidating stored expression types after impure calls, prevent them from being stored in the first place. TypeSpecifier::createForExpr() now walks the expression tree and returns empty SpecifiedTypes when any sub-expression is a non-pure function/method/static call. This fixes cases like strlen(impure()) === 3 where the outer call (strlen) is pure but the inner argument is impure — the previous approach only checked the top-level expression. Closes phpstan/phpstan#13416 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's a summary of the changes: Root cause identified: The previous approach (post-hoc invalidation via Fix: Added Key behaviors:
Removed: |
The individual FuncCall, MethodCall, and StaticCall early returns in createForExpr() are fully covered by the expressionContainsNonPureCall() check that walks the entire expression tree. Remove the redundant top-level checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
This produce a regression on phpstan-beberlei-assert tests which does not report the second call
Assert::uuid($a);
Assert::uuid($a);
anymore ; any idea why ?
ac9419d to
eb6ad5a
Compare
Refactor expressionContainsNonPureCall to return TrinaryLogic instead of bool, separating the purity determination from the rememberPossiblyImpureFunctionValues policy decision. Extract callNodeHasSideEffects helper that returns the raw TrinaryLogic purity of each call node. Replace NodeFinder::findFirst (which requires a bool callback) with NodeFinder::find to collect all call nodes, then aggregate their purity via TrinaryLogic::or with early exit on yes. Unknown methods and dynamic call names return maybe (we genuinely don't know their purity). Unknown named functions keep returning yes for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| private function expressionContainsNonPureCall(Expr $expr, Scope $scope): bool | ||
| { | ||
| $nodeFinder = new NodeFinder(); | ||
| $found = $nodeFinder->findFirst([$expr], function (Node $node) use ($scope): bool { |
There was a problem hiding this comment.
do we have the same bug on impure ArrowFunctions and impure Closures?
if so, add tests and expand the callable to cover it
There was a problem hiding this comment.
Pushed. Here's a summary of the changes:
Investigation: The reviewer asked whether impure ArrowFunctions and Closures are handled by expressionContainsNonPureCall. The answer is yes, the current code handles them correctly through two mechanisms:
- NodeFinder descends into closure/arrow function bodies — impure FuncCall/MethodCall/StaticCall nodes inside closure bodies are found and flagged (e.g.,
(fn() => MyRecord::find())()) - The FuncCall dynamic callable path checks
ClosureType::isPure()viagetCallableParametersAcceptors(), which catches impurity from non-call sources likeecho
Tests added (7 new test cases):
- Arrow function IIFE with impure static call in body
- Closure IIFE with impure static call in body
strlenwrapping impure arrow function IIFE- Impure closure called via variable
- Closure IIFE with
echo+ impure calls in body - Pure closure IIFE stays narrowed (positive case)
- Pure function wrapping impure function with coalesce (covers #12734 pattern)
All 12071 tests pass and PHPStan self-analysis reports no errors.
eb6ad5a to
452a3c8
Compare
Verify that expressionContainsNonPureCall correctly handles: - Impure arrow function IIFEs - Impure closure IIFEs - strlen wrapping impure arrow function IIFEs - Impure closures called via variable - Closures with echo (non-call impurity) wrapping impure calls - Pure closure IIFEs staying narrowed (positive case) - Pure function wrapping impure function with coalesce Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 452a3c8.
…hecks The expressionContainsNonPureCall() method walks all sub-expressions looking for impure calls. Unknown functions (not found in the reflection provider) were treated as non-pure, which broke extensions using FAUX_FUNCTION_ markers in rootExpr (e.g. phpstan-beberlei-assert's isJsonString). The old code only checked the top-level expression, so unknown functions in sub-expressions were never flagged. Fix: return false for unknown functions in the tree walk (sub-expression check), but keep a separate top-level guard for unknown FuncCall expressions in createForExpr for backwards compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed successfully. Here's a summary of what I did: Root cause: The Fix (two changes):
Verified: All 12071 tests pass (79697 assertions), PHPStan self-analysis clean, original bug-13416 fix still works, and the beberlei-assert regression (string, integer, uuid, isJsonString double-call detection) is fixed. |
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression test for the fix made about FAUX_FUNCTION_ markers
Verifies that an unknown function (like FAUX_FUNCTION used by StrContainingTypeSpecifyingExtension and third-party extensions) appearing as a sub-expression does not prevent type narrowing in createForExpr. The test calls TypeSpecifier::create() with count(FAUX_FUNCTION($foo)) and asserts the expression is narrowed, which would fail if expressionContainsNonPureCall incorrectly treated unknown functions as non-pure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Added a non-regression test in |
Summary
assert(count(MyRecord::find()) === 1), calling an impure method like$msg2->insert()now correctly invalidates the narrowed count, socount(MyRecord::find())returnsint<0, max>instead of staying narrowed to1MutatingScope::invalidateAllMaybeImpureFunctionReturnValues()which removes stored expression types that contain maybe-impure function/method/static callsMethodCallHandlerandStaticCallHandlerfor calls withhasSideEffects()->yes(), skipping$this->calls to avoid over-invalidationDetails
The root cause was that
specifyTypesForCountFuncCall()narrows the argument expression (e.g.,MyRecord::find()->array{0: MyRecord}) and stores it in scope. Sincefind()hashasSideEffects()->maybe()andrememberPossiblyImpureFunctionValuesis enabled, the narrowing persists. When$msg2->insert()runs,invalidateExpression()only invalidates expressions containing$msg2, soMyRecord::find()was never cleared.The fix adds a new method that walks all stored expression types with
NodeFinder, checking each for function calls, method calls, or static calls that are not proven pure (hasSideEffects()->no()). Any expression containing such a call is removed from the scope.Test plan
tests/PHPStan/Analyser/nsrt/bug-13416.phpwith test cases covering:strlen()of impure call invalidated by method callCloses phpstan/phpstan#13416
Closes phpstan/phpstan#12734