Skip to content

Commit 63d438b

Browse files
ondrejmirtesclaude
ondrejmirtes
and
claude
committed
Invalidate the foreach value alias on writes that bypass the assignment path
The value-variable alias projected onto $array[$key] relied on every write to the value, key or iteratee variable severing the link through the assignment-time containment invalidation. Several paths bypass that walk: a by-ref closure use (processClosureScope), extract() and a dynamic $$name write update variable holders directly, so drop the aliases there. And because foreach iterates a snapshot, a cross-iteration write into the iteratee desyncs the live element from the snapshot value variable - do not record the alias when the loop body mutates the iteratee at a foreign key, reassigns the iteratee, or lets it escape by reference; a same-key write ($array[$key] = ...) is still kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
1 parent 484a1cb commit 63d438b

5 files changed

Lines changed: 391 additions & 19 deletions

File tree

‎src/Analyser/ExprHandler/AssignHandler.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1225,6 +1225,13 @@ public function applyWrite(
12251225
$isAlwaysTerminating = $isAlwaysTerminating || $nameExprResult->isAlwaysTerminating();
12261226
$scope = $nameExprResult->getScope();
12271227
}
1228+
1229+
if (!is_string($var->name)) {
1230+
// a dynamic $$name write can target any variable, including the
1231+
// foreach value/key/iteratee - drop the value aliases so a later
1232+
// narrowing is not projected onto a dim fetch it may have desynced
1233+
$scope = $scope->invalidateForeachValueAliases();
1234+
}
12281235
} elseif ($kind === PreparedAssignTarget::KIND_ARRAY_DIM_FETCH) {
12291236
if (!$var instanceof ArrayDimFetch) {
12301237
throw new ShouldNotHappenException();

‎src/Analyser/MutatingScope.php‎

Lines changed: 94 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -495,23 +495,28 @@ public function canAnyVariableExist(): bool
495495

496496
public function afterExtractCall(): self
497497
{
498-
return $this->scopeFactory->create(
499-
$this->context,
500-
$this->isDeclareStrictTypes(),
501-
$this->getFunction(),
502-
$this->getNamespace(),
503-
$this->expressionTypes,
504-
$this->nativeExpressionTypes,
498+
// extract() may (re)define any variable, including the foreach value/key/
499+
// iteratee - drop the value aliases so a later narrowing is not projected
500+
// onto a dim fetch the extracted values may have desynced.
501+
$scope = $this->invalidateForeachValueAliases();
502+
503+
return $scope->scopeFactory->create(
504+
$scope->context,
505+
$scope->isDeclareStrictTypes(),
506+
$scope->getFunction(),
507+
$scope->getNamespace(),
508+
$scope->expressionTypes,
509+
$scope->nativeExpressionTypes,
505510
[],
506-
$this->inClosureBindScopeClasses,
507-
$this->anonymousFunctionReflection,
508-
$this->isInFirstLevelStatement(),
509-
$this->currentlyAssignedExpressions,
510-
$this->currentlyAllowedUndefinedExpressions,
511-
$this->inFunctionCallsStack,
511+
$scope->inClosureBindScopeClasses,
512+
$scope->anonymousFunctionReflection,
513+
$scope->isInFirstLevelStatement(),
514+
$scope->currentlyAssignedExpressions,
515+
$scope->currentlyAllowedUndefinedExpressions,
516+
$scope->inFunctionCallsStack,
512517
true,
513-
$this->parentScope,
514-
$this->nativeTypesPromoted,
518+
$scope->parentScope,
519+
$scope->nativeTypesPromoted,
515520
);
516521
}
517522

@@ -2760,7 +2765,7 @@ public function enterMatch(Expr\Match_ $expr, Type $condType, Type $condNativeTy
27602765
return $this->assignExpression($condExpr, $type, $nativeType);
27612766
}
27622767

2763-
public function enterForeach(self $originalScope, Expr $iteratee, Type $iterateeType, Type $nativeIterateeType, string $valueName, ?string $keyName, bool $valueByRef): self
2768+
public function enterForeach(self $originalScope, Expr $iteratee, Type $iterateeType, Type $nativeIterateeType, string $valueName, ?string $keyName, bool $valueByRef, bool $recordValueAlias = true): self
27642769
{
27652770
$valueType = $originalScope->getIterableValueType($iterateeType);
27662771
$nativeValueType = $originalScope->getIterableValueType($nativeIterateeType);
@@ -2796,12 +2801,16 @@ public function enterForeach(self $originalScope, Expr $iteratee, Type $iteratee
27962801
if ($keyName !== null) {
27972802
$scope = $scope->enterForeachKey($originalScope, $iteratee, $iterateeType, $nativeIterateeType, $keyName);
27982803

2799-
if ($iterateeType->isArray()->yes()) {
2804+
if ($recordValueAlias && $iterateeType->isArray()->yes()) {
28002805
// for the current iteration the value variable and the iteratee dim
28012806
// fetch alias one runtime value - narrowings landed on the value
28022807
// variable are projected onto the tracked dim fetch through this
28032808
// link (applySpecifiedTypes()); a write to any of the three
2804-
// participating expressions invalidates it through containment
2809+
// participating expressions invalidates it through containment.
2810+
// The alias is only recorded when the loop body does not mutate the
2811+
// iteratee at a foreign key or reassign the key ($recordValueAlias,
2812+
// decided in ForeachHandler): a cross-iteration write would desync
2813+
// $array[$key] from the snapshot value variable.
28052814
$scope = $scope->assignExpression(
28062815
new ForeachValueAliasExpr($valueName, new Expr\ArrayDimFetch($iteratee, new Variable($keyName))),
28072816
$valueType,
@@ -3591,6 +3600,57 @@ public function invalidateExpression(Expr $expressionToInvalidate, bool $require
35913600
);
35923601
}
35933602

3603+
/**
3604+
* Drops every foreach value-variable alias (ForeachValueAliasExpr). The alias
3605+
* is a precision optimization whose soundness depends on the value variable
3606+
* still holding the iteratee element - code paths that write a variable
3607+
* without going through the assignment-time containment invalidation (a by-ref
3608+
* closure use, extract(), a dynamic $$name write) must sever it here, because
3609+
* they may have desynced the value variable from $array[$key]. Losing the alias
3610+
* conservatively is always sound; the element type falls back to the iterable
3611+
* value type.
3612+
*/
3613+
public function invalidateForeachValueAliases(): self
3614+
{
3615+
$changed = false;
3616+
$expressionTypes = $this->expressionTypes;
3617+
$nativeExpressionTypes = $this->nativeExpressionTypes;
3618+
foreach ([$this->expressionTypes, $this->nativeExpressionTypes] as $types) {
3619+
foreach (array_keys($types) as $exprString) {
3620+
if (!str_starts_with($exprString, ForeachValueAliasExpr::KEY_PREFIX)) {
3621+
continue;
3622+
}
3623+
3624+
unset($expressionTypes[$exprString]);
3625+
unset($nativeExpressionTypes[$exprString]);
3626+
$changed = true;
3627+
}
3628+
}
3629+
3630+
if (!$changed) {
3631+
return $this;
3632+
}
3633+
3634+
return $this->scopeFactory->create(
3635+
$this->context,
3636+
$this->isDeclareStrictTypes(),
3637+
$this->getFunction(),
3638+
$this->getNamespace(),
3639+
$expressionTypes,
3640+
$nativeExpressionTypes,
3641+
$this->conditionalExpressions,
3642+
$this->inClosureBindScopeClasses,
3643+
$this->anonymousFunctionReflection,
3644+
$this->isInFirstLevelStatement(),
3645+
$this->currentlyAssignedExpressions,
3646+
$this->currentlyAllowedUndefinedExpressions,
3647+
$this->inFunctionCallsStack,
3648+
$this->afterExtractCall,
3649+
$this->parentScope,
3650+
$this->nativeTypesPromoted,
3651+
);
3652+
}
3653+
35943654
/** @internal called by ScopeOps */
35953655
public function isPrivatePropertyOfDifferentClass(Expr $expr, ClassReflection $invalidatingClass): bool
35963656
{
@@ -4495,6 +4555,22 @@ public function processClosureScope(
44954555
$nativeExpressionTypes[$variableExprString] = $holder;
44964556
}
44974557

4558+
// The by-ref uses above write variable holders directly, bypassing the
4559+
// assignment-time containment invalidation. A by-ref use may write the
4560+
// foreach value/key/iteratee variable, desyncing the value variable from
4561+
// $array[$key] - drop the value aliases so a later narrowing is not
4562+
// projected onto the tracked dim fetch.
4563+
foreach ([$expressionTypes, $nativeExpressionTypes] as $types) {
4564+
foreach (array_keys($types) as $exprString) {
4565+
if (!str_starts_with($exprString, ForeachValueAliasExpr::KEY_PREFIX)) {
4566+
continue;
4567+
}
4568+
4569+
unset($expressionTypes[$exprString]);
4570+
unset($nativeExpressionTypes[$exprString]);
4571+
}
4572+
}
4573+
44984574
return $this->scopeFactory->create(
44994575
$this->context,
45004576
$this->isDeclareStrictTypes(),

‎src/Analyser/StmtHandler/ForeachHandler.php‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use PhpParser\Node\Stmt\Break_;
2020
use PhpParser\Node\Stmt\Continue_;
2121
use PhpParser\Node\Stmt\Foreach_;
22+
use PhpParser\NodeFinder;
2223
use PHPStan\Analyser\ConditionalExpressionHolder;
2324
use PHPStan\Analyser\ExpressionContext;
2425
use PHPStan\Analyser\ExpressionResultStorage;
@@ -73,6 +74,7 @@ final class ForeachHandler implements StmtHandler
7374

7475
private const FOREACH_UNROLL_LIMIT = 16;
7576
private const FOREACH_UNROLL_NESTED_LIMIT = 8;
77+
private const RECORD_VALUE_ALIAS_ATTRIBUTE = 'foreachValueAliasRecordable';
7678

7779
public function __construct(
7880
private Container $container,
@@ -500,6 +502,7 @@ private function enterForeach(NodeScopeResolver $nodeScopeResolver, MutatingScop
500502
$stmt->valueVar->name,
501503
$keyVarName,
502504
$stmt->byRef,
505+
$this->shouldRecordForeachValueAlias($stmt),
503506
);
504507
$vars = [$stmt->valueVar->name];
505508
if ($keyVarName !== null) {
@@ -605,6 +608,176 @@ private function enterForeach(NodeScopeResolver $nodeScopeResolver, MutatingScop
605608
return $this->varAnnotationProcessor->processVarAnnotation($scope, $vars, $stmt);
606609
}
607610

611+
/**
612+
* Whether the foreach value variable may alias the iteratee dim fetch
613+
* ($array[$key]) for its whole iteration. `foreach` iterates a snapshot, so
614+
* the alias is unsound once the body mutates the iteratee at a foreign key,
615+
* reassigns the iteratee, or lets it escape by reference: a later iteration
616+
* would read a live element the snapshot value variable no longer matches.
617+
* A same-key write ($array[$key] = ...) is kept - it only touches the current
618+
* iteration's element, invalidated in-body through containment. The verdict is
619+
* static, so it is cached on the loop node across convergence passes.
620+
*/
621+
private function shouldRecordForeachValueAlias(Foreach_ $stmt): bool
622+
{
623+
$cached = $stmt->getAttribute(self::RECORD_VALUE_ALIAS_ATTRIBUTE);
624+
if ($cached !== null) {
625+
return $cached;
626+
}
627+
628+
$result = $this->computeShouldRecordForeachValueAlias($stmt);
629+
$stmt->setAttribute(self::RECORD_VALUE_ALIAS_ATTRIBUTE, $result);
630+
631+
return $result;
632+
}
633+
634+
private function computeShouldRecordForeachValueAlias(Foreach_ $stmt): bool
635+
{
636+
// the value alias exists only for `foreach ($var as $key => $value)` over a
637+
// plain-variable iteratee with plain key/value variables
638+
if (!$stmt->expr instanceof Variable || !is_string($stmt->expr->name)) {
639+
return false;
640+
}
641+
if (!$stmt->keyVar instanceof Variable || !is_string($stmt->keyVar->name)) {
642+
return false;
643+
}
644+
if (!$stmt->valueVar instanceof Variable || !is_string($stmt->valueVar->name)) {
645+
return false;
646+
}
647+
648+
$iterateeName = $stmt->expr->name;
649+
$keyName = $stmt->keyVar->name;
650+
651+
$desyncingNode = (new NodeFinder())->findFirst(
652+
$stmt->stmts,
653+
fn (Node $node): bool => $this->foreachAliasDesyncingNode($node, $iterateeName, $keyName),
654+
);
655+
656+
return $desyncingNode === null;
657+
}
658+
659+
private function foreachAliasDesyncingNode(Node $node, string $iterateeName, string $keyName): bool
660+
{
661+
if ($node instanceof Assign || $node instanceof Expr\AssignRef || $node instanceof Expr\AssignOp) {
662+
return $this->foreachAliasDesyncingTarget($node->var, $iterateeName, $keyName);
663+
}
664+
665+
if (
666+
$node instanceof Expr\PreInc || $node instanceof Expr\PreDec
667+
|| $node instanceof Expr\PostInc || $node instanceof Expr\PostDec
668+
) {
669+
return $this->foreachAliasDesyncingTarget($node->var, $iterateeName, $keyName);
670+
}
671+
672+
if ($node instanceof Stmt\Unset_) {
673+
foreach ($node->vars as $var) {
674+
if ($this->foreachAliasDesyncingTarget($var, $iterateeName, $keyName)) {
675+
return true;
676+
}
677+
}
678+
679+
return false;
680+
}
681+
682+
if ($node instanceof Expr\Closure) {
683+
foreach ($node->uses as $use) {
684+
if (
685+
$use->byRef
686+
&& is_string($use->var->name)
687+
&& ($use->var->name === $iterateeName || $use->var->name === $keyName)
688+
) {
689+
return true;
690+
}
691+
}
692+
693+
return false;
694+
}
695+
696+
if ($node instanceof Stmt\Global_) {
697+
foreach ($node->vars as $var) {
698+
if ($var instanceof Variable && is_string($var->name) && $var->name === $iterateeName) {
699+
return true;
700+
}
701+
}
702+
703+
return false;
704+
}
705+
706+
if ($node instanceof Stmt\Static_) {
707+
foreach ($node->vars as $staticVar) {
708+
if (is_string($staticVar->var->name) && $staticVar->var->name === $iterateeName) {
709+
return true;
710+
}
711+
}
712+
713+
return false;
714+
}
715+
716+
if ($node instanceof Foreach_ && $node->byRef) {
717+
// foreach ($iteratee as &$x) writes elements of the iteratee back
718+
return $node->expr instanceof Variable && is_string($node->expr->name) && $node->expr->name === $iterateeName;
719+
}
720+
721+
if (
722+
$node instanceof FuncCall || $node instanceof Expr\MethodCall
723+
|| $node instanceof Expr\NullsafeMethodCall || $node instanceof Expr\StaticCall
724+
|| $node instanceof Expr\New_
725+
) {
726+
// the whole iteratee (or the key) passed to a call may be mutated by
727+
// reference - conservatively treat it as a desync
728+
foreach ($node->getArgs() as $arg) {
729+
if (
730+
$arg->value instanceof Variable
731+
&& is_string($arg->value->name)
732+
&& ($arg->value->name === $iterateeName || $arg->value->name === $keyName)
733+
) {
734+
return true;
735+
}
736+
}
737+
738+
return false;
739+
}
740+
741+
return false;
742+
}
743+
744+
private function foreachAliasDesyncingTarget(Expr $target, string $iterateeName, string $keyName): bool
745+
{
746+
if ($target instanceof List_ || $target instanceof Array_) {
747+
foreach ($target->items as $item) {
748+
if ($item === null) {
749+
continue;
750+
}
751+
if ($this->foreachAliasDesyncingTarget($item->value, $iterateeName, $keyName)) {
752+
return true;
753+
}
754+
}
755+
756+
return false;
757+
}
758+
759+
// whole-iteratee reassignment ($iteratee = ...)
760+
if ($target instanceof Variable && is_string($target->name) && $target->name === $iterateeName) {
761+
return true;
762+
}
763+
764+
// a write into the iteratee is safe only when the outermost access on the
765+
// iteratee variable uses exactly the current key ($iteratee[$key]...); any
766+
// other offset, or an append ($iteratee[]), may hit a future iteration's
767+
// element
768+
$node = $target;
769+
while ($node instanceof ArrayDimFetch) {
770+
$inner = $node->var;
771+
if ($inner instanceof Variable && is_string($inner->name) && $inner->name === $iterateeName) {
772+
return !($node->dim instanceof Variable && is_string($node->dim->name) && $node->dim->name === $keyName);
773+
}
774+
775+
$node = $inner;
776+
}
777+
778+
return false;
779+
}
780+
608781
/**
609782
* @return array{bodyScope: MutatingScope, endScope: MutatingScope, totalKeys: int}|null
610783
*/

‎src/Node/Expr/ForeachValueAliasExpr.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
final class ForeachValueAliasExpr extends Expr implements VirtualNode
1818
{
1919

20+
public const KEY_PREFIX = '__phpstanForeachValueAlias(';
21+
2022
public Expr\Variable $var;
2123

2224
public function __construct(private string $variableName, public Expr\ArrayDimFetch $dimFetch)
@@ -38,7 +40,7 @@ public function getDimFetch(): Expr\ArrayDimFetch
3840
/** The expression key this node prints to - derivable from the value variable name alone. */
3941
public static function key(string $variableName): string
4042
{
41-
return sprintf('__phpstanForeachValueAlias(%s)', $variableName);
43+
return sprintf('%s%s)', self::KEY_PREFIX, $variableName);
4244
}
4345

4446
#[Override]

0 commit comments

Comments
 (0)