Skip to content

Commit c6cbe79

Browse files
ondrejmirtesclaude
andcommitted
Walk the dynamic variable name once, drop the vestigial clone walks
Two leftover noop-callback walks over real AST: - A dynamic-name read-modify-write target (`$$name op= ...`) priced the whole variable through the walk machinery because the name expression was only walked later by the write flow. PHP evaluates the name before reading the old value, so prepareTarget() now walks it once - with the real node callback, at its actual evaluation point - composes the read from the result, and threads it on the PreparedAssignTarget; the write flow consumes it instead of walking the name again after the value. - The ExistingArrayDimFetch branch walked the cloned chain so its nodes had stored results, but the composition below prices the clones directly through the scope - the walks had no observable effect here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DaBZjgksga4c5s6Q9FniY7
1 parent ef1e49f commit c6cbe79

2 files changed

Lines changed: 30 additions & 14 deletions

File tree

src/Analyser/ExprHandler/AssignHandler.php

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -448,24 +448,29 @@ public function prepareTarget(
448448
$isAlwaysTerminating = false;
449449
$isAssignOp = $assignedExpr instanceof Expr\AssignOp && !$enterExpressionAssign;
450450
if ($var instanceof Variable) {
451+
$variableNameResult = null;
451452
if ($mode->producesTargetReadResult()) {
452453
// `$lvalue OP= ...` reads the old value of `$lvalue`; the write walk
453454
// processes a Variable target only as an assignment target, never as
454455
// a read. The read is composed here without a walk - for ??= with
455456
// isset() semantics (mirroring CoalesceHandler's left-side
456457
// processing, with the isset descriptor - bug-13623).
458+
if (!is_string($var->name)) {
459+
// `$$name OP= ...` evaluates the name before reading the old
460+
// value: walk it once here, the write flow consumes the result
461+
$variableNameResult = $nodeScopeResolver->processExprNode($stmt, $var->name, $scope, $storage, $nodeCallback, $context);
462+
$hasYield = $variableNameResult->hasYield();
463+
$throwPoints = $variableNameResult->getThrowPoints();
464+
$impurePoints = $variableNameResult->getImpurePoints();
465+
$isAlwaysTerminating = $variableNameResult->isAlwaysTerminating();
466+
$scope = $variableNameResult->getScope();
467+
}
457468
$readScope = $scope;
458469
if ($mode->issetSemanticsForRead()) {
459470
$nonNullabilityResult = $this->nonNullabilityHelper->ensureNonNullability($scope, $var);
460471
$readScope = $nodeScopeResolver->lookForSetAllowedUndefinedExpressions($nonNullabilityResult->getScope(), $var);
461472
}
462-
if (is_string($var->name)) {
463-
$targetReadResult = $this->variableHandler->composeResult($var, null, $readScope);
464-
} else {
465-
// a dynamic-name target: the name expression is only walked later
466-
// by the write flow, so the read prices it here first
467-
$targetReadResult = $nodeScopeResolver->processExprNode($stmt, $var, $readScope, $storage, new NoopNodeCallback(), $context->enterDeep());
468-
}
473+
$targetReadResult = $this->variableHandler->composeResult($var, $variableNameResult, $readScope);
469474
}
470475

471476
return new PreparedAssignTarget(
@@ -481,6 +486,7 @@ public function prepareTarget(
481486
$impurePoints,
482487
$isAlwaysTerminating,
483488
targetReadResult: $targetReadResult,
489+
variableNameResult: $variableNameResult,
484490
);
485491
}
486492

@@ -754,16 +760,13 @@ public function prepareTarget(
754760
$var = $var->getVar();
755761
}
756762

757-
// the chain is usually a clone of AST nodes already processed elsewhere
758-
// (see Unset_ handling) - process it with a noop callback so that
759-
// results for its nodes are stored without invoking rules twice
760-
$nodeScopeResolver->processExprNode($stmt, $var, $scope, $storage, new NoopNodeCallback(), $context->enterDeep());
761-
763+
// the chain is a clone of AST nodes already processed elsewhere (see
764+
// Unset_ handling) - the types below price the clones directly, no
765+
// walk is needed
762766
$offsetTypes = [];
763767
$offsetNativeTypes = [];
764768
foreach (array_reverse($dimFetchStack) as $dimFetch) {
765769
$dimExpr = $dimFetch->getDim();
766-
$nodeScopeResolver->processExprNode($stmt, $dimExpr, $scope, $storage, new NoopNodeCallback(), $context->enterDeep());
767770
$offsetTypes[] = [$scope->getType($dimExpr), $dimFetch];
768771
$offsetNativeTypes[] = [$scope->getNativeType($dimExpr), $dimFetch];
769772
}
@@ -956,7 +959,10 @@ public function applyWrite(
956959
if ($assignedExpr instanceof Expr\Array_) {
957960
$scope = $this->processArrayByRefItems($scope, $var->name, $assignedExpr, new Variable($var->name));
958961
}
959-
} else {
962+
} elseif ($target->getVariableNameResult() === null) {
963+
// a plain assignment does not read the target, so the dynamic name
964+
// is walked here; read-modify-write targets walked it in
965+
// prepareTarget() and already carry its state
960966
$nameExprResult = $nodeScopeResolver->processExprNode($stmt, $var->name, $scope, $storage, $nodeCallback, $context);
961967
$hasYield = $hasYield || $nameExprResult->hasYield();
962968
$throwPoints = array_merge($throwPoints, $nameExprResult->getThrowPoints());

src/Analyser/PreparedAssignTarget.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ public function __construct(
6161
private ?string $propertyName = null,
6262
private ?Type $propertyHolderType = null,
6363
private ?ExpressionResult $targetReadResult = null,
64+
private ?ExpressionResult $variableNameResult = null,
6465
)
6566
{
6667
}
@@ -236,4 +237,13 @@ public function getTargetReadResult(): ExpressionResult
236237
return $this->targetReadResult;
237238
}
238239

240+
/**
241+
* A dynamic variable name (`$$name`) already walked by prepareTarget() -
242+
* read-modify-write targets evaluate the name before reading the old value.
243+
*/
244+
public function getVariableNameResult(): ?ExpressionResult
245+
{
246+
return $this->variableNameResult;
247+
}
248+
239249
}

0 commit comments

Comments
 (0)