Skip to content

Commit e2f5930

Browse files
phpstan-botondrejmirtes
phpstan-bot
authored and
ondrejmirtes
committed
Rebase adopted entries onto this pass's walk state when a loop pass converges with the previous one
- StatementsHandler::walkAgainstPreviousPass(): when a later loop pass converges at statement $i, the previous pass's per-statement StatementListWalkState entries were stored verbatim in this pass's record. Their cumulative exit/throw/impure points, dependencies and variable flows carried the previous pass's prefix, so the next pass slicing them by count in appendRecordedStatementResults() picked the wrong points and lost e.g. a `break` (and the array key set before it) or a `throw`. - Each adopted entry $j > $i is now this pass's state at $i plus the previous pass's delta from entry $i to entry $j, with the previous pass's scope at $j; entry $i keeps this pass's own snapshot. - Same change in the turbo-ext C++ mirror (turbo-ext/src/StatementsHandler.cpp). - The other converged-branch sites (template argument observation re-walk, static variable inference) read only the self-consistent observation-pass entries and do not record them for later passes, so they are not affected. - Regression test covers foreach (exit points) and a lost throw point in a try/catch; while/for/do-while variants of the same body are covered too (they converged differently and already produced the right types).
1 parent 1cb6abf commit e2f5930

3 files changed

Lines changed: 222 additions & 22 deletions

File tree

