Skip to content

Re-resolve ShortNameResolver short names on stmts change - #8697

Merged
TomasVotruba merged 3 commits into
mainfrom
tv-shortname-version-cache
Oct 17, 2026
Merged

TomasVotruba merged 3 commits into
mainfrom
tv-shortname-version-cache

Conversation

@TomasVotruba

@TomasVotruba TomasVotruba commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

What

ShortNameResolver::resolveFromFile() cached its short-name-to-FQN map by file path and never invalidated it. Across the file-processing convergence loop the stmts change between iterations, so later iterations read a stale map - a correctness bug - and the map for every processed file was retained for the whole run.

Change

resolveFromFile() now caches by file identity + getNewStmtsVersion() and re-resolves when the stmts version bumps - the same pattern resolveShortClassLikeNames() already uses in this class. The per-file-path array is dropped, so only the current file's map is held.

Proof the bug is real

ShortNameResolverTest::testResolveFromFileAgainAfterStmtsChange resolves a file, renames its class, bumps the stmts version, and resolves again. It mirrors the existing testResolveShortClassLikeNamesAgainAfterStmtsChange guard for the sibling method.

  • on the old filePath cache it fails: Failed asserting that an array does not have the key 'VariousImports' (stale map returned after the change)
  • with this fix it passes

Tradeoff - correctness + memory, costs some CPU

Measured with bench/short_name_resolver_bench.php (8 convergence passes, 400 node queries each, 25 repeats):

pattern before (filePath cache) after (version cache)
single-version 0.51s cpu 0.58s cpu
multi-version (stmts bumped each pass) 0.50s cpu 1.05s cpu

The old cache ignored version bumps, so convergence passes were free; the new one re-resolves per version (~2x cpu on the multi-version pattern). The cost only hits files that actually change across iterations, scaling with iteration count. In return the short-name map is correct on every iteration and only one file's map is held at a time.

Tests

148 import tests green, plus the new regression test. ECS + full PHPStan clean. Benchmark lives in bench/ (not scanned by ECS/PHPStan/Rector).

@TomasVotruba
TomasVotruba marked this pull request as ready for review October 17, 2026 20:15
@TomasVotruba
TomasVotruba merged commit 66a13fd into main Oct 17, 2026
49 checks passed
@TomasVotruba
TomasVotruba deleted the tv-shortname-version-cache branch October 17, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant