Skip to content

Commit dd39ca7

Browse files
ondrejmirtesclaude
ondrejmirtes
and
claude
committed
Project narrowings through stored booleans for more expression kinds
Extends the conditional-expression machinery used for assignments like `$ok = $x->foo() !== null` so the narrowing survives through a later `if ($ok) { … }`. Previously only Variable/PropertyFetch/ArrayDimFetch/FuncCall sure-type targets were projected, so method calls (and static/nullsafe calls) were dropped entirely. For call-typed sure-type targets (FuncCall, MethodCall, NullsafeMethodCall, StaticCall) we only project when the sure-type expression is a sub-expression of the assigned RHS — not the RHS itself. That keeps the narrowing of `$x->foo()` in `$ok = $x->foo() !== null` while dropping the falsey-scalar loop's `$this->nullable() === null` projections that would wrongly survive across subsequent reassignments. PHPStan virtual nodes (e.g. NativeTypeExpr), scalars and const-fetches are rejected to avoid numeric-string array-key autocast and internal nodes leaking into the conditional-expression map. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d78d9c0 commit dd39ca7

3 files changed

Lines changed: 178 additions & 40 deletions

File tree

‎src/Analyser/ExprHandler/AssignHandler.php‎

Lines changed: 91 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313
use PhpParser\Node\Expr\FuncCall;
1414
use PhpParser\Node\Expr\List_;
1515
use PhpParser\Node\Expr\MethodCall;
16+
use PhpParser\Node\Expr\NullsafeMethodCall;
1617
use PhpParser\Node\Expr\PropertyFetch;
18+
use PhpParser\Node\Expr\StaticCall;
1719
use PhpParser\Node\Expr\StaticPropertyFetch;
1820
use PhpParser\Node\Expr\Ternary;
1921
use PhpParser\Node\Expr\Variable;
@@ -44,6 +46,7 @@
4446
use PHPStan\Node\Expr\TypeExpr;
4547
use PHPStan\Node\PropertyAssignNode;
4648
use PHPStan\Node\VariableAssignNode;
49+
use PHPStan\Node\VirtualNode;
4750
use PHPStan\Php\PhpVersion;
4851
use PHPStan\ShouldNotHappenException;
4952
use PHPStan\TrinaryLogic;
@@ -259,23 +262,23 @@ public function processAssignVar(
259262
$truthyType->isSuperTypeOf($falseyType)->no()
260263
&& $falseyType->isSuperTypeOf($truthyType)->no()
261264
) {
262-
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType);
263-
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType);
264-
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);
265-
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);
265+
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $impurePoints, $assignedExpr);
266+
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $impurePoints, $assignedExpr);
267+
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $impurePoints, $assignedExpr);
268+
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $impurePoints, $assignedExpr);
266269
}
267270
}
268271

269272
$truthyType = TypeCombinator::removeFalsey($type);
270273
if ($truthyType !== $type) {
271274
$truthySpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $assignedExpr, TypeSpecifierContext::createTruthy());
272-
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType);
273-
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType);
275+
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $impurePoints, $assignedExpr);
276+
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $impurePoints, $assignedExpr);
274277

275278
$falseyType = TypeCombinator::intersect($type, StaticTypeFactory::falsey());
276279
$falseySpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $assignedExpr, TypeSpecifierContext::createFalsey());
277-
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);
278-
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);
280+
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $impurePoints, $assignedExpr);
281+
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $impurePoints, $assignedExpr);
279282
}
280283

281284
foreach ([null, false, 0, 0.0, '', '0', []] as $falseyScalar) {
@@ -304,13 +307,13 @@ public function processAssignVar(
304307

305308
$notIdenticalConditionExpr = new Expr\BinaryOp\NotIdentical($assignedExpr, $astNode);
306309
$notIdenticalSpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $notIdenticalConditionExpr, TypeSpecifierContext::createTrue());
307-
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType);
308-
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType);
310+
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType, $impurePoints, $assignedExpr);
311+
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType, $impurePoints, $assignedExpr);
309312

310313
$identicalConditionExpr = new Expr\BinaryOp\Identical($assignedExpr, $astNode);
311314
$identicalSpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $identicalConditionExpr, TypeSpecifierContext::createTrue());
312-
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType);
313-
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType);
315+
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType, $impurePoints, $assignedExpr);
316+
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType, $impurePoints, $assignedExpr);
314317
}
315318

