From 0c8b6b90228306553d3f4f18d11736ac9932ecc5 Mon Sep 17 00:00:00 2001 From: "ondrej@mirtes.cz" Date: Fri, 13 Nov 2026 23:10:34 +0000 Subject: [PATCH] Walk each isset() subject narrowed by the subjects before it isset() evaluates its arguments left to right and stops at the first one that is not set, so isset($key, $map[$key]) never reads $map[$key] with $key = null. IssetHandler walked every subject on the same scope, so rules saw the offset of the second subject as possibly null - with PHP 8.5 in the analysed version range, "Possibly invalid array key type string|null". Each subject after the first is now walked on the scope narrowed by the previous subject being set, like isset($a) && isset($b). The walk may stop after any subject, so the resulting scope merges the scopes after each subject. The turbo-ext mirror gets the same change. Amp-Thread-ID: https://ampcode.com/threads/T-01a1d722-733e-76bf-aded-6de7e1ddd980 --- src/Analyser/ExprHandler/IssetHandler.php | 54 +++++++----- ...lidKeyInArrayDimFetchRuleConfigPhpTest.php | 10 +++ tests/PHPStan/Rules/Arrays/data/bug-15729.php | 21 +++++ turbo-ext/src/IssetHandler.cpp | 87 +++++++++++-------- 4 files changed, 117 insertions(+), 55 deletions(-) create mode 100644 tests/PHPStan/Rules/Arrays/data/bug-15729.php diff --git a/src/Analyser/ExprHandler/IssetHandler.php b/src/Analyser/ExprHandler/IssetHandler.php index 0ad3a20cb62..e3c8cb19e9b 100644 --- a/src/Analyser/ExprHandler/IssetHandler.php +++ b/src/Analyser/ExprHandler/IssetHandler.php @@ -36,7 +36,6 @@ use PHPStan\Type\Type; use function array_map; use function array_merge; -use function array_reverse; use function count; /** @@ -68,11 +67,25 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $hasYield = false; $throwPoints = []; $impurePoints = []; - $nonNullabilityResults = []; $isAlwaysTerminating = false; $varResults = []; - foreach ($expr->vars as $var) { - $nonNullabilityResult = $this->nonNullabilityHelper->ensureNonNullability($scope, $var); + $chainResults = []; + $subjectScope = $scope; + $mergedScope = null; + foreach ($expr->vars as $i => $var) { + if ($i > 0) { + // isset() stops at the first unset subject, so each subject is + // walked only when the ones before it are set - like + // isset($a) && isset($b) + $subjectScope = $subjectScope->applySpecifiedTypes($this->defaultNarrowingHelper->createIssetTruthyChainTypes( + $subjectScope, + $expr->vars[$i - 1], + $this->defaultNarrowingHelper->buildChainTypeReader($chainResults, $subjectScope), + $expr, + TypeSpecifierContext::createTruthy(), + )); + } + $nonNullabilityResult = $this->nonNullabilityHelper->ensureNonNullability($subjectScope, $var); $scope = $nodeScopeResolver->lookForSetAllowedUndefinedExpressions($nonNullabilityResult->getScope(), $var); $varResult = $nodeScopeResolver->processExprNode($stmt, $var, $scope, $storage, $nodeCallback, $context->enterDeep()); $varResults[] = $varResult; @@ -81,7 +94,22 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $throwPoints = array_merge($throwPoints, $varResult->getThrowPoints()); $impurePoints = array_merge($impurePoints, $varResult->getImpurePoints()); $isAlwaysTerminating = $isAlwaysTerminating || $varResult->isAlwaysTerminating(); - $nonNullabilityResults[] = $nonNullabilityResult; + + // the next subject is walked without this subject's devices + $subjectScope = $this->nonNullabilityHelper->revertNonNullability( + $nodeScopeResolver->lookForUnsetAllowedUndefinedExpressions($scope, $var), + $nonNullabilityResult->getSpecifiedExpressions(), + ); + + // The subject and its chain links were just processed, so their + // ExpressionResults are in the storage; capture them (the results, not the + // storage - no reference cycle) so the narrowing reads their types via + // getTypeOnScope() instead of re-walking through Scope::getType(). + $this->defaultNarrowingHelper->captureChainResults($var, $storage, $chainResults); + + // the walk may stop after any subject: the narrowing the later + // subjects were walked with holds only when they were reached + $mergedScope = $mergedScope === null ? $subjectScope : $mergedScope->mergeWith($subjectScope); if (!($var instanceof ArrayDimFetch)) { continue; @@ -99,21 +127,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex new MethodCall(new TypeExpr($varType), 'offsetExists'), )); } - foreach (array_reverse($expr->vars) as $var) { - $scope = $nodeScopeResolver->lookForUnsetAllowedUndefinedExpressions($scope, $var); - } - foreach (array_reverse($nonNullabilityResults) as $nonNullabilityResult) { - $scope = $this->nonNullabilityHelper->revertNonNullability($scope, $nonNullabilityResult->getSpecifiedExpressions()); - } - - // The subjects and their chain links were just processed, so their - // ExpressionResults are in the storage; capture them (the results, not the - // storage - no reference cycle) so the narrowing reads their types via - // getTypeOnScope() instead of re-walking through Scope::getType(). - $chainResults = []; - foreach ($expr->vars as $var) { - $this->defaultNarrowingHelper->captureChainResults($var, $storage, $chainResults); - } + $scope = $mergedScope ?? $scope; $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new IssetExpressionNode($expr, $varResults), $beforeScope, $storage, $context); diff --git a/tests/PHPStan/Rules/Arrays/InvalidKeyInArrayDimFetchRuleConfigPhpTest.php b/tests/PHPStan/Rules/Arrays/InvalidKeyInArrayDimFetchRuleConfigPhpTest.php index 24a646acfb9..6525cfc8ccc 100644 --- a/tests/PHPStan/Rules/Arrays/InvalidKeyInArrayDimFetchRuleConfigPhpTest.php +++ b/tests/PHPStan/Rules/Arrays/InvalidKeyInArrayDimFetchRuleConfigPhpTest.php @@ -30,6 +30,16 @@ public function testRuleInPhpVersionRange(): void ]); } + public function testBug15729(): void + { + $this->analyse([__DIR__ . '/data/bug-15729.php'], [ + [ + 'Possibly invalid array key type string|null.', + 14, + ], + ]); + } + public static function getAdditionalConfigFiles(): array { return [ diff --git a/tests/PHPStan/Rules/Arrays/data/bug-15729.php b/tests/PHPStan/Rules/Arrays/data/bug-15729.php new file mode 100644 index 00000000000..341c1ef6a72 --- /dev/null +++ b/tests/PHPStan/Rules/Arrays/data/bug-15729.php @@ -0,0 +1,21 @@ + $map */ +function foo(?string $key, array $map): bool +{ + return isset($key, $map[$key]); +} + +/** @param array $map */ +function keyCheckedAfterOffset(?string $key, array $map): bool +{ + return isset($map[$key], $key); +} + +/** @param array> $map */ +function nested(?string $key, ?string $innerKey, array $map): bool +{ + return isset($key, $innerKey, $map[$key][$innerKey]); +} diff --git a/turbo-ext/src/IssetHandler.cpp b/turbo-ext/src/IssetHandler.cpp index 5083acee440..86c1a9b6b82 100644 --- a/turbo-ext/src/IssetHandler.cpp +++ b/turbo-ext/src/IssetHandler.cpp @@ -204,15 +204,35 @@ class IssetHandler // foreach iterates the array it was handed zv::Val vars = zv::Val::copyOf(zv::Ref(varsSlot)); uint32_t varCount = zend_hash_num_elements(Z_ARRVAL_P(vars.raw())); - zv::Arr nonNullabilityResults = zv::Arr::create(varCount); zv::Arr varResults = zv::Arr::create(varCount); + zv::Val chainResults = zv::Val(zv::Arr::empty()); + zval *defaultNarrowingHelper = OBJ_PROP_NUM(self, slots::defaultNarrowingHelper); + zv::Val subjectScope = zv::Val::copyOf(zv::Ref(scope.raw())); + // UNDEF = null + zv::Val mergedScope; zend_class_entry *arrayDimFetchCe = pt_class(PT_CLASS_ARRAY_DIM_FETCH); if (UNEXPECTED(arrayDimFetchCe == NULL)) return zv::Val(); zv::Val hold; + zval *previousVar = NULL; for (zv::ArrayEntry entry : zv::TableRef(Z_ARRVAL_P(vars.raw()))) { zval *var = entry.value().deref().raw(); - zv::Val nonNullabilityResult = pt_non_nullability_helper_ensure_non_nullability(nonNullabilityHelper, scope.raw(), var); + if (previousVar != NULL) { + // isset() stops at the first unset subject, so each subject is + // walked only when the ones before it are set - like + // isset($a) && isset($b) + zv::Val readType = pt_default_narrowing_helper_build_chain_type_reader(defaultNarrowingHelper, chainResults.raw(), subjectScope.raw()); + if (UNEXPECTED(readType.isUndef())) return zv::Val(); + zend_object *truthy = pt_type_specifier_context_create_truthy(); + if (UNEXPECTED(truthy == NULL)) return zv::Val(); + zval truthyZval; + ZVAL_OBJ(&truthyZval, truthy); + zv::Val next = applySpecifiedTypes(subjectScope.raw(), pt_default_narrowing_helper_create_isset_truthy_chain_types(defaultNarrowingHelper, subjectScope.raw(), previousVar, readType.raw(), expr, &truthyZval)); + if (UNEXPECTED(next.isUndef())) return zv::Val(); + subjectScope = std::move(next); + } + previousVar = var; + zv::Val nonNullabilityResult = pt_non_nullability_helper_ensure_non_nullability(nonNullabilityHelper, subjectScope.raw(), var); if (UNEXPECTED(nonNullabilityResult.isUndef())) return zv::Val(); if (UNEXPECTED(!nonNullabilityResult.ref().isObject())) { zend_throw_error(NULL, "Call to a member function getScope() on %s", zend_zval_value_name(nonNullabilityResult.raw())); @@ -244,7 +264,34 @@ class IssetHandler if (UNEXPECTED(borrowed == NULL)) return zv::Val(); impurePoints = arrayMerge(impurePoints.raw(), borrowed); if (!isAlwaysTerminating && UNEXPECTED(!pt_expression_result_is_always_terminating(varResult.raw(), isAlwaysTerminating))) return zv::Val(); - nonNullabilityResults.push(std::move(nonNullabilityResult)); + + // the next subject is walked without this subject's devices + { + zv::Val unset = pt_node_scope_resolver_look_for_unset_allowed_undefined_expressions(nodeScopeResolver, scope.raw(), var); + if (UNEXPECTED(unset.isUndef())) return zv::Val(); + zval *specifiedExpressions = pt_ensured_non_nullability_result_specified_expressions(nonNullabilityResult.raw(), hold); + if (UNEXPECTED(specifiedExpressions == NULL)) return zv::Val(); + zv::Val heldExpressions = zv::Val::copyOf(zv::Ref(specifiedExpressions)); + subjectScope = pt_non_nullability_helper_revert_non_nullability(nonNullabilityHelper, unset.raw(), heldExpressions.raw()); + if (UNEXPECTED(subjectScope.isUndef())) return zv::Val(); + } + + // capture the subject's and its chain links' stored results + if (UNEXPECTED(!captureChainResults(defaultNarrowingHelper, var, storage, chainResults))) return zv::Val(); + + // the walk may stop after any subject: the narrowing the later + // subjects were walked with holds only when they were reached + if (mergedScope.isUndef()) { + mergedScope = zv::Val::copyOf(zv::Ref(subjectScope.raw())); + } else { + if (UNEXPECTED(!mergedScope.ref().isObject())) { + zend_throw_error(NULL, "Call to a member function mergeWith() on %s", zend_zval_value_name(mergedScope.raw())); + return zv::Val(); + } + zv::Val merged = pt_mutating_scope_merge_with(Z_OBJ_P(mergedScope.raw()), subjectScope.raw()); + if (UNEXPECTED(merged.isUndef())) return zv::Val(); + mergedScope = std::move(merged); + } if (Z_TYPE_P(var) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(var), arrayDimFetchCe)) continue; @@ -296,38 +343,8 @@ class IssetHandler throwPoints = arrayMerge(throwPoints.raw(), callThrowPoints.raw()); } - // foreach (array_reverse($expr->vars) as $var) - { - HashTable *table = Z_ARRVAL_P(vars.raw()); - zval *var; - ZEND_HASH_REVERSE_FOREACH_VAL(table, var) { - ZVAL_DEREF(var); - zv::Val next = pt_node_scope_resolver_look_for_unset_allowed_undefined_expressions(nodeScopeResolver, scope.raw(), var); - if (UNEXPECTED(next.isUndef())) return zv::Val(); - scope = std::move(next); - } ZEND_HASH_FOREACH_END(); - } - // foreach (array_reverse($nonNullabilityResults) as $nonNullabilityResult) - { - HashTable *table = nonNullabilityResults.table(); - zval *nonNullabilityResult; - ZEND_HASH_REVERSE_FOREACH_VAL(table, nonNullabilityResult) { - zval *specifiedExpressions = pt_ensured_non_nullability_result_specified_expressions(nonNullabilityResult, hold); - if (UNEXPECTED(specifiedExpressions == NULL)) return zv::Val(); - zv::Val heldExpressions = zv::Val::copyOf(zv::Ref(specifiedExpressions)); - zv::Val next = pt_non_nullability_helper_revert_non_nullability(nonNullabilityHelper, scope.raw(), heldExpressions.raw()); - if (UNEXPECTED(next.isUndef())) return zv::Val(); - scope = std::move(next); - } ZEND_HASH_FOREACH_END(); - } - - // capture the subjects' and their chain links' stored results - zv::Val chainResults = zv::Val(zv::Arr::empty()); - { - zval *defaultNarrowingHelper = OBJ_PROP_NUM(self, slots::defaultNarrowingHelper); - for (zv::ArrayEntry entry : zv::TableRef(Z_ARRVAL_P(vars.raw()))) { - if (UNEXPECTED(!captureChainResults(defaultNarrowingHelper, entry.value().deref().raw(), storage, chainResults))) return zv::Val(); - } + if (!mergedScope.isUndef()) { + scope = std::move(mergedScope); } zv::Val varResultsValue(std::move(varResults));