Skip to content

Commit d291bf2

Browse files
phpstan-botphpstan-bot
committed
Re-process a stored scope-changing expression at a foreign ask position instead of re-anchoring it
- NodeScopeResolver::processExprNode() answered an on-demand walk from a node's stored ExpressionResult re-anchored with atAskPosition(), which sets both the before and the after scope to the asking scope. For a node that changed the scope (assignment, assign-op, ++/--, list destructuring, ...) this dropped its effect: when ArrayMapFunctionReturnTypeExtension priced the synthetic `$callback(...)` call, the closure body's `$id = f()` was answered from storage without defining `$id`, so only the `if ($id === null)` branch survived and the callback seemed to return `null` only. - Such a node (stored after-scope !== before-scope) is now re-processed on the asking scope; its pure subnodes still answer from storage. Closures and arrow functions keep the existing answer (their stored result is their converged walk). The consume mode is unchanged. - Mirrored in turbo-ext/src/NodeScopeResolver.cpp (processExprNodeBody()). - Probed `+=`, `??=`, `++`, by-ref out parameter (preg_match), list destructuring, arrow function with an assignment and an array_filter() callback: `+=`, `++` and list destructuring had the same bug; the others already worked and are kept as coverage of the same path.
1 parent acd0511 commit d291bf2

5 files changed

Lines changed: 264 additions & 0 deletions

File tree

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -786,6 +786,18 @@ public function processExprNode(
786786
return $storedResult;
787787
}
788788

789+
// re-anchoring drops what the node did to the scope (an assignment
790+
// inside a closure body an extension prices on demand) - such a
791+
// node is re-processed on the asking scope instead
792+
if (
793+
!$consume
794+
&& $storedResult->getScope() !== $storedResult->getBeforeScope()
795+
&& !$expr instanceof Expr\Closure
796+
&& !$expr instanceof Expr\ArrowFunction
797+
) {
798+
return $this->processExprNodeInternal($stmt, $expr, $scope, $storage, $nodeCallback, $context);
799+
}
800+
789801
$reanchored = $storedResult->atAskPosition($scope);
790802
if ($consume) {
791803
// the re-anchored view IS this walk's result for the node

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

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15491;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
function idFromAlias(string $alias): ?string
8+
{
9+
return strpos($alias, '.') !== false ? md5($alias) : null;
10+
}
11+
12+
/**
13+
* @param array<string, mixed> $data
14+
* @return array<string, mixed>
15+
*/
16+
function withCoalesce(array $data): array
17+
{
18+
$data['ids'] = array_filter(
19+
array_map(
20+
function (string $alias): ?string {
21+
$id = idFromAlias($alias);
22+
if ($id === null) {
23+
echo 'invalid';
24+
}
25+
return $id;
26+
},
27+
explode('|', (string) ($data['options'] ?? ''))
28+
)
29+
);
30+
assertType('array<int<0, max>, non-falsy-string>', $data['ids']);
31+
32+
return $data;
33+
}
34+
35+
/**
36+
* @template T
37+
* @param T $x
38+
* @return T
39+
*/
40+
function identity($x)
41+
{
42+
return $x;
43+
}
44+
45+
/**
46+
* @param mixed $x
47+
* @return list<string>
48+
*/
49+
function strings($x): array
50+
{
51+
return [];
52+
}
53+
54+
/** @param array<string, mixed> $data */
55+
function assignOp(array $data): void
56+
{
57+
$x = identity(array_map(
58+
function (string $alias): int {
59+
$i = 0;
60+
$i += strlen($alias);
61+
if ($i === 0) {
62+
echo 'empty';
63+
}
64+
return $i;
65+
},
66+
strings($data['options'] ?? '')
67+
));
68+
assertType('list<int<0, max>>', $x);
69+
}
70+
71+
/** @param array<string, mixed> $data */
72+
function coalesceAssign(array $data): void
73+
{
74+
$x = identity(array_map(
75+
function (string $alias): ?string {
76+
$id = null;
77+
$id ??= idFromAlias($alias);
78+
if ($id === null) {
79+
echo 'invalid';
80+
}
81+
return $id;
82+
},
83+
strings($data['options'] ?? '')
84+
));
85+
assertType('list<string|null>', $x);
86+
}
87+
88+
/** @param array<string, mixed> $data */
89+
function increment(array $data): void
90+
{
91+
$x = identity(array_map(
92+
function (string $alias): int {
93+
$i = 0;
94+
$i++;
95+
if ($i === 0) {
96+
echo 'never';
97+
}
98+
return $i;
99+
},
100+
strings($data['options'] ?? '')
101+
));
102+
assertType('list<1>', $x);
103+
}
104+
105+
/** @param array<string, mixed> $data */
106+
function byRefOutParameter(array $data): void
107+
{
108+
$x = identity(array_map(
109+
function (string $alias): ?string {
110+
$m = null;
111+
preg_match('/a/', $alias, $m);
112+
if ($m === null) {
113+
echo 'never';
114+
}
115+
return $m[0] ?? null;
116+
},
117+
strings($data['options'] ?? '')
118+
));
119+
assertType('list<non-empty-string|null>', $x);
120+
}
121+
122+
/** @param array<string, mixed> $data */
123+
function listAssign(array $data): void
124+
{
125+
$x = identity(array_map(
126+
function (string $alias): ?string {
127+
[$id] = [idFromAlias($alias)];
128+
if ($id === null) {
129+
echo 'invalid';
130+
}
131+
return $id;
132+
},
133+
strings($data['options'] ?? '')
134+
));
135+
assertType('list<string|null>', $x);
136+
}
137+
138+
/** @param array<string, mixed> $data */
139+
function arrowFunction(array $data): void
140+
{
141+
$x = identity(array_map(
142+
fn (string $alias): ?string => ($id = idFromAlias($alias)) === null ? $id : $id,
143+
strings($data['options'] ?? '')
144+
));
145+
assertType('list<string|null>', $x);
146+
}
147+
148+
/** @param array<string, mixed> $data */
149+
function arrayFilterCallback(array $data): void
150+
{
151+
$x = identity(array_filter(
152+
strings($data['options'] ?? ''),
153+
function (string $alias): bool {
154+
$id = idFromAlias($alias);
155+
if ($id === null) {
156+
echo 'invalid';
157+
}
158+
return $id !== null;
159+
}
160+
));
161+
assertType('array<int<0, max>, string>', $x);
162+
}

‎tests/PHPStan/Rules/Functions/ArrayFilterRuleTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,4 +99,9 @@ public function testBug2065WithoutPhpDocTypesAsCertain(): void
9999
$this->analyse([__DIR__ . '/data/bug-array-filter.php'], []);
100100
}
101101

