Skip to content

Commit 02f74b1

Browse files
Sander Mullerclaude
authored andcommitted
Discard the cache when the cached results cannot be reconstructed
unserialize() has no reconstruction hook, so a cache written by a PHPStan whose classes have since changed can fail while the objects are rebuilt: a property the payload does not carry stays uninitialized and reading it throws. var_export absorbed this because __set_state() reconstructs through the constructor, which applies the declared defaults - renaming Error::$tip is harmless there and aborted the run here. cacheVersion and phpstanVersion keep a released version away from another release's objects, but phpstanVersion comes from Composer's installed.php rather than the working tree, so a source checkout keeps one value across every edit of these classes. That is where it is reachable, and it is also where PHPStan is developed. The four callbacks are now invoked inside restore()'s failure path, so a cache that cannot be read back is discarded and everything re-analysed, the same as for a damaged file. Before this the same shape aborted the command with an uncaught error and a usage dump - on both formats, since the var_export closures are evaluated at the same point. e2e/result-cache-stale-objects renames Error::$message inside the cache file, which is that shape without needing two PHPStan versions installed, keeping the byte length so the frame headers stay valid. The run reports the cause and re-analyses; without the change it dies on the uninitialized property. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 3e35810 commit 02f74b1

7 files changed

Lines changed: 91 additions & 4 deletions

File tree

.github/workflows/e2e-tests.yml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,22 @@ jobs:
179179
OUTPUT=$(../../bin/phpstan -vvv 2>&1)
180180
echo "$OUTPUT"
181181
../bashunit -a contains 'Result cache restored.' "$OUTPUT"
182+
- script: |
183+
cd e2e/result-cache-stale-objects
184+
OUTPUT=$(../bashunit -a exit_code "1" "../../bin/phpstan -vvv")
185+
echo "$OUTPUT"
186+
# Renaming a property inside the cache file is what a cache written by a PHPStan whose
187+
# classes have changed since looks like: the payload does not carry the property the class
188+
# now declares, so reconstructing the object leaves it uninitialized and reading it throws.
189+
php rename-property.php
190+
OUTPUT=$(../bashunit -a exit_code "1" "../../bin/phpstan -vvv")
191+
echo "$OUTPUT"
192+
../bashunit -a contains 'could not be read back' "$OUTPUT"
193+
../bashunit -a contains 'Result cache is saved.' "$OUTPUT"
194+
# The cache written in its place is usable again.
195+
OUTPUT=$(../bashunit -a exit_code "1" "../../bin/phpstan -vvv")
196+
echo "$OUTPUT"
197+
../bashunit -a contains 'Result cache restored.' "$OUTPUT"
182198
- script: |
183199
cd e2e/bug-14514
184200
composer install

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,5 @@ e2e/bashunit
2020
/e2e/result-cache-atomic-save/tmp
2121
/e2e/result-cache-moved-tmpdir/tmp-a
2222
/e2e/result-cache-moved-tmpdir/tmp-b
23+
/e2e/result-cache-stale-objects/tmp
2324
/e2e/result-cache-truncated/tmp
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
parameters:
2+
level: 8
3+
tmpDir: tmp
4+
paths:
5+
- src
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<?php declare(strict_types = 1);
2+
3+
// A cache written by a PHPStan whose classes have changed since cannot be reconstructed: the payload
4+
// does not carry the property the class now declares, so the object comes back with it uninitialized
5+
// and reading it throws. Renaming one inside the cache file is that shape without needing two
6+
// PHPStan versions installed, and keeping the byte length identical leaves the rest of the payload
7+
// valid - the frame headers count bytes.
8+
$file = __DIR__ . '/tmp/resultCache.php';
9+
$contents = file_get_contents($file);
10+
if ($contents === false) {
11+
throw new RuntimeException('No result cache at ' . $file);
12+
}
13+
14+
// Error::$message, read by transformPaths() while the cached errors are absolutized.
15+
$property = "\0PHPStan\\Analyser\\Error\0message";
16+
if (substr_count($contents, $property) !== 1) {
17+
throw new RuntimeException('Expected exactly one cached Error carrying that property.');
18+
}
19+
20+
$renamed = substr($property, 0, -strlen('message')) . 'messagf';
21+
if (strlen($renamed) !== strlen($property)) {
22+
throw new RuntimeException('The replacement has to keep the byte length.');
23+
}
24+
25+
file_put_contents($file, str_replace($property, $renamed, $contents));
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ResultCacheE2EStaleObjects;
4+
5+
class Foo
6+
{
7+
8+
public function doFoo(): int
9+
{
10+
return 'not an int';
11+
}
12+
13+
}
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
*
2+
!.gitignore

src/Analyser/ResultCache/ResultCacheManager.php

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,12 @@ final class ResultCacheManager
134134
* Sections restore() hands back as callbacks instead of arrays, so a run that never asks for them
135135
* never pays for decoding them. Each is a whole array frame in the file, so the reader only has to
136136
* remember where it starts and walk past its entries.
137+
*
138+
* Nothing closure-shaped is written: a frame holds the plain serialized payload
139+
* (`a:1:{s:9:"file.php";a:1:{i:0;O:22:"PHPStan\Analyser\Error"...`), and readCacheFile() builds
140+
* the callback in PHP around the open handle and that offset, which is only the shape restore()
141+
* expects. The var_export format writes a real `static function (): array` into the file instead,
142+
* and PHP still compiles the array literal inside it.
137143
*/
138144
private const LAZY_SECTIONS = ['errors', 'locallyIgnoredErrors', 'collectedData', 'exportedNodes'];
139145

@@ -593,12 +599,31 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ?
593599
$filesToAnalyse = [];
594600
$invertedDependenciesToReturn = [];
595601
$invertedUsedTraitDependenciesToReturn = [];
596-
$errors = $data['errorsCallback']();
597-
$locallyIgnoredErrors = $data['locallyIgnoredErrorsCallback']();
598602
$linesToIgnore = $data['linesToIgnore'];
599603
$unmatchedLineIgnores = $data['unmatchedLineIgnores'];
600-
$collectedData = $data['collectedDataCallback']();
601-
$exportedNodes = $data['exportedNodesCallback']();
604+
605+
try {
606+
// The cached objects are reconstructed here, and a cache written by a PHPStan whose classes
607+
// have since changed can fail at it: a property the payload does not carry stays
608+
// uninitialized, and reading it throws. The cacheVersion and phpstanVersion in the metadata
609+
// keep a released version away from another release's objects, but a source checkout keeps
610+
// one phpstanVersion across every edit of these classes, so this is reachable there. A cache
611+
// that cannot be reconstructed is discarded like any other unusable one.
612+
$errors = $data['errorsCallback']();
613+
$locallyIgnoredErrors = $data['locallyIgnoredErrorsCallback']();
614+
$collectedData = $data['collectedDataCallback']();
615+
$exportedNodes = $data['exportedNodesCallback']();
616+
} catch (Throwable $e) {
617+
@unlink($cacheFilePath);
618+
619+
return $this->fullAnalysis(
620+
sprintf('Result cache not used because the cached results could not be read back: %s', $e->getMessage()),
621+
$allAnalysedFiles,
622+
$meta,
623+
$currentFileHashes,
624+
$output,
625+
);
626+
}
602627
$filteredErrors = [];
603628
$filteredLocallyIgnoredErrors = [];
604629
$filteredLinesToIgnore = [];

0 commit comments

Comments
 (0)