Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Invalidate cached methods when referenced class property types change #9955

Merged
merged 4 commits into from Jun 24, 2023
Merged

Invalidate cached methods when referenced class property types change #9955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9939cae
Support project analysis with file diffing in tests
tscni Jun 24, 2023
1b1915e
Make file storage cache handling consistent
tscni Jun 24, 2023
703a4b4
Add test case for property type change not invalidating methods
tscni Jun 24, 2023
a737ca8
Consider a property type change as a signature change
tscni Jun 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/Psalm/Internal/Analyzer/ProjectAnalyzer.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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