Skip to content

Commit

Permalink
Merge pull request #9955 from tscni/fix/class-property-cached-method-…
Browse files Browse the repository at this point in the history 
…invalidation

Invalidate cached methods when referenced class property types change
  • Loading branch information
@orklah
orklah committed Jun 24, 2023
2 parents 81946bb + a737ca8 commit b9a7c37
Show file tree
Hide file tree
Showing 12 changed files with 176 additions and 42 deletions.
9 changes: 9 additions & 0 deletions src/Psalm/Internal/Analyzer/ProjectAnalyzer.php
View file
Expand Up @@ -1098,6 +1098,15 @@ public function checkPaths(array $paths_to_check): void
}
}

public function finish(float $start_time, string $psalm_version): void
{
$this->codebase->file_reference_provider->removeDeletedFilesFromReferences();

if ($this->project_cache_provider) {
$this->project_cache_provider->processSuccessfulRun($start_time, $psalm_version);
}
}

public function getConfig(): Config
{
return $this->config;
Expand Down
16 changes: 16 additions & 0 deletions src/Psalm/Internal/Diff/ClassStatementsDiffer.php
View file
Expand Up @@ -158,6 +158,22 @@ static function (
return false;
}

if ($a->type xor $b->type) {
return false;
}

if ($a->type && $b->type) {
$a_type_start = (int) $a->type->getAttribute('startFilePos');
$a_type_end = (int) $a->type->getAttribute('endFilePos');
$b_type_start = (int) $b->type->getAttribute('startFilePos');
$b_type_end = (int) $b->type->getAttribute('endFilePos');
if (substr($a_code, $a_type_start, $a_type_end - $a_type_start + 1)
!== substr($b_code, $b_type_start, $b_type_end - $b_type_start + 1)
) {
return false;
}
}

$body_change = substr($a_code, $a_comments_end, $a_end - $a_comments_end)
!== substr($b_code, $b_comments_end, $b_end - $b_comments_end);
} else {
Expand Down
24 changes: 9 additions & 15 deletions src/Psalm/Internal/LanguageServer/Provider/FileStorageCacheProvider.php
View file
Expand Up @@ -19,30 +19,24 @@ public function __construct()
{
}

public function writeToCache(FileStorage $storage, string $file_contents): void
/**
* @param lowercase-string $file_path
*/
protected function storeInCache(string $file_path, FileStorage $storage): void
{
$file_path = strtolower($storage->file_path);
$this->cache[$file_path] = $storage;
}

public function getLatestFromCache(string $file_path, string $file_contents): ?FileStorage
{
$cached_value = $this->loadFromCache(strtolower($file_path));

if (!$cached_value) {
return null;
}

return $cached_value;
}

public function removeCacheForFile(string $file_path): void
{
unset($this->cache[strtolower($file_path)]);
}

private function loadFromCache(string $file_path): ?FileStorage
/**
* @param lowercase-string $file_path
*/
protected function loadFromCache(string $file_path): ?FileStorage
{
return $this->cache[strtolower($file_path)] ?? null;
return $this->cache[$file_path] ?? null;
}
}
7 changes: 4 additions & 3 deletions src/Psalm/Internal/Provider/FileReferenceProvider.php
View file
Expand Up @@ -14,7 +14,6 @@
use function array_merge;
use function array_unique;
use function explode;
use function file_exists;

/**
* Used to determine which files reference other files, necessary for using the --diff
Expand Down Expand Up @@ -164,10 +163,12 @@ class FileReferenceProvider
*/
private static array $method_param_uses = [];

private FileProvider $file_provider;
public ?FileReferenceCacheProvider $cache = null;

public function __construct(?FileReferenceCacheProvider $cache = null)
public function __construct(FileProvider $file_provider, ?FileReferenceCacheProvider $cache = null)
{
$this->file_provider = $file_provider;
$this->cache = $cache;
}

