Skip to content

Commit 2152a04

Browse files
ondrejmirtesclaude
ondrejmirtes
and
claude
committed
Keep variable-write markers out of loop convergence decisions
The markers made consecutive convergence-pass scopes compare unequal, so loops sometimes took extra passes or skipped pass replays - and the extra generalization could change inferred types, accidentally masking engine bugs (the array_shift-in-while case converged to better types than the engine without tracking produces). Convergence termination and pass-replay gates now use equalsIgnoringVariableWriteMarkers(), making analysis results independent of the tracking again. Replaying a recorded pass can now settle on an entry scope no pass was walked with, so the frame records which names the body reads and the replay sites reconcile first: every read name's writes reaching the settled entry count as read, keeping back-edge reads correct in type-stable loops. The array_shift NSRT test temporarily pins the engine's actual (wrong) non-empty-list behaviour; the proper convergence fix on 2.2.x flips it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
1 parent 11d16c3 commit 2152a04

9 files changed

Lines changed: 122 additions & 63 deletions

File tree

‎src/Analyser/MutatingScope.php‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5332,10 +5332,10 @@ public function equals(self $otherScope): bool
53325332
return false;
53335333
}
53345334

5335-
if (!$this->compareVariableTypeHolders($this->expressionTypes, $otherScope->expressionTypes)) {
5335+
if (!$this->compareVariableTypeHolders($this->expressionTypes, $otherScope->expressionTypes, false)) {
53365336
return false;
53375337
}
5338-
if (!$this->compareVariableTypeHolders($this->nativeExpressionTypes, $otherScope->nativeExpressionTypes)) {
5338+
if (!$this->compareVariableTypeHolders($this->nativeExpressionTypes, $otherScope->nativeExpressionTypes, false)) {
53395339
return false;
53405340
}
53415341

@@ -5386,16 +5386,45 @@ private function compareConditionalExpressions(array $conditionalExpressions, ar
53865386
return true;
53875387
}
53885388

5389+
/**
5390+
* Equality for loop and closure convergence: the unused-variable write
5391+
* markers are bookkeeping, not analysis state - two passes that differ
5392+
* only in them must converge exactly like they would without the
5393+
* tracking, or the markers would change inferred types. The strict
5394+
* equals() stays in charge of pass replays, where a marker difference
5395+
* means the recorded walk saw different reaching writes.
5396+
*/
5397+
public function equalsIgnoringVariableWriteMarkers(self $otherScope): bool
5398+
{
5399+
if (!$this->context->equals($otherScope->context)) {
5400+
return false;
5401+
}
5402+
5403+
if (!$this->compareVariableTypeHolders($this->expressionTypes, $otherScope->expressionTypes, true)) {
5404+
return false;
5405+
}
5406+
if (!$this->compareVariableTypeHolders($this->nativeExpressionTypes, $otherScope->nativeExpressionTypes, true)) {
5407+
return false;
5408+
}
5409+
5410+
return $this->compareConditionalExpressions($this->conditionalExpressions, $otherScope->conditionalExpressions);
5411+
}
5412+
53895413
/**
53905414
* @param array<string, ExpressionTypeHolder> $variableTypeHolders
53915415
* @param array<string, ExpressionTypeHolder> $otherVariableTypeHolders
53925416
*/
5393-
private function compareVariableTypeHolders(array $variableTypeHolders, array $otherVariableTypeHolders): bool
5417+
private function compareVariableTypeHolders(array $variableTypeHolders, array $otherVariableTypeHolders, bool $ignoreVariableWriteMarkers): bool
53945418
{
5395-
if (count($variableTypeHolders) !== count($otherVariableTypeHolders)) {
5419+
if (!$ignoreVariableWriteMarkers && count($variableTypeHolders) !== count($otherVariableTypeHolders)) {
53965420
return false;
53975421
}
5422+
$count = 0;
53985423
foreach ($variableTypeHolders as $variableExprString => $variableTypeHolder) {
5424+
if ($ignoreVariableWriteMarkers && str_starts_with($variableExprString, VariableWrittenExpr::KEY_PREFIX)) {
5425+
continue;
5426+
}
5427+
$count++;
53995428
if (!isset($otherVariableTypeHolders[$variableExprString])) {
54005429
return false;
54015430
}
@@ -5408,6 +5437,17 @@ private function compareVariableTypeHolders(array $variableTypeHolders, array $o
54085437
return false;
54095438
}
54105439
}
5440+
if ($ignoreVariableWriteMarkers) {
5441+
$otherCount = 0;
5442+
foreach (array_keys($otherVariableTypeHolders) as $variableExprString) {
5443+
if (str_starts_with($variableExprString, VariableWrittenExpr::KEY_PREFIX)) {
5444+
continue;
5445+
}
5446+
$otherCount++;
5447+
}
5448+
5449+
return $count === $otherCount;
5450+
}
54115451

54125452
return true;
54135453
}

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ private function resolveBackwardGotoScope(
509509
if ($mergeBodyScopeEachIteration) {
510510
$bodyScope = $bodyScope->mergeWith($scope);
511511
}
512-
if ($prevEntryScope !== null && $bodyScope->equals($prevEntryScope)) {
512+
if ($prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)) {
513513
// walking is deterministic in the entry scope - an unchanged entry
514514
// reproduces the previous pass's exit, so the verification walk is skipped
515515
$bodyScope = $prevScope;
@@ -540,7 +540,7 @@ private function resolveBackwardGotoScope(
540540
$bodyScope = $scope->mergeWith($gotoScope);
541541
}
542542

543-
if ($bodyScope->equals($prevScope)) {
543+
if ($bodyScope->equalsIgnoringVariableWriteMarkers($prevScope)) {
544544
break;
545545
}
546546

@@ -1981,6 +1981,25 @@ public function markAllReachingVariablesRead(MutatingScope $scope): void
19811981
$this->replaceVariableWritesFrame($newFrame);
19821982
}
19831983

1984+
/**
1985+
* A convergence loop is about to replay a recorded pass instead of really
1986+
* walking the body: reconcile the read tracking with the settled entry
1987+
* scope first, so writes reaching it through back edges count as read even
1988+
* though no pass was walked with this exact entry.
1989+
*/
1990+
public function markReadNamesReadOnScope(MutatingScope $scope): void
1991+
{
1992+
$frame = $this->getVariableWritesFrame();
1993+
if ($frame === null) {
1994+
return;
1995+
}
1996+
$newFrame = $frame->withReadsForAllReadNames($scope);
1997+
if ($newFrame === $frame) {
1998+
return;
1999+
}
2000+
$this->replaceVariableWritesFrame($newFrame);
2001+
}
2002+
19842003
/**
19852004
* The current position in the write-site id sequence - taken before walking
19862005
* a branch, so a statically dead branch's writes can be suppressed after.
@@ -2454,7 +2473,7 @@ private function processClosureNodeInternal(
24542473
$closureScope = $closureScope->processClosureScope($intermediaryClosureScope, $prevScope, $byRefUses);
24552474
$closureScope = $this->recordVariableImportWrites($this->getByValueClosureUseVariables($expr), VariableWrite::KIND_CLOSURE_USE, $closureScope);
24562475

2457-
if ($closureScope->equals($prevScope)) {
2476+
if ($closureScope->equalsIgnoringVariableWriteMarkers($prevScope)) {
24582477
break;
24592478
}
24602479
if ($count >= self::GENERALIZE_AFTER_ITERATION) {
@@ -2473,14 +2492,15 @@ private function processClosureNodeInternal(
24732492
if (
24742493
$replayBodyRecording !== null && $replayPassStorage !== null
24752494
&& $replayPassResult !== null && $replayEntryScope !== null
2476-
&& $closureScope->equals($replayEntryScope)
2495+
&& $closureScope->equalsIgnoringVariableWriteMarkers($replayEntryScope)
24772496
) {
24782497
// the final walk would repeat the recorded fixpoint pass exactly
24792498
// (same entry scope, deterministic walk) - adopt the pass's result
24802499
// and replay its emissions through the real callback instead.
24812500
// The pass's own entry scope takes over: the recorded pairs carry
24822501
// its anonymous-function reflection, which the gatherer's filter
24832502
// compares by identity (the state is equals-identical anyway).
2503+
$this->markReadNamesReadOnScope($closureScope);
24842504
$closureScope = $replayEntryScope;
24852505
$originalStorage->mergeResults($replayPassStorage);
24862506
$this->replayRecording($replayBodyRecording, $nodeCallback, $originalStorage, $closureScope);

‎src/Analyser/StmtHandler/DoWhileHandler.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public function processStmt(
5858
do {
5959
$prevScope = $bodyScope;
6060
$bodyScope = $bodyScope->mergeWith($scope);
61-
if ($prevEntryScope !== null && $bodyScope->equals($prevEntryScope)) {
61+
if ($prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)) {
6262
// walking is deterministic in the entry scope - an unchanged entry
6363
// reproduces the previous pass's exit (and repeats only idempotent
6464
// merges into the final scope), so the verification walk is skipped
@@ -91,7 +91,7 @@ public function processStmt(
9191
} finally {
9292
$scope->popExpressionResultStorage();
9393
}
94-
if ($bodyScope->equals($prevScope)) {
94+
if ($bodyScope->equalsIgnoringVariableWriteMarkers($prevScope)) {
9595
break;
9696
}
9797

@@ -107,12 +107,13 @@ public function processStmt(
107107
$storage = $originalStorage;
108108
if (
109109
$replayBodyRecording !== null && $replayPassStorage !== null && $replayPassResult !== null
110-
&& $prevEntryScope !== null && $bodyScope->equals($prevEntryScope)
110+
&& $prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)
111111
) {
112112
// the final body walk would repeat the recorded fixpoint pass exactly
113113
// (same entry scope, deterministic walk) - adopt the pass's results
114114
// and replay its emissions through the real callback instead; the
115115
// condition walks below stay real
116+
$nodeScopeResolver->markReadNamesReadOnScope($bodyScope);
116117
$originalStorage->mergeResults($replayPassStorage);
117118
$nodeScopeResolver->replayRecording($replayBodyRecording, $nodeCallback, $originalStorage, $scope);
118119
$bodyScopeResult = $replayPassResult;

‎src/Analyser/StmtHandler/ForHandler.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public function processStmt(
171171
$prevScope = $bodyScope;
172172
$storage = $originalStorage->duplicate();
173173
$bodyScope = $bodyScope->mergeWith($initScope);
174-
if ($prevEntryScope !== null && $bodyScope->equals($prevEntryScope)) {
174+
if ($prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)) {
175175
// walking is deterministic in the entry scope - an unchanged entry
176176
// reproduces the previous pass's exit, so the verification walk is skipped
177177
$bodyScope = $prevScope;
@@ -200,7 +200,7 @@ public function processStmt(
200200
$scope->popExpressionResultStorage();
201201
}
202202

203-
if ($bodyScope->equals($prevScope)) {
203+
if ($bodyScope->equalsIgnoringVariableWriteMarkers($prevScope)) {
204204
break;
205205
}
206206

‎src/Analyser/StmtHandler/ForeachHandler.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ static function () use ($condResult, $emptyArrayType): Type {
217217
do {
218218
$prevScope = $bodyScope;
219219
$bodyScope = $bodyScope->mergeWith($nonEmptyIterateeScope);
220-
if ($prevEntryScope !== null && $bodyScope->equals($prevEntryScope)) {
220+
if ($prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)) {
221221
// walking is deterministic in the entry scope - an unchanged entry
222222
// reproduces the previous pass's exit, so the verification walk is skipped
223223
$bodyScope = $prevScope;
@@ -245,7 +245,7 @@ static function () use ($condResult, $emptyArrayType): Type {
245245
$replayPassResult = $bodyScopeResult;
246246
$replayEntryScope = $prevEntryScope;
247247
}
248-
if ($bodyScope->equals($prevScope)) {
248+
if ($bodyScope->equalsIgnoringVariableWriteMarkers($prevScope)) {
249249
break;
250250
}
251251

@@ -264,11 +264,12 @@ static function () use ($condResult, $emptyArrayType): Type {
264264
if (
265265
$replayBodyRecording !== null && $replayPassStorage !== null
266266
&& $replayPassResult !== null && $replayEntryScope !== null
267-
&& $unrolledTotalKeys === null && $finalEntryScope->equals($replayEntryScope)
267+
&& $unrolledTotalKeys === null && $finalEntryScope->equalsIgnoringVariableWriteMarkers($replayEntryScope)
268268
) {
269269
// the final walk would repeat the recorded fixpoint pass exactly
270270
// (same entry scope, deterministic walk) - adopt the pass's results
271271
// and replay its emissions through the real callback instead
272+
$nodeScopeResolver->markReadNamesReadOnScope($finalEntryScope);
272273
$originalStorage->mergeResults($replayPassStorage);
273274
$nodeScopeResolver->replayRecording($replayBodyRecording, $nodeCallback, $originalStorage, $scope);
274275
$finalScopeResult = $replayPassResult;
@@ -821,7 +822,7 @@ private function tryProcessUnrolledConstantArrayForeach(
821822
$endScope = $endScope->mergeWith($breakExitPoint->getScope());
822823
}
823824
$bodyScope = $bodyScope->mergeWith($loopScope);
824-
if ($loopScope->equals($prevLoopScope)) {
825+
if ($loopScope->equalsIgnoringVariableWriteMarkers($prevLoopScope)) {
825826
break;
826827
}
827828
if ($count >= NodeScopeResolver::GENERALIZE_AFTER_ITERATION) {

‎src/Analyser/StmtHandler/WhileHandler.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public function processStmt(
8383
do {
8484
$prevScope = $bodyScope;
8585
$bodyScope = $bodyScope->mergeWith($scope);
86-
if ($prevEntryScope !== null && $bodyScope->equals($prevEntryScope)) {
86+
if ($prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)) {
8787
// walking is deterministic in the entry scope - an unchanged entry
8888
// reproduces the previous pass's exit, so the verification walk is skipped
8989
$bodyScope = $prevScope;
@@ -114,7 +114,7 @@ public function processStmt(
114114
$replayPassResult = $bodyScopeResult;
115115
$replayCondResult = $passCondResult;
116116
}
117-
if ($bodyScope->equals($prevScope)) {
117+
if ($bodyScope->equalsIgnoringVariableWriteMarkers($prevScope)) {
118118
break;
119119
}
120120

@@ -132,11 +132,12 @@ public function processStmt(
132132
$replayCondRecording !== null && $replayBodyRecording !== null
133133
&& $replayPassStorage !== null && $replayPassResult !== null
134134
&& $replayCondResult !== null
135-
&& $prevEntryScope !== null && $bodyScope->equals($prevEntryScope)
135+
&& $prevEntryScope !== null && $bodyScope->equalsIgnoringVariableWriteMarkers($prevEntryScope)
136136
) {
137137
// the final walk would repeat the recorded fixpoint pass exactly
138138
// (same entry scope, deterministic walk) - adopt the pass's results
139139
// and replay its emissions through the real callback instead
140+
$nodeScopeResolver->markReadNamesReadOnScope($bodyScope);
140141
$originalStorage->mergeResults($replayPassStorage);
141142
$nodeScopeResolver->replayRecording($replayCondRecording, $nodeCallback, $originalStorage, $scope);
142143
// the While_ callback is deferred from processStmtNode(): it fires

0 commit comments

Comments
 (0)