316319
$nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval, $storage);
@@ -848,24 +851,13 @@ private function unwrapAssign(Expr $expr): Expr
848851

849852
/**
850853
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
854+
* @param ImpurePoint[] $rhsImpurePoints
851855
* @return array<string, ConditionalExpressionHolder[]>
852856
*/
853-
private function processSureTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType): array
857+
private function processSureTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType, array $rhsImpurePoints, Expr $assignedExpr): array
854858
{
855859
foreach ($specifiedTypes->getSureTypes() as $exprString => [$expr, $exprType]) {
856-
if ($expr instanceof Variable) {
857-
if (!is_string($expr->name)) {
858-
continue;
859-
}
860-
861-
if ($expr->name === $variableName) {
862-
continue;
863-
}
864-
} elseif (
865-
!$expr instanceof PropertyFetch
866-
&& !$expr instanceof ArrayDimFetch
867-
&& !$expr instanceof FuncCall
868-
) {
860+
if (!$this->isExprSafeToProjectThroughVariable($expr, $variableName, $rhsImpurePoints, $assignedExpr)) {
869861
continue;
870862
}
871863

@@ -887,24 +879,13 @@ private function processSureTypesForConditionalExpressionsAfterAssign(Scope $sco
887879

888880
/**
889881
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
882+
* @param ImpurePoint[] $rhsImpurePoints
890883
* @return array<string, ConditionalExpressionHolder[]>
891884
*/
892-
private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType): array
885+
private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType, array $rhsImpurePoints, Expr $assignedExpr): array
893886
{
894887
foreach ($specifiedTypes->getSureNotTypes() as $exprString => [$expr, $exprType]) {
895-
if ($expr instanceof Variable) {
896-
if (!is_string($expr->name)) {
897-
continue;
898-
}
899-
900-
if ($expr->name === $variableName) {
901-
continue;
902-
}
903-
} elseif (
904-
!$expr instanceof PropertyFetch
905-
&& !$expr instanceof ArrayDimFetch
906-
&& !$expr instanceof FuncCall
907-
) {
888+
if (!$this->isExprSafeToProjectThroughVariable($expr, $variableName, $rhsImpurePoints, $assignedExpr)) {
908889
continue;
909890
}
910891

@@ -924,6 +905,76 @@ private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $
924905
return $conditionalExpressions;
925906
}
926907

908+
/**
909+
* We're about to remember "when $variableName is truthy/falsy, $expr has a narrower type".
910+
* Whether that's safe to project forward depends on whether re-evaluating $expr later will
911+
* still return the same value as when we observed the narrowing — i.e. whether $expr is
912+
* referentially transparent with respect to the intervening code.
913+
*
914+
* Scalar/const-fetch literals are never narrowing targets, so skip them up front (they also
915+
* happen to stringify to numeric exprStrings which collide with PHP's numeric-string
916+
* array-key autocast).
917+
*
918+
* A plain variable is always safe: reading it doesn't produce side effects, and if it gets
919+
* reassigned the existing conditional-expression-holder machinery invalidates the binding.
920+
* This case matters for e.g. `$ok = preg_match(..., $matches); if ($ok) { use $matches }` —
921+
* `preg_match` itself has impure points, but `$matches` is a plain variable and the
922+
* narrowing attached to it should still survive.
923+
*
924+
* Other common tracked expressions (property/dim fetches, function/method calls) can always
925+
* carry narrowings: PHPStan already memoises their types per exprString, and condition
926+
* checks like `$x = $obj->foo() !== null; if ($x) { $obj->foo(); }` rely on this even when
927+
* the RHS itself has impure points (as a method call without @phpstan-pure always does).
928+
*
929+
* Anything else is accepted only when the right-hand side evaluation recorded zero impure
930+
* points — in that case all sub-expressions it produced sure types for were evaluated
931+
* without side effects and can be re-evaluated later with the same result.
932+
*
933+
* @param ImpurePoint[] $rhsImpurePoints
934+
*/
935+
private function isExprSafeToProjectThroughVariable(Expr $expr, string $variableName, array $rhsImpurePoints, Expr $assignedExpr): bool
936+
{
937+
// Scalar/const-fetch literals and PHPStan virtual nodes (e.g. NativeTypeExpr) are never
938+
// narrowing targets at a usage site — skip them so they don't collide with PHP's
939+
// numeric-string array-key autocast or leak internal virtual expressions into the
940+
// conditional-expression map.
941+
if ($expr instanceof Node\Scalar || $expr instanceof ConstFetch || $expr instanceof VirtualNode) {
942+
return false;
943+
}
944+
945+
if ($expr instanceof Variable) {
946+
return is_string($expr->name) && $expr->name !== $variableName;
947+
}
948+
949+
if (
950+
$expr instanceof PropertyFetch
951+
|| $expr instanceof ArrayDimFetch
952+
) {
953+
return true;
954+
}
955+
956+
if (
957+
$expr instanceof FuncCall
958+
|| $expr instanceof MethodCall
959+
|| $expr instanceof NullsafeMethodCall
960+
|| $expr instanceof StaticCall
961+
) {
962+
// A call's type can change between evaluations. We're willing to project the
963+
// narrowing through a stored boolean only when the sure-type expression is a
964+
// *sub*-expression of the assigned RHS — e.g. `$ok = $x->foo() !== null` builds
965+
// a sure type for the sub-call `$x->foo()`. In that case the RHS as a whole
966+
// carries the comparison result, and later `if ($ok)` usefully re-narrows the
967+
// remembered sub-call. When the sure-type expression IS the whole RHS (e.g.
968+
// `$device = $this->nullable(); if ($device === null) { … }` with the
969+
// falsey-scalar loop producing `$this->nullable() === null` narrowings), the
970+
// projection would survive across subsequent reassignments of the target
971+
// expression and wrongly re-narrow fresh calls — so skip it.
972+
return $expr !== $assignedExpr;
973+
}
974+
975+
return count($rhsImpurePoints) === 0;
976+
}
977+
927978
/**
928979
* @param list<ArrayDimFetch> $dimFetchStack
929980
*/

‎tests/PHPStan/Analyser/nsrt/conditional-expr-narrowing-through-variable.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
3+
namespace ConditionalExprNarrowingThroughVariable;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
class Holder
8+
{
9+
10+
/**
11+
* @return array{int, string}|null
12+
* @phpstan-pure
13+
*/
14+
public function getPair(): ?array
15+
{
16+
throw new \Exception();
17+
}
18+
19+
}
20+
21+
function pureMethodCallNarrowsThroughVariable(Holder $a, Holder $b): void
22+
{
23+
$bothReady = $a->getPair() !== null && $b->getPair() !== null;
24+
25+
if ($bothReady) {
26+
$aPair = $a->getPair();
27+
$bPair = $b->getPair();
28+
29+
assertType('array{int, string}', $aPair);
30+
assertType('array{int, string}', $bPair);
31+
32+
assertType('int', $aPair[0]);
33+
assertType('int', $bPair[0]);
34+
}
35+
}
36+
37+
function pregMatchNarrowsByRefVariable(string $in): void
38+
{
39+
$matches = [];
40+
$result = preg_match('~^/xxx/([\w\-]+)/?([\w\-]+)?/?$~', $in, $matches);
41+
if ($result) {
42+
// preg_match has impure points (it writes $matches by ref), but $matches
43+
// is a plain variable so the narrowing attached to it must still survive
44+
// through the stored `$result` guard.
45+
assertType('array{0: non-falsy-string, 1: non-empty-string, 2?: non-empty-string}', $matches);
46+
}
47+
}

‎tests/PHPStan/Analyser/nsrt/try-catch-reassign-method-narrowing.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
3+
namespace TryCatchReassignMethodNarrowing;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
class Foo
8+
{
9+
10+
public function doFoo(): void
11+
{
12+
$device = $this->nullable();
13+
if ($device === null) {
14+
$device = 1;
15+
try {
16+
$device = $this->throwsException();
17+
} catch (\Exception) {
18+
$device = $this->nullable();
19+
// After reassignment to a fresh `$this->nullable()` call inside the catch,
20+
// the variable's type must be the method's declared return type. Earlier
21+
// conditional-expression holders stored at the initial `$device = $this->nullable()`
22+
// (narrowing `$this->nullable()` to `null` when `$device` is null) must not leak
23+
// through to this re-evaluation: each call may return a different value.
24+
assertType('int|null', $device);
25+
}
26+
}
27+
}
28+
29+
public function nullable(): ?int
30+
{
31+
throw new \Exception();
32+
}
33+
34+
/** @throws \Exception */
35+
private function throwsException(): int
36+
{
37+
throw new \Exception();
38+
}
39+
40+
}

0 commit comments

Comments
 (0)