Skip to content

Commit 08e3736

Browse files
ondrejmirtesclaude
andcommitted
Pin the closure node in ClosureTypeResolver's object-id keyed cache
PHP hands out the spl_object_id() of a freed object again. The parsed file's nodes live for its whole analysis, but a closure node that is built or parsed and dropped mid-file - by a handler desugaring a call, or by an extension or a test walking its own nodes through NodeScopeResolver - frees its id, and the next closure allocated may get it. With an equal context key the cache then answered the freed node's types for an unrelated closure: that is what flaked ExpressionResultTest's `(fn() => exit())();` on the 7.4 and 8.0 jobs, where an earlier data set's `fn() => yield (exit());` left a Generator<int, never, mixed, void> return type behind and the immediately invoked arrow function read it before walking its own body. Each entry now pins the node it was built for and answers for that very node only. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKj5Zgji9eL5MoSuaTEJxF
1 parent 4baf513 commit 08e3736

2 files changed

Lines changed: 57 additions & 15 deletions

File tree

‎src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,15 @@ final class ClosureTypeResolver implements PerFileAnalysisResettable
7272
* file's analysis ends - the per-file reset releases the Types and
7373
* throw/impure points with the rest of the file's result graph.
7474
*
75-
* Keyed by the closure node's spl_object_id(). The keys are AST nodes that
76-
* live for the whole file's analysis (the parser cache retains them), so
77-
* ids of live entries never collide; the per-file reset empties the map
78-
* before another file could reuse them.
75+
* Keyed by the closure node's spl_object_id(), which PHP hands out again
76+
* as soon as a node is freed. The parsed file's nodes live for its whole
77+
* analysis, but a closure node that is built or parsed and dropped mid-file
78+
* - by a handler desugaring a call, or by an extension or a test walking
79+
* its own nodes through NodeScopeResolver - frees its id for the next
80+
* allocation. Each entry therefore pins the node it was built for, and
81+
* findCachedTypes() answers for that very node only.
7982
*
80-
* @var array<int, array<string, array{returnType: Type, throwPoints: SimpleThrowPoint[], impurePoints: SimpleImpurePoint[], invalidateExpressions: InvalidateExprNode[], usedVariables: string[]}>>
83+
* @var array<int, array{expr: Node\Expr\Closure|ArrowFunction, types: array<string, array{returnType: Type, throwPoints: SimpleThrowPoint[], impurePoints: SimpleImpurePoint[], invalidateExpressions: InvalidateExprNode[], usedVariables: string[]}>}>
8184
*/
8285
private array $cachedTypes = [];
8386

@@ -93,6 +96,19 @@ public function resetFileAnalysisState(): void
9396
$this->cachedTypes = [];
9497
}
9598

