Skip to content

Commit 119d067

Browse files
phpstan-botondrejmirtes
phpstan-bot
authored and
ondrejmirtes
committed
Consume only the nullsafe twin's receiver from stored results instead of its whole subtree
- NodeScopeResolver::processExprNodeConsumingStored() now takes the node to consume (the twin's receiver). The bool $consumeStoredExpressionResults flag becomes ?Expr $consumeStoredExpression, and processExprNode() consumes a stored result unconditionally only for that exact node. Everything else in the plain MethodCall / PropertyFetch twin (arguments, a dynamic method or property name) takes the normal path. - Since #6765, later loop convergence passes are seeded with the previous pass's stored results. The over-broad consume mode then answered the twin's arguments from pass-1 results without walking them. Rules never saw those nodes and gatherers never recorded them, so errors went missing and property.onlyWritten was reported falsely. - NullsafeMethodCallHandler / NullsafePropertyFetchHandler pass $expr->var as the consumed node. - ArgumentsHandler no longer consumes stored closure / arrow-function argument results in consume mode (removed NodeScopeResolver::isConsumingStoredExpressionResults()). Consume mode no longer reaches arguments, and that branch was the same bug for closures in `?->` arguments. - Mirrored in turbo-ext (NodeScopeResolver.cpp, ArgumentsHandler.cpp, Nullsafe*Handler.cpp, support.h, generated/NodeScopeResolver.h). - Probed and covered: foreach/while/do-while/for, closure and arrow-function arguments, a dynamic nullsafe property name, a dynamic nullsafe method name, nested `?->` in arguments. Also checked chained `?->`, `??`/isset around `?->`, spread and named arguments; all are fixed by the same change.
1 parent e2f5930 commit 119d067

14 files changed

Lines changed: 400 additions & 212 deletions

‎src/Analyser/ArgumentsHandler.php‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -370,18 +370,14 @@ public function processArgs(
370370
if ($arg->value instanceof Expr\Closure) {
371371

372372
$storedClosureArgResult = null;
373-
if ($nodeScopeResolver->isReturningStoredExpressionResults() || $nodeScopeResolver->isConsumingStoredExpressionResults()) {
373+
if ($nodeScopeResolver->isReturningStoredExpressionResults()) {
374374
// an on-demand re-walk of the enclosing call must not re-run the
375375
// closure's whole by-ref convergence: consume the main walk's
376376
// stored result, or (when the body release already dropped it)
377377
// price the closure through getClosureType's per-node cache -
378378
// a single body walk on miss, none on repeat asks
379379
$storedClosureArgResult = $storage->findExpressionResult($arg->value);
380-
// consume mode alone (the nullsafe call's plain-twin walk) is the
381-
// FIRST and only walk of these arguments - nothing stored them, so
382-
// a miss means "walk it", not "price it silently": pricing skips
383-
// the body walk and never fires the closure's node callbacks
384-
if ($storedClosureArgResult === null && $nodeScopeResolver->isReturningStoredExpressionResults()) {
380+
if ($storedClosureArgResult === null) {
385381
$storedClosureArgResult = $this->expressionResultFactory->create(
386382
$scopeToPass,
387383
beforeScope: $scopeToPass,
@@ -528,12 +524,10 @@ public function processArgs(
528524
} elseif ($arg->value instanceof Expr\ArrowFunction) {
529525

530526
$storedClosureArgResult = null;
531-
if ($nodeScopeResolver->isReturningStoredExpressionResults() || $nodeScopeResolver->isConsumingStoredExpressionResults()) {
527+
if ($nodeScopeResolver->isReturningStoredExpressionResults()) {
532528
// see the Closure branch above - consume or price via the cache
533529
$storedClosureArgResult = $storage->findExpressionResult($arg->value);
534-
// consume mode alone (the nullsafe call's plain-twin walk) is the
535-
// first and only walk of the argument - a miss means walk it
536-
if ($storedClosureArgResult === null && $nodeScopeResolver->isReturningStoredExpressionResults()) {
530+
if ($storedClosureArgResult === null) {
537531
$storedClosureArgResult = $this->expressionResultFactory->create(
538532
$scopeToPass,
539533
beforeScope: $scopeToPass,

‎src/Analyser/ExprHandler/NullsafeMethodCallHandler.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
9292
$exprResult = $nodeScopeResolver->processExprNodeConsumingStored(
9393
$stmt,
9494
$methodCall,
95+
$expr->var,
9596
$nonNullabilityResult->getScope(),
9697
$storage,
9798
$nodeCallback,

‎src/Analyser/ExprHandler/NullsafePropertyFetchHandler.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
8585
$expr->name,
8686
$attributes,
8787
);
88-
$exprResult = $nodeScopeResolver->processExprNodeConsumingStored($stmt, $propertyFetch, $nonNullabilityResult->getScope(), $storage, $nodeCallback, $context);
88+
$exprResult = $nodeScopeResolver->processExprNodeConsumingStored($stmt, $propertyFetch, $expr->var, $nonNullabilityResult->getScope(), $storage, $nodeCallback, $context);
8989
$scope = $this->nonNullabilityHelper->revertNonNullability($exprResult->getScope(), $nonNullabilityResult->getSpecifiedExpressions());
9090

9191
// The `?->`'s own type on the asking scope. $receiverType is the receiver's

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,13 @@ class NodeScopeResolver
8080
/**
8181
* Consume-stored mode: a walk that deliberately re-enters an
8282
* already-walked subtree (the nullsafe plain twin re-walking its
83-
* receiver) consumes stored results unconditionally instead of
84-
* re-processing - node callbacks fired during the original walk.
83+
* receiver) consumes this node's stored result unconditionally instead
84+
* of re-processing it - node callbacks fired during the original walk.
85+
* Only this exact node is consumed: the rest of the twin (arguments,
86+
* a dynamic name) is walked normally, even when the storage was seeded
87+
* with results of an earlier loop convergence pass.
8588
*/
86-
private bool $consumeStoredExpressionResults = false;
89+
private ?Expr $consumeStoredExpression = null;
8790

8891
private ?NonNullabilityHelper $nonNullabilityHelper = null;
8992

@@ -493,12 +496,6 @@ public function isReturningStoredExpressionResults(): bool
493496
return $this->returnStoredExpressionResults;
494497
}
495498

496-
/** Whether the walk consumes stored results unconditionally (see processExprNodeConsumingStored()). */
497-
public function isConsumingStoredExpressionResults(): bool
498-
{
499-
return $this->consumeStoredExpressionResults;
500-
}
501-
502499
public function lookForSetAllowedUndefinedExpressions(MutatingScope $scope, Expr $expr): MutatingScope
503500
{
504501
return $this->lookForExpressionCallback($scope, $expr, static fn (MutatingScope $scope, Expr $expr): MutatingScope => $scope->setAllowedUndefinedExpression($expr));
@@ -544,22 +541,22 @@ private function lookForExpressionCallback(MutatingScope $scope, Expr $expr, Clo
544541
* are stored into the given storage - pass a duplicate to keep them isolated.
545542
*/
546543
/**
547-
* Processes an expression whose already-walked subtrees must be CONSUMED
548-
* from their stored results instead of re-walked: the nullsafe handlers
544+
* Processes an expression whose already-walked receiver must be CONSUMED
545+
* from its stored result instead of re-walked: the nullsafe handlers
549546
* process the receiver once (real callbacks) and then walk the plain twin,
550-
* whose receiver subtree answers from storage, re-anchored to the twin's
547+
* whose receiver answers from storage, re-anchored to the twin's
551548
* (ensured) scope.
552549
*
553550
* @param callable(Node $node, Scope $scope): void $nodeCallback
554551
*/
555-
public function processExprNodeConsumingStored(Node\Stmt $stmt, Expr $expr, MutatingScope $scope, ExpressionResultStorage $storage, callable $nodeCallback, ExpressionContext $context): ExpressionResult
552+
public function processExprNodeConsumingStored(Node\Stmt $stmt, Expr $expr, Expr $consumedExpr, MutatingScope $scope, ExpressionResultStorage $storage, callable $nodeCallback, ExpressionContext $context): ExpressionResult
556553
{
557-
$previous = $this->consumeStoredExpressionResults;
558-
$this->consumeStoredExpressionResults = true;
554+
$previous = $this->consumeStoredExpression;
555+
$this->consumeStoredExpression = $consumedExpr;
559556
try {
560557
return $this->processExprNode($stmt, $expr, $scope, $storage, $nodeCallback, $context);
561558
} finally {
562-
$this->consumeStoredExpressionResults = $previous;
559+
$this->consumeStoredExpression = $previous;
563560
}
564561
}
565562

@@ -775,7 +772,8 @@ public function processExprNode(
775772
ExpressionContext $context,
776773
): ExpressionResult
777774
{
778-
if ($this->returnStoredExpressionResults || $this->consumeStoredExpressionResults) {
775+
$consume = $this->consumeStoredExpression === $expr;
776+
if ($this->returnStoredExpressionResults || $consume) {
779777
$storedResult = $storage->findExpressionResult($expr);
780778
// a stored result only answers when the current scope agrees with its
781779
// evaluation position on the variables the expression reads - a
@@ -784,7 +782,7 @@ public function processExprNode(
784782
// re-processes the node on its own scope instead. In CONSUME mode the
785783
// divergence is intentional (an ensured-non-null device) and the
786784
// stored result is consumed unconditionally, re-anchored below.
787-
if ($storedResult !== null && ($this->consumeStoredExpressionResults || $storedResult->askScopeVariableStateMatches($scope, $scope->nativeTypesPromoted))) {
785+
if ($storedResult !== null && ($consume || $storedResult->askScopeVariableStateMatches($scope, $scope->nativeTypesPromoted))) {
788786
// a foreign-position answer must not thread its original walk
789787
// scopes into THIS walk - re-anchor it to the asking position so
790788
// subsequent operands keep evaluating on the asking scope
@@ -793,7 +791,7 @@ public function processExprNode(
793791
}
794792

795793
$reanchored = $storedResult->atAskPosition($scope);
796-
if ($this->consumeStoredExpressionResults) {
794+
if ($consume) {
797795
// the re-anchored view IS this walk's result for the node
798796
// (the nullsafe twin's receiver at the ensured position) -
799797
// store it so later asks (rules' storage reads) see the same

‎tests/PHPStan/Analyser/AnalyserIntegrationTest.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use function array_map;
1717
use function array_unique;
1818
use function array_values;
19+
use function count;
1920
use function extension_loaded;
2021
use function sprintf;
2122
use const PHP_VERSION_ID;
@@ -1807,6 +1808,31 @@ public function testBug15331(): void
18071808
$this->assertNoErrors($errors);
18081809
}
18091810

1811+
#[RequiresPhp('>= 8.0.0')]
1812+
public function testBug15520(): void
1813+
{
1814+
$errors = $this->runAnalyse(__DIR__ . '/data/bug-15520.php');
1815+
$expected = [
1816+
['Access to an undefined property Bug15520\\Presenter::$undefinedProperty.', 20],
1817+
['Binary operation "+" between 1 and \'x\' results in an error.', 20],
1818+
['Call to an undefined method Bug15520\\Presenter::undefinedMethod().', 20],
1819+
['Method Bug15520\\Category::imageFor() invoked with 2 parameters, 1 required.', 20],
1820+
['Call to an undefined method Bug15520\\Presenter::undefinedWhile().', 33],
1821+
['Call to an undefined method Bug15520\\Presenter::undefinedDoWhile().', 46],
1822+
['Call to an undefined method Bug15520\\Presenter::undefinedFor().', 59],
1823+
['Call to an undefined method Bug15520\\Presenter::undefinedInClosure().', 74],
1824+
['Call to an undefined method Bug15520\\Presenter::undefinedInArrowFunction().', 76],
1825+
['Call to an undefined method Bug15520\\Presenter::undefinedInPropertyName().', 90],
1826+
['Call to an undefined method Bug15520\\Presenter::undefinedInNested().', 104],
1827+
['Call to an undefined method Bug15520\\Presenter::undefinedInMethodName().', 105],
1828+
];
1829+
$this->assertCount(count($expected), $errors);
1830+
foreach ($expected as $i => [$message, $line]) {
1831+
$this->assertSame($message, $errors[$i]->getMessage());
1832+
$this->assertSame($line, $errors[$i]->getLine());
1833+
}
1834+
}
1835+
18101836
/**
18111837
* @param string[]|null $allAnalysedFiles
18121838
* @return list<Error>

‎tests/PHPStan/Analyser/data/bug-15520.php‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15520;
4+
5+
interface Category
6+
{
7+
public function imageFor(string $season): ?string;
8+
}
9+
10+
final class Presenter
11+
{
12+
/**
13+
* @param list<?Category> $categories
14+
* @return list<?string>
15+
*/
16+
public function images(array $categories): array
17+
{
18+
$images = [];
19+
foreach ($categories as $category) {
20+
$images[] = $category?->imageFor($this->undefinedMethod($this->undefinedProperty), 1 + 'x');
21+
}
22+
23+
return $images;
24+
}
25+
26+
/**
27+
* @return list<?string>
28+
*/
29+
public function whileLoop(?Category $category): array
30+
{
31+
$images = [];
32+
while (count($images) < 10) {
33+
$images[] = $category?->imageFor($this->undefinedWhile());
34+
}
35+
36+
return $images;
37+
}
38+
39+
/**
40+
* @return list<?string>
41+
*/
42+
public function doWhileLoop(?Category $category): array
43+
{
44+
$images = [];
45+
do {
46+
$images[] = $category?->imageFor($this->undefinedDoWhile());
47+
} while (count($images) < 10);
48+
49+
return $images;
50+
}
51+
52+
/**
53+
* @return list<?string>
54+
*/
55+
public function forLoop(?Category $category): array
56+
{
57+
$images = [];
58+
for ($i = 0; $i < 10; $i++) {
59+
$images[] = $category?->imageFor($this->undefinedFor());
60+
}
61+
62+
return $images;
63+
}
64+
65+
/**
66+
* @param list<?Category> $categories
67+
* @return list<?string>
68+
*/
69+
public function closureArgument(array $categories): array
70+
{
71+
$images = [];
72+
foreach ($categories as $category) {
73+
$images[] = $category?->imageFor((function (): string {
74+
return $this->undefinedInClosure();
75+
})());
76+
$images[] = $category?->imageFor((fn (): string => $this->undefinedInArrowFunction())());
77+
}
78+
79+
return $images;
80+
}
81+
82+
/**
83+
* @param list<?object> $objects
84+
* @return list<mixed>
85+
*/
86+
public function nullsafePropertyFetch(array $objects, string $name): array
87+
{
88+
$values = [];
89+
foreach ($objects as $object) {
90+
$values[] = $object?->{$name . $this->undefinedInPropertyName()};
91+
}
92+
93+
return $values;
94+
}
95+
96+
/**
97+
* @param list<?Category> $categories
98+
* @return list<?string>
99+
*/
100+
public function nestedAndChained(array $categories, string $method): array
101+
{
102+
$images = [];
103+
foreach ($categories as $category) {
104+
$images[] = $category?->imageFor($category?->imageFor($this->undefinedInNested()) ?? '');
105+
$images[] = $category?->{$method . $this->undefinedInMethodName()}();
106+
}
107+
108+
return $images;
109+
}
110+
}

‎tests/PHPStan/Rules/DeadCode/UnusedPrivatePropertyRuleTest.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,15 @@ public function testBug14703(): void
475475
$this->analyse([__DIR__ . '/data/bug-14703.php'], []);
476476
}
477477

478+
#[RequiresPhp('>= 8.0.0')]
479+
public function testBug15520(): void
480+
{
481+
$this->alwaysWrittenTags = [];
482+
$this->alwaysReadTags = [];
483+
484+
$this->analyse([__DIR__ . '/data/bug-15520.php'], []);
485+
}
486+
478487
public function testBug14880(): void
479488
{
480489
$this->alwaysWrittenTags = [];

‎tests/PHPStan/Rules/DeadCode/data/bug-15520.php‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
<?php declare(strict_types = 1); // lint >= 8.0
2+
3+
namespace Bug15520Property;
4+
5+
interface SeasonResolver
6+
{
7+
public function current(): string;
8+
}
9+
10+
interface Category
11+
{
12+
public function imageFor(string $season): ?string;
13+
}
14+
15+
final class Presenter
16+
{
17+
public function __construct(
18+
private SeasonResolver $seasonResolver,
19+
)
20+
{
21+
}
22+
23+
/**
24+
* @param list<?Category> $categories
25+
* @return list<?string>
26+
*/
27+
public function images(array $categories): array
28+
{
29+
$images = [];
30+
foreach ($categories as $category) {
31+
$images[] = $category?->imageFor($this->seasonResolver->current());
32+
}
33+
34+
return $images;
35+
}
36+
}
37+
38+
final class WhilePresenter
39+
{
40+
public function __construct(
41+
private SeasonResolver $seasonResolver,
42+
)
43+
{
44+
}
45+
46+
/**
47+
* @return list<?string>
48+
*/
49+
public function images(?Category $category): array
50+
{
51+
$images = [];
52+
while (count($images) < 10) {
53+
$images[] = $category?->imageFor($this->seasonResolver->current());
54+
}
55+
56+
return $images;
57+
}
58+
}
59+
60+
final class PropertyNamePresenter
61+
{
62+
public function __construct(
63+
private string $name,
64+
)
65+
{
66+
}
67+
68+
/**
69+
* @param list<?object> $objects
70+
* @return list<mixed>
71+
*/
72+
public function values(array $objects): array
73+
{
74+
$values = [];
75+
foreach ($objects as $object) {
76+
$values[] = $object?->{$this->name};
77+
}
78+
79+
return $values;
80+
}
81+
}

0 commit comments

Comments
 (0)