Skip to content

Walk each isset() subject narrowed by the subjects before it #7062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ondrejmirtes merged 1 commit into from
Nov 14, 2026
+117 −55
Merged

Walk each isset() subject narrowed by the subjects before it #7062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions src/Analyser/ExprHandler/IssetHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
use PHPStan\Type\Type;
use function array_map;
use function array_merge;
use function array_reverse;
use function count;

/**
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);

Expand Down
10 changes: 10 additions & 0 deletions tests/PHPStan/Rules/Arrays/InvalidKeyInArrayDimFetchRuleConfigPhpTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 [
Expand Down
21 changes: 21 additions & 0 deletions tests/PHPStan/Rules/Arrays/data/bug-15729.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

namespace Bug15729;

/** @param array<string, bool> $map */
function foo(?string $key, array $map): bool
{
return isset($key, $map[$key]);
}

/** @param array<string, bool> $map */
function keyCheckedAfterOffset(?string $key, array $map): bool
{
return isset($map[$key], $key);
}

/** @param array<string, array<string, bool>> $map */
function nested(?string $key, ?string $innerKey, array $map): bool
{
return isset($key, $innerKey, $map[$key][$innerKey]);
}
87 changes: 52 additions & 35 deletions turbo-ext/src/IssetHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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));
Expand Down
Loading