Expand All @@ -179,7 +180,7 @@ public function getDeletedReferencedFiles(): array
if (self::$deleted_files === null) {
self::$deleted_files = array_filter(
array_keys(self::$file_references),
static fn(string $file_name): bool => !file_exists($file_name)
fn(string $file_name): bool => !$this->file_provider->fileExists($file_name)
);
}

Expand Down
15 changes: 13 additions & 2 deletions src/Psalm/Internal/Provider/FileStorageCacheProvider.php
View file
Expand Up @@ -64,9 +64,17 @@ public function __construct(Config $config)
public function writeToCache(FileStorage $storage, string $file_contents): void
{
$file_path = strtolower($storage->file_path);
$cache_location = $this->getCacheLocationForPath($file_path, true);
$storage->hash = $this->getCacheHash($file_path, $file_contents);

$this->storeInCache($file_path, $storage);
}

/**
* @param lowercase-string $file_path
*/
protected function storeInCache(string $file_path, FileStorage $storage): void
{
$cache_location = $this->getCacheLocationForPath($file_path, true);
$this->cache->saveItem($cache_location, $storage);
}

Expand Down Expand Up @@ -107,7 +115,10 @@ private function getCacheHash(string $_unused_file_path, string $file_contents):
return PHP_VERSION_ID >= 8_01_00 ? hash('xxh128', $data) : hash('md4', $data);
}

private function loadFromCache(string $file_path): ?FileStorage
/**
* @param lowercase-string $file_path
*/
protected function loadFromCache(string $file_path): ?FileStorage
{
$storage = $this->cache->getItem($this->getCacheLocationForPath($file_path));
if ($storage instanceof FileStorage) {
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Provider/Providers.php
View file
Expand Up @@ -51,7 +51,7 @@ public function __construct(
$parser_cache_provider,
$file_storage_cache_provider,
);
$this->file_reference_provider = new FileReferenceProvider($file_reference_cache_provider);
$this->file_reference_provider = new FileReferenceProvider($file_provider, $file_reference_cache_provider);
}

public static function safeFileGetContents(string $path): string
Expand Down
6 changes: 1 addition & 5 deletions src/Psalm/IssueBuffer.php
View file
Expand Up @@ -792,11 +792,7 @@ public static function finish(
}

if ($is_full && $start_time) {
$codebase->file_reference_provider->removeDeletedFilesFromReferences();

if ($project_analyzer->project_cache_provider) {
$project_analyzer->project_cache_provider->processSuccessfulRun($start_time, PSALM_VERSION);
}
$project_analyzer->finish($start_time, PSALM_VERSION);
}

if ($error_count
Expand Down
44 changes: 44 additions & 0 deletions tests/Cache/CacheTest.php
View file
Expand Up @@ -19,6 +19,7 @@
use Psalm\Tests\Internal\Provider\ProjectCacheProvider;
use Psalm\Tests\TestCase;

use function microtime;
use function str_replace;

use const DIRECTORY_SEPARATOR;
Expand Down Expand Up @@ -101,8 +102,10 @@ public function testCacheInteractions(

RuntimeCaches::clearAll();

$start_time = microtime(true);
$project_analyzer = new ProjectAnalyzer($config, $providers);
$project_analyzer->check($config->base_dir, true);
$project_analyzer->finish($start_time, PSALM_VERSION);

$issues = self::normalizeIssueData(IssueBuffer::getIssuesData());
self::assertSame($interaction['issues'] ?? [], $issues);
Expand Down Expand Up @@ -155,5 +158,46 @@ public function do(): void
],
],
];

