From 931d570be2f0518e821701cf8f5569d46963ed62 Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:44:37 +0000 Subject: [PATCH] Narrow the falsey branch of `isset($a['x']['y'])` only when the intermediate offset is known to exist - `IssetHandler::specifyTypes()` no longer removes types from `$issetExpr->var` in the falsey branch when that expression itself may be undefined. Removing a type from `$a['x']` writes `hasOffsetValue('x', ...)` back into `$a` via `MutatingScope::specifyExpressionType()`, which wrongly asserted that the intermediate offset exists for the rest of the scope. - Added `IssetHandler::isAlwaysSet()` which walks the array-dim-fetch chain and reports whether every offset in it definitely exists. Undefined *variables* keep the previous behaviour because their certainty is separately degraded to "maybe" through `IssetExpr`. - When every possible value of the intermediate expression has the checked offset set, `isset()` can only be false because the intermediate offset itself is missing, so the enclosing array is now specified with that offset unset (`Type::unsetOffset()`). This turns `array{K?: array{Port: int}}` into `array{}` and `array` into `array|int<1, max>, array{Port: int}>` in the falsey branch instead of `*NEVER*`. The specification is skipped when it would collapse to `never` (list types cannot express a missing offset 0). - Same fix covers integer offsets, list offsets, property-fetch bases (`isset($this->arr['K']['Port'])`), deeper chains (`$r['A']['B']['Port']`) and multi-var `isset()` (rewritten to an and-chain), all of which leaked the same way. - Probed and found already correct: `empty()` (delegates to `!isset() || !expr`), `??`, `unset()`, non-constant offsets, `ArrayAccess` objects, and `array_key_exists()` (which evaluates its array argument, so the intermediate offset's existence really is implied there). --- src/Analyser/ExprHandler/IssetHandler.php | 94 +++++++++--- tests/PHPStan/Analyser/nsrt/bug-15005.php | 142 ++++++++++++++++++ .../Rules/Variables/NullCoalesceRuleTest.php | 5 + .../Rules/Variables/data/bug-15005.php | 25 +++ 4 files changed, 249 insertions(+), 17 deletions(-) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-15005.php create mode 100644 tests/PHPStan/Rules/Variables/data/bug-15005.php diff --git a/src/Analyser/ExprHandler/IssetHandler.php b/src/Analyser/ExprHandler/IssetHandler.php index da8e48431fd..c3ede980b49 100644 --- a/src/Analyser/ExprHandler/IssetHandler.php +++ b/src/Analyser/ExprHandler/IssetHandler.php @@ -39,6 +39,7 @@ use PHPStan\Type\Constant\ConstantStringType; use PHPStan\Type\IntersectionType; use PHPStan\Type\MixedType; +use PHPStan\Type\NeverType; use PHPStan\Type\NullType; use PHPStan\Type\ObjectType; use PHPStan\Type\ObjectWithoutClassType; @@ -212,25 +213,59 @@ public function specifyTypes(TypeSpecifier $typeSpecifier, Scope $scope, Expr $e if ($typesToRemove !== []) { $typeToRemove = TypeCombinator::union(...$typesToRemove); - $result = $typeSpecifier->create( - $issetExpr->var, - $typeToRemove, - TypeSpecifierContext::createFalse(), - $scope, - )->setRootExpr($expr); - - if ($scope->hasExpressionType($issetExpr->var)->maybe()) { - $result = $result->unionWith( - $typeSpecifier->create( - new IssetExpr($issetExpr->var), - new NullType(), - TypeSpecifierContext::createTruthy(), - $scope, - )->setRootExpr($expr), - ); + // isset() can also be false because $issetExpr->var itself does not exist. + // Narrowing its type is only sound when it's known to exist, or when its + // certainty can be degraded to "maybe" alongside the narrowing. + if ( + $issetExpr->var instanceof Expr\Variable + || $this->isAlwaysSet($scope, $issetExpr->var) + ) { + $result = $typeSpecifier->create( + $issetExpr->var, + $typeToRemove, + TypeSpecifierContext::createFalse(), + $scope, + )->setRootExpr($expr); + + if ($scope->hasExpressionType($issetExpr->var)->maybe()) { + $result = $result->unionWith( + $typeSpecifier->create( + new IssetExpr($issetExpr->var), + new NullType(), + TypeSpecifierContext::createTruthy(), + $scope, + )->setRootExpr($expr), + ); + } + + return $result; } - return $result; + // Every possible value of $issetExpr->var has the offset set, + // so the only way for isset() to be false is $issetExpr->var not existing. + if ( + TypeCombinator::remove($varType, $typeToRemove) instanceof NeverType + && $issetExpr->var instanceof ArrayDimFetch + && $issetExpr->var->dim !== null + && $this->isAlwaysSet($scope, $issetExpr->var->var) + ) { + $varDimTypes = $scope->getType($issetExpr->var->dim)->toArrayKey()->getConstantScalarTypes(); + if (count($varDimTypes) === 1) { + $varVarType = $scope->getType($issetExpr->var->var); + $withoutOffset = $varVarType->unsetOffset($varDimTypes[0]); + + // A list type cannot express "offset 0 is missing", the intersection + // would collapse to never and wrongly kill the whole branch. + if (!TypeCombinator::intersect($withoutOffset, $varVarType) instanceof NeverType) { + return $typeSpecifier->create( + $issetExpr->var->var, + $withoutOffset, + TypeSpecifierContext::createTruthy(), + $scope, + )->setRootExpr($expr); + } + } + } } } } @@ -342,6 +377,31 @@ public function specifyTypes(TypeSpecifier $typeSpecifier, Scope $scope, Expr $e return $types; } + /** + * Whether the expression is guaranteed to exist, so that narrowing it does not + * leak the existence of an intermediate offset into the enclosing array type. + */ + private function isAlwaysSet(Scope $scope, Expr $expr): bool + { + if ($expr instanceof Expr\Variable) { + return is_string($expr->name) && $scope->hasVariableType($expr->name)->yes(); + } + + if ($expr instanceof ArrayDimFetch) { + if ($expr->dim === null) { + return false; + } + + if (!$scope->getType($expr->var)->hasOffsetValueType($scope->getType($expr->dim))->yes()) { + return false; + } + + return $this->isAlwaysSet($scope, $expr->var); + } + + return true; + } + public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Expr $expr, MutatingScope $scope, ExpressionResultStorage $storage, callable $nodeCallback, ExpressionContext $context): ExpressionResult { $hasYield = false; diff --git a/tests/PHPStan/Analyser/nsrt/bug-15005.php b/tests/PHPStan/Analyser/nsrt/bug-15005.php new file mode 100644 index 00000000000..83a1b4f64d2 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-15005.php @@ -0,0 +1,142 @@ + $r */ +function nestedIssetLeak(array $r): void +{ + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); + + $port = isset($r['K']['Port']) ? $r['K']['Port'] : null; + + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); + + $secure = $r['K']['Secure'] ?? null; + + echo $port, $secure; +} + +/** @param array $r */ +function alsoAfterPlainIf(array $r): void +{ + if (isset($r['K']['Port'])) { + echo $r['K']['Port']; + } + + assertType('string|null', $r['K']['Secure'] ?? null); +} + +/** @param array $r */ +function branches(array $r): void +{ + if (isset($r['K']['Port'])) { + assertType('array{Port: int, Secure: string|null}', $r['K'] ?? null); + } else { + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); + } + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); +} + +/** @param array{K?: array{Port: int, Secure: string|null}} $r */ +function optionalIntermediateKey(array $r): void +{ + if (isset($r['K']['Port'])) { + assertType('array{K: array{Port: int, Secure: string|null}}', $r); + } else { + assertType('array{}', $r); + } + assertType('array{}|array{K: array{Port: int, Secure: string|null}}', $r); +} + +/** @param array> $r */ +function threeLevels(array $r): void +{ + if (isset($r['A']['B']['Port'])) { + assertType('array{Port: int}', $r['A']['B'] ?? null); + } else { + assertType('array|null', $r['A'] ?? null); + assertType('array>', $r); + } +} + +/** @param array{a: array{x: int}|array{y: int}} $r */ +function definitelySetIntermediate(array $r): void +{ + if (isset($r['a']['x'])) { + assertType('array{x: int}', $r['a']); + } else { + assertType('array{y: int}', $r['a']); + } +} + +/** @param array $r */ +function nestedEmptyLeak(array $r): void +{ + if (empty($r['K']['Port'])) { + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); + } + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); +} + +/** @param array $r */ +function nestedCoalesceLeak(array $r): void +{ + $port = $r['K']['Port'] ?? null; + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); + echo $port; +} + +/** @param array $r */ +function intOffsets(array $r): void +{ + if (isset($r[0]['Port'])) { + assertType('array{Port: int}', $r[0] ?? null); + } else { + assertType('array|int<1, max>, array{Port: int}>', $r); + assertType('array{Port: int}|null', $r[1] ?? null); + } + assertType('array{Port: int}|null', $r[0] ?? null); +} + +/** @param list $r */ +function listOffsets(array $r): void +{ + if (isset($r[0]['Port'])) { + assertType('array{Port: int}', $r[0] ?? null); + } else { + assertType('list', $r); + assertType('array{Port: int}|null', $r[0] ?? null); + } + assertType('array{Port: int}|null', $r[0] ?? null); +} + +final class Holder +{ + + /** @var array */ + public array $arr = []; + + public function doFoo(): void + { + if (isset($this->arr['K']['Port'])) { + assertType('array{Port: int, Secure: string|null}', $this->arr['K'] ?? null); + } else { + assertType('array{Port: int, Secure: string|null}|null', $this->arr['K'] ?? null); + } + assertType('array{Port: int, Secure: string|null}|null', $this->arr['K'] ?? null); + } + +} + +/** @param array $r */ +function multipleIssetVars(array $r): void +{ + if (isset($r['K']['Port'], $r['K']['Secure'])) { + assertType('array{Port: int, Secure: string}', $r['K']); + } else { + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); + } + assertType('array{Port: int, Secure: string|null}|null', $r['K'] ?? null); +} diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index fc6c577822d..c46678c5ee2 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -560,4 +560,9 @@ public function testBug14393(): void ]); } + public function testBug15005(): void + { + $this->analyse([__DIR__ . '/data/bug-15005.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-15005.php b/tests/PHPStan/Rules/Variables/data/bug-15005.php new file mode 100644 index 00000000000..bb7f4de6202 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-15005.php @@ -0,0 +1,25 @@ + $r */ +function nestedIssetLeak(array $r): void +{ + $port = isset($r['K']['Port']) ? $r['K']['Port'] : null; + + $secure = $r['K']['Secure'] ?? null; + + echo $port, $secure; +} + +/** @param array $r */ +function alsoAfterPlainIf(array $r): void +{ + if (isset($r['K']['Port'])) { + echo $r['K']['Port']; + } + + $secure = $r['K'] ?? null; + + echo count((array) $secure); +}