102+
public function testBug15491(): void
103+
{
104+
$this->analyse([__DIR__ . '/data/bug-15491.php'], []);
105+
}
106+
102107
}

‎tests/PHPStan/Rules/Functions/data/bug-15491.php‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15491Rule;
4+
5+
function idFromAlias(string $alias): ?string
6+
{
7+
return strpos($alias, '.') !== false ? md5($alias) : null;
8+
}
9+
10+
/**
11+
* @param array<string, mixed> $data
12+
* @return array<string, mixed>
13+
*/
14+
function withCoalesce(array $data): array
15+
{
16+
$data['ids'] = array_filter(
17+
array_map(
18+
function (string $alias): ?string {
19+
$id = idFromAlias($alias);
20+
if ($id === null) {
21+
echo 'invalid';
22+
}
23+
return $id;
24+
},
25+
explode('|', (string) ($data['options'] ?? ''))
26+
)
27+
);
28+
29+
return $data;
30+
}
31+
32+
/**
33+
* @param array<string, mixed> $data
34+
* @return array<string, mixed>
35+
*/
36+
function withoutCoalesce(array $data): array
37+
{
38+
$data['ids'] = array_filter(
39+
array_map(
40+
function (string $alias): ?string {
41+
$id = idFromAlias($alias);
42+
if ($id === null) {
43+
echo 'invalid';
44+
}
45+
return $id;
46+
},
47+
explode('|', (string) $data['options'])
48+
)
49+
);
50+
51+
return $data;
52+
}
53+
54+
/**
55+
* @param array<string, mixed> $data
56+
* @return array<string, mixed>
57+
*/
58+
function withoutIf(array $data): array
59+
{
60+
$data['ids'] = array_filter(
61+
array_map(
62+
function (string $alias): ?string {
63+
$id = idFromAlias($alias);
64+
return $id;
65+
},
66+
explode('|', (string) ($data['options'] ?? ''))
67+
)
68+
);
69+
70+
return $data;
71+
}

‎turbo-ext/src/NodeScopeResolver.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1963,6 +1963,20 @@ class NodeScopeResolver
19631963
if (UNEXPECTED(beforeScope.isUndef())) return zv::Val();
19641964
if (beforeScope.ref().isObject() && Z_OBJ_P(beforeScope.raw()) == Z_OBJ_P(scope)) return storedResult;
19651965

1966+
// re-anchoring drops what the node did to the scope - such a
1967+
// node is re-processed on the asking scope instead
1968+
if (!consume) {
1969+
zv::Val afterScope = expressionResultScope(storedResult.raw());
1970+
if (UNEXPECTED(afterScope.isUndef())) return zv::Val();
1971+
bool sameScope = afterScope.ref().isObject() && beforeScope.ref().isObject() && Z_OBJ_P(afterScope.raw()) == Z_OBJ_P(beforeScope.raw());
1972+
if (!sameScope) {
1973+
bool isClosure;
1974+
if (UNEXPECTED(!isInstance(Z_OBJ_P(expr), PT_CLASS_CLOSURE_EXPR, isClosure))) return zv::Val();
1975+
if (!isClosure && UNEXPECTED(!isInstance(Z_OBJ_P(expr), PT_CLASS_ARROW_FUNCTION, isClosure))) return zv::Val();
1976+
if (!isClosure) return processExprNodeInternal(stmt, expr, scope, storage, nodeCallback, context);
1977+
}
1978+
}
1979+
19661980
zv::Val reanchored = pt_expression_result_at_ask_position(storedResult.raw(), scope);
19671981
if (UNEXPECTED(reanchored.isUndef())) return zv::Val();
19681982
if (consume && UNEXPECTED(!thisStoreExpressionResult(storage, expr, reanchored.raw()))) return zv::Val();

0 commit comments

Comments
 (0)