yield 'classPropertyTypeChangeInvalidatesReferencingMethod' => [
[
[
'files' => [
'/src/A.php' => <<<'PHP'
<?php
class A {
public function foo(B $b): int
{
return $b->value;
}
}
PHP,
'/src/B.php' => <<<'PHP'
<?php
class B {
public ?int $value = 0;
}
PHP,
],
'issues' => [
'/src/A.php' => [
"NullableReturnStatement: The declared return type 'int' for A::foo is not nullable, but the function returns 'int|null'",
"InvalidNullableReturnType: The declared return type 'int' for A::foo is not nullable, but 'int|null' contains null",
],
],
],
[
'files' => [
'/src/B.php' => <<<'PHP'
<?php
class B {
public int $value = 0;
}
PHP,
],
'issues' => [],
],
],
];
}
}
57 changes: 57 additions & 0 deletions tests/FileDiffTest.php
View file
Expand Up @@ -544,6 +544,63 @@ class A {
[],
[[84, 133]],
],
'propertyTypeAddition' => [
'<?php
namespace Foo;
class A {
public $a;
}',
'<?php
namespace Foo;
class A {
public string $a;
}',
[],
[],
['foo\a::$a', 'foo\a::$a'],
[],
[[84, 93]],
],
'propertyTypeRemoval' => [
'<?php
namespace Foo;
class A {
public string $a;
}',
'<?php
namespace Foo;
class A {
public $a;
}',
[],
[],
['foo\a::$a', 'foo\a::$a'],
[],
[[84, 100]],
],
'propertyTypeChange' => [
'<?php
namespace Foo;
class A {
public string $a;
}',
'<?php
namespace Foo;
class A {
public ?string $a;
}',
[],
[],
['foo\a::$a', 'foo\a::$a'],
[],
[[84, 100]],
],
'addDocblockToFirst' => [
'<?php
namespace Foo;
Expand Down
12 changes: 12 additions & 0 deletions tests/Internal/Provider/FakeFileReferenceCacheProvider.php
View file
Expand Up @@ -58,6 +58,9 @@ class FakeFileReferenceCacheProvider extends FileReferenceCacheProvider
*/
private array $cached_file_maps = [];

/** @var array<string, array{int, int}> */
private array $cached_type_coverage = [];

public function __construct()
{
parent::__construct(Config::getInstance());
Expand Down Expand Up @@ -269,10 +272,19 @@ public function setFileMapCache(array $file_maps): void
$this->cached_file_maps = $file_maps;
}

/**
* @return array<string, array{int, int}>
*/
public function getTypeCoverage(): array
{
return $this->cached_type_coverage;
}

/**
* @param array<string, array{int, int}> $mixed_counts
*/
public function setTypeCoverage(array $mixed_counts): void
{
$this->cached_type_coverage = $mixed_counts;
}
}
24 changes: 9 additions & 15 deletions tests/Internal/Provider/FileStorageInstanceCacheProvider.php
View file
Expand Up @@ -16,30 +16,24 @@ public function __construct()
{
}

public function writeToCache(FileStorage $storage, string $file_contents): void
/**
* @param lowercase-string $file_path
*/
protected function storeInCache(string $file_path, FileStorage $storage): void
{
$file_path = strtolower($storage->file_path);
$this->cache[$file_path] = $storage;
}

public function getLatestFromCache(string $file_path, string $file_contents): ?FileStorage
{
$cached_value = $this->loadFromCache(strtolower($file_path));

if (!$cached_value) {
return null;
}

return $cached_value;
}

public function removeCacheForFile(string $file_path): void
{
unset($this->cache[strtolower($file_path)]);
}

private function loadFromCache(string $file_path): ?FileStorage
/**
* @param lowercase-string $file_path
*/
protected function loadFromCache(string $file_path): ?FileStorage
{
return $this->cache[strtolower($file_path)] ?? null;
return $this->cache[$file_path] ?? null;
}
}
2 changes: 1 addition & 1 deletion tests/ProjectCheckerTest.php
View file
Expand Up @@ -188,7 +188,7 @@ public function testCheckAfterNoChange(): void
$this->assertSame(0, IssueBuffer::getErrorCount());

$this->assertSame(
'Psalm was able to infer types for 100% of the codebase',
"No files analyzed\nPsalm was able to infer types for 100% of the codebase",
$this->project_analyzer->getCodebase()->analyzer->getTypeInferenceSummary(
$this->project_analyzer->getCodebase(),
),
Expand Down

0 comments on commit b9a7c37

Please sign in to comment.