99+
/**
100+
* @return array<string, array{returnType: Type, throwPoints: SimpleThrowPoint[], impurePoints: SimpleImpurePoint[], invalidateExpressions: InvalidateExprNode[], usedVariables: string[]}>
101+
*/
102+
private function findCachedTypes(Node\Expr\Closure|ArrowFunction $expr): array
103+
{
104+
$entry = $this->cachedTypes[spl_object_id($expr)] ?? null;
105+
if ($entry === null || $entry['expr'] !== $expr) {
106+
return [];
107+
}
108+
109+
return $entry['types'];
110+
}
111+
96112
/**
97113
* Resolves a closure/arrow function type by walking its body itself. Used by
98114
* the paths that have no prior walk to read return/yield types from. A
@@ -128,7 +144,7 @@ public function getClosureType(
128144
);
129145
}
130146

131-
$cachedTypes = $this->cachedTypes[spl_object_id($expr)] ?? [];
147+
$cachedTypes = $this->findCachedTypes($expr);
132148
$cacheKey = $this->closureContextCacheKey($scope, $expr, $callableParameters, $parameters);
133149
if (array_key_exists($cacheKey, $cachedTypes)) {
134150
return $this->createClosureTypeFromCache($expr, $parameters, $isVariadic, $cachedTypes[$cacheKey]);
@@ -387,15 +403,15 @@ public function seedCacheFromClosureWalk(MutatingScope $scope, Node\Expr\Closure
387403

388404
[$parameters, , $callableParameters] = $this->buildParametersAndAcceptors($scope, $expr);
389405
$phpdocKey = $this->closureContextCacheKey($scope, $expr, $callableParameters, $parameters);
390-
$cachedTypes = $this->cachedTypes[spl_object_id($expr)] ?? [];
406+
$cachedTypes = $this->findCachedTypes($expr);
391407
if (!array_key_exists($phpdocKey, $cachedTypes)) {
392408
return;
393409
}
394410

395411
$promotedScope = $scope->doNotTreatPhpDocTypesAsCertain();
396412
[$promotedParameters, , $promotedCallableParameters] = $this->buildParametersAndAcceptors($promotedScope, $expr);
397413
$cachedTypes[$this->closureContextCacheKey($promotedScope, $expr, $promotedCallableParameters, $promotedParameters)] = $cachedTypes[$phpdocKey];
398-
$this->cachedTypes[spl_object_id($expr)] = $cachedTypes;
414+
$this->cachedTypes[spl_object_id($expr)] = ['expr' => $expr, 'types' => $cachedTypes];
399415
}
400416

401417
/**
@@ -912,7 +928,7 @@ private function assembleClosureType(
912928
$impurePointsForClosureType = array_map(static fn (ImpurePoint $impurePoint) => new SimpleImpurePoint($impurePoint->getIdentifier(), $impurePoint->getDescription(), $impurePoint->isCertain()), $impurePoints);
913929

914930
if ($writeCache) {
915-
$cachedTypes = $this->cachedTypes[spl_object_id($expr)] ?? [];
931+
$cachedTypes = $this->findCachedTypes($expr);
916932
$cacheKey ??= $this->closureContextCacheKey($scope, $expr, null, $parameters);
917933
$cachedTypes[$cacheKey] = [
918934
'returnType' => $returnType,
@@ -921,7 +937,7 @@ private function assembleClosureType(
921937
'invalidateExpressions' => $invalidateExpressions,
922938
'usedVariables' => $usedVariables,
923939
];
924-
$this->cachedTypes[spl_object_id($expr)] = $cachedTypes;
940+
$this->cachedTypes[spl_object_id($expr)] = ['expr' => $expr, 'types' => $cachedTypes];
925941
}
926942

927943
$mustUseReturnValue = TrinaryLogic::createNo();

‎tests/PHPStan/Analyser/ExpressionResultTest.php‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
namespace PHPStan\Analyser;
44

5+
use PhpParser\Node\Expr\ArrowFunction;
6+
use PhpParser\Node\Expr\Exit_;
7+
use PhpParser\Node\Expr\FuncCall;
58
use PhpParser\Node\Stmt;
69
use PHPStan\Parser\Parser;
710
use PHPStan\ShouldNotHappenException;
@@ -193,9 +196,33 @@ public function testIsAlwaysTerminating(
193196
if (!$stmts[0] instanceof Stmt\Expression) {
194197
throw new ShouldNotHappenException('Expecting code contains a single statement expression, got: ' . get_class($stmts[0]));
195198
}
196-
$stmt = $stmts[0];
197-
$expr = $stmt->expr;
199+
$this->assertSame($expectedIsAlwaysTerminating, $this->processExpressionStatement($stmts[0])->isAlwaysTerminating());
200+
}
201+
202+
public function testClosureTypeIsNotReusedForClosureWithRecycledObjectId(): void
203+
{
204+
/** @var Parser $parser */
205+
$parser = self::getContainer()->getService('currentPhpVersionRichParser');
198206

207+
/** @var Stmt\Expression $generatorStmt */
208+
$generatorStmt = $parser->parseString('<?php fn() => yield (exit());')[0];
209+
$generatorArrowFunction = $generatorStmt->expr;
210+
$this->assertInstanceOf(ArrowFunction::class, $generatorArrowFunction);
211+
$this->assertFalse($this->processExpressionStatement($generatorStmt)->isAlwaysTerminating());
212+
213+
// PHP hands the most recently freed object id to the next allocation - drop the
214+
// generator arrow function last, and build the next arrow function right after,
215+
// so it gets the same spl_object_id() unless something still holds the first one
216+
$exit = new Exit_();
217+
unset($generatorStmt);
218+
unset($generatorArrowFunction);
219+
$arrowFunction = new ArrowFunction(['expr' => $exit], ['isImmediatelyInvokedClosure' => true, 'immediatelyInvokedClosureArgs' => []]);
220+
221+
$this->assertTrue($this->processExpressionStatement(new Stmt\Expression(new FuncCall($arrowFunction)))->isAlwaysTerminating());
222+
}
223+
224+
private function processExpressionStatement(Stmt\Expression $stmt): ExpressionResult
225+
{
199226
/** @var NodeScopeResolver $nodeScopeResolver */
200227
$nodeScopeResolver = self::getContainer()->getByType(NodeScopeResolver::class);
201228
/** @var ScopeFactory $scopeFactory */
@@ -204,16 +231,15 @@ public function testIsAlwaysTerminating(
204231
->assignVariable('x', new IntegerType(), new IntegerType(), TrinaryLogic::createYes())
205232
->assignVariable('arr', new ArrayType(new MixedType(), new MixedType()), new ArrayType(new MixedType(), new MixedType()), TrinaryLogic::createYes());
206233

207-
$result = $nodeScopeResolver->processExprNode(
234+
return $nodeScopeResolver->processExprNode(
208235
$stmt,
209-
$expr,
236+
$stmt->expr,
210237
$scope,
211238
new ExpressionResultStorage(),
212239
static function (): void {
213240
},
214241
ExpressionContext::createTopLevel(),
215242
);
216-
$this->assertSame($expectedIsAlwaysTerminating, $result->isAlwaysTerminating());
217243
}
218244

219245
}

0 commit comments

Comments
 (0)