‎src/Analyser/StatementsHandler.php‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -523,14 +523,20 @@ private function walkAgainstPreviousPass(
523523
if ($replayTarget !== null && $previousRecording !== null) {
524524
$nodeScopeResolver->replayRecordingRange($previousRecording, $offset, $endOffset, $replayTarget, $storage, $scope);
525525
}
526-
$this->appendRecordedStatementResults($state, $recordedEntry, $recordedEnd);
527-
$state->scope = $recordedEnd->scope;
528526
if (!$finalWalk) {
529-
$base = $entries[$i][1];
530-
for ($j = $i; $j <= $stmtCount; $j++) {
531-
$entries[$j] = [$previous[0][$j][0], $base + $previous[0][$j][1] - $offset];
527+
// the previous pass's entries accumulated its own
528+
// points before statement $i: rebase them onto this pass's
529+
[$entryState, $base] = $entries[$i];
530+
for ($j = $i + 1; $j <= $stmtCount; $j++) {
531+
[$recordedState, $recordedOffset] = $previous[0][$j];
532+
$adoptedState = clone $entryState;
533+
$this->appendRecordedStatementResults($adoptedState, $recordedEntry, $recordedState);
534+
$adoptedState->scope = $recordedState->scope;
535+
$entries[$j] = [$adoptedState, $base + $recordedOffset - $offset];
532536
}
533537
}
538+
$this->appendRecordedStatementResults($state, $recordedEntry, $recordedEnd);
539+
$state->scope = $recordedEnd->scope;
534540

535541
return [$state->toResult(), $entries];
536542
}

‎tests/PHPStan/Analyser/nsrt/bug-15521.php‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15521;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
interface Search
8+
{
9+
10+
public function isEmpty(): bool;
11+
12+
}
13+
14+
interface Factory
15+
{
16+
17+
/** @return array{Search, bool} */
18+
public function create(string $filter, bool $changeStep): array;
19+
20+
}
21+
22+
/**
23+
* @param list<string> $steps
24+
*/
25+
function foo(array $steps, Factory $factory): void
26+
{
27+
$resultData = ['a' => 1];
28+
$changeStep = false;
29+
30+
foreach ($steps as $step => $filter) {
31+
[$search, $emptyResultAllowed] = $factory->create($filter, $changeStep);
32+
33+
if ($changeStep) {
34+
if (!$search->isEmpty()) {
35+
$resultData['changeStep'] = $step;
36+
37+
break;
38+
}
39+
40+
continue;
41+
}
42+
43+
if ($emptyResultAllowed || !$search->isEmpty()) {
44+
$resultData = ['a' => 2];
45+
46+
break;
47+
}
48+
49+
$changeStep = true;
50+
}
51+
52+
assertType('array{a: 1, changeStep?: int<0, max>}|array{a: 2}', $resultData);
53+
}
54+
55+
/**
56+
* @param list<string> $steps
57+
*/
58+
function whileLoop(array $steps, Factory $factory): void
59+
{
60+
$resultData = ['a' => 1];
61+
$changeStep = false;
62+
$step = 0;
63+
64+
while ($step < count($steps)) {
65+
[$search, $emptyResultAllowed] = $factory->create($steps[$step], $changeStep);
66+
67+
if ($changeStep) {
68+
if (!$search->isEmpty()) {
69+
$resultData['changeStep'] = $step;
70+
71+
break;
72+
}
73+
74+
$step++;
75+
continue;
76+
}
77+
78+
if ($emptyResultAllowed || !$search->isEmpty()) {
79+
$resultData = ['a' => 2];
80+
81+
break;
82+
}
83+
84+
$changeStep = true;
85+
}
86+
87+
assertType('array{a: 1, changeStep?: int<0, max>}|array{a: 2}', $resultData);
88+
}
89+
90+
/**
91+
* @param list<string> $steps
92+
*/
93+
function forLoop(array $steps, Factory $factory): void
94+
{
95+
$resultData = ['a' => 1];
96+
$changeStep = false;
97+
98+
for ($step = 0; $step < count($steps); $step++) {
99+
[$search, $emptyResultAllowed] = $factory->create($steps[$step], $changeStep);
100+
101+
if ($changeStep) {
102+
if (!$search->isEmpty()) {
103+
$resultData['changeStep'] = $step;
104+
105+
break;
106+
}
107+
108+
continue;
109+
}
110+
111+
if ($emptyResultAllowed || !$search->isEmpty()) {
112+
$resultData = ['a' => 2];
113+
114+
break;
115+
}
116+
117+
$changeStep = true;
118+
}
119+
120+
assertType('array{a: 1, changeStep?: int<1, max>}|array{a: 2}', $resultData);
121+
}
122+
123+
/**
124+
* @param list<string> $steps
125+
*/
126+
function doWhileLoop(array $steps, Factory $factory): void
127+
{
128+
$resultData = ['a' => 1];
129+
$changeStep = false;
130+
$step = 0;
131+
132+
do {
133+
[$search, $emptyResultAllowed] = $factory->create($steps[$step], $changeStep);
134+
135+
if ($changeStep) {
136+
if (!$search->isEmpty()) {
137+
$resultData['changeStep'] = $step;
138+
139+
break;
140+
}
141+
142+
$step++;
143+
continue;
144+
}
145+
146+
if ($emptyResultAllowed || !$search->isEmpty()) {
147+
$resultData = ['a' => 2];
148+
149+
break;
150+
}
151+
152+
$changeStep = true;
153+
} while ($step < count($steps));
154+
155+
assertType('array{a: 1, changeStep?: int<0, max>}|array{a: 2}', $resultData);
156+
}
157+
158+
/**
159+
* @param list<string> $steps
160+
*/
161+
function throwPoints(array $steps, Factory $factory): void
162+
{
163+
$resultData = 1;
164+
$changeStep = false;
165+
166+
try {
167+
foreach ($steps as $step => $filter) {
168+
[$search, $emptyResultAllowed] = $factory->create($filter, $changeStep);
169+
170+
if ($changeStep) {
171+
if (!$search->isEmpty()) {
172+
$resultData = 3;
173+
throw new \RuntimeException();
174+
}
175+
176+
continue;
177+
}
178+
179+
if ($emptyResultAllowed || !$search->isEmpty()) {
180+
$resultData = 2;
181+
182+
break;
183+
}
184+
185+
$changeStep = true;
186+
}
187+
} catch (\RuntimeException $e) {
188+
assertType('1|3', $resultData);
189+
}
190+
}

‎turbo-ext/src/StatementsHandler.cpp‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -759,25 +759,29 @@ class StatementsHandler
759759
zval *recordedEnd = entryState(recordedEndPair);
760760
zend_long endOffset = zval_get_long(entryOffset(recordedEndPair));
761761
if (replayTarget != NULL && previousRecording != NULL && UNEXPECTED(!pt_node_scope_resolver_replay_recording_range(nodeScopeResolver, previousRecording, offset, endOffset, replayTarget, storage, scope))) return zv::Val();
762+
if (!finalWalk) {
763+
// the previous pass's entries accumulated its own
764+
// points before statement $i: rebase them onto this pass's
765+
zval *entryPair = entryAt(entries.raw(), i);
766+
if (UNEXPECTED(entryPair == NULL)) return zv::Val();
767+
zv::Val entryStateHeld = zv::Val::copyOf(zv::Ref(entryState(entryPair)));
768+
for (zend_long j = i + 1; j <= stmtCount; j++) {
769+
zval *pair = entryAt(previousEntries, j);
770+
if (UNEXPECTED(pair == NULL)) return zv::Val();
771+
zval *recordedState = entryState(pair);
772+
zv::Val adoptedState = cloneObject(entryStateHeld.raw());
773+
if (UNEXPECTED(adoptedState.isUndef())) return zv::Val();
774+
if (UNEXPECTED(!appendRecordedStatementResults(adoptedState.raw(), recordedEntry, recordedState))) return zv::Val();
775+
zv::Ref(OBJ_PROP_NUM(Z_OBJ_P(adoptedState.raw()), stateSlots::scope)).assign(zv::Val::copyOf(zv::Ref(OBJ_PROP_NUM(Z_OBJ_P(recordedState), stateSlots::scope))));
776+
zv::Arr shifted = zv::Arr::create(2);
777+
shifted.push(std::move(adoptedState));
778+
shifted.push(zv::Val::integer(entryCount + zval_get_long(entryOffset(pair)) - offset));
779+
entries.push(zv::Val(std::move(shifted)));
780+
}
781+
}
762782
if (UNEXPECTED(!appendRecordedStatementResults(state.raw(), recordedEntry, recordedEnd))) return zv::Val();
763783
zv::Ref(OBJ_PROP_NUM(Z_OBJ_P(state.raw()), stateSlots::scope)).assign(zv::Val::copyOf(zv::Ref(OBJ_PROP_NUM(Z_OBJ_P(recordedEnd), stateSlots::scope))));
764-
if (finalWalk) return pt_statement_list_walk_state_to_result(state.raw());
765-
// $entries[$j] = [$previous[0][$j][0], $base + $previous[0][$j][1] - $offset] for $j >= $i
766-
zv::Arr adopted = zv::Arr::create((uint32_t) stmtCount + 1);
767-
zend_long j = 0;
768-
for (auto existing : zv::ArrRef(entries.raw())) {
769-
if (j++ >= i) break;
770-
adopted.push(zv::Val::copyOf(existing.value()));
771-
}
772-
for (j = i; j <= stmtCount; j++) {
773-
zval *pair = entryAt(previousEntries, j);
774-
if (UNEXPECTED(pair == NULL)) return zv::Val();
775-
zv::Arr shifted = zv::Arr::create(2);
776-
shifted.push(zv::Val::copyOf(zv::Ref(entryState(pair))));
777-
shifted.push(zv::Val::integer(entryCount + zval_get_long(entryOffset(pair)) - offset));
778-
adopted.push(zv::Val(std::move(shifted)));
779-
}
780-
*record = passRecord(zv::Val(std::move(adopted)), recording);
784+
if (!finalWalk) *record = passRecord(zv::Val(std::move(entries)), recording);
781785
return pt_statement_list_walk_state_to_result(state.raw());
782786
}
783787
bool reWalk = differingRoots.isNull();

0 commit comments

Comments
 (0)