Skip to content

ADFA-5231: Pin the live KtFile for the duration of an analysis - #1745

Open
itsaky-adfa wants to merge 4 commits into
ADFA-5231-pin-apifrom
ADFA-5231-migrate-call-sites
Open

ADFA-5231: Pin the live KtFile for the duration of an analysis#1745
itsaky-adfa wants to merge 4 commits into
ADFA-5231-pin-apifrom
ADFA-5231-migrate-call-sites

Conversation

@itsaky-adfa

@itsaky-adfa itsaky-adfa commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Stack 3 of 5 for ADFA-5231. This is the layer that fixes the ticket.

Migrates every acquisition site onto the pinned scope from the previous PR, then makes the raw accessors private so an unpinned analysis stops compiling.

Review by commit

Two commits, and the order matters:

  1. add failing test for stale KtFile instance redeclarations - lands red on purpose, as the bisectable record of the regression. It fails with the ticket's own errors: INVISIBLE_REFERENCE: Cannot access 'fun extracted(...)': it is private in file. and CONFLICTING_OVERLOADS: Conflicting overloads: fun render(a: Int, b: Int): Int.
  2. pin the live KtFile for the duration of an analysis - turns it green.

The PR tip is green; only the intermediate commit is red.

The migration

Every site had the same shape - getCurrentKtFile(path).get()/.await(), then project.read { ... analyzeMaybeDangling(ktFile, ...) { ... } } - and becomes a withLiveKtFile(path) { live -> ... } scope using live.read / live.analyzing. It is one transformation repeated 13 times, so it reads faster than the line count suggests. Worth actual attention:

  • KotlinCompletions is the intricate one: its dangling placeholder copy now goes through analyzingVariant, which wires originalFile/originalKtFile to the pinned instance so callers cannot forget to.
  • GoToDefinition, FindUsages, OrganizeImportsAction, ImplementMembersAction take the scope inside retryingOnPreemption, so each attempt pins afresh - the existing "awaited per attempt, not once" reasoning still holds and that comment is kept.
  • FindUsages.ktFileFor is deleted; the scope handles the open and on-disk cases itself.
  • KotlinDiagnosticProvider now drops a superseded result via live.isStale and reschedules instead of publishing diagnostics for text the user has already replaced.
  • AdvancedKotlinEditHandler is the single production caller that legitimately needs an unpinned peek - UI thread, PSI only, no analysis session - so it sits behind @OptIn(UnpinnedKtFileAccess::class) on the function.

getCurrentKtFile, getCurrentVersionedKtFile and getCurrentKtFileIfPresent become private. Comments that only restated the rule the API now enforces ("fetch the live KtFile BEFORE entering read", "safe to await a blocking refresh here") are deleted; ones that say something the types do not are kept.

StaleKtFileInstanceDiagnosticsTest and CurrentKtFileCacheTest are rewritten to go through the scope - they called the now-private accessors and could not compile otherwise.

Testing

462 tests, 0 failures in :lsp:kotlin; 61 tests, 0 failures in :common. Removing the pin short-circuits fails the repro with the ticket's exact diagnostics.

Review follow-ups

Two findings from review, both fixed here.

Import candidates resolve before the file is pinned. AddImportAction.computeImportCandidates used to
hold the pin across findSymbolBySimpleName(referenceName, limit = 0) - an unbounded SQLite scan that
never reads the file - freezing live-PSI refresh for the path for the whole scan while concurrent
acquirers joined the frozen instance. The query now runs above withLiveKtFile and is materialized
there, so the index's lazy source-active filter cannot trail into the scope either; the pin covers only
insertImport. An empty candidate list returns before the pin is taken, so the common no-candidates path
no longer resolves the live document at all. AddImportActionPinScopeTest guards the ordering with a
query-time hook on the fixture's source index.

The superseded-diagnostics reschedule is a self-send, on one of two paths. Reached through
fileAnalyzer's own action (CompilationEnvironment.kt:194), env.fileAnalyzer.schedule(file) reads to
the worker as a newer key and cancels the run it came from. Reached through
KotlinLanguageServer.analyze() instead, it is an ordinary reschedule - so the comment scopes the claim
rather than stating it flat.
That is intended - the key is still re-sent, and the cancelled tail was only going to publish
NO_UPDATE - but it does not read like an ordinary reschedule, so it now carries a comment saying so.
KeyedDebouncingActionSelfScheduleTest in :common pins down both halves of the mechanism; nothing
covered it before.

One behavioural consequence worth stating plainly: when the document moves on with no new keystroke
arriving, that analysis is computed and thrown away. Sustained typing does not take this path - each
keystroke schedules a key that cancels the analysis mid-flight through the same select - so the
computed-then-discarded case is narrower than the debounce interval suggests.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@github-actions github-actions Bot deleted a comment from atlassian Bot Aug 25, 2026
@itsaky-adfa
itsaky-adfa requested a review from a team August 25, 2026 16:53
@itsaky-adfa itsaky-adfa self-assigned this Aug 25, 2026
@itsaky-adfa itsaky-adfa changed the title ADFA 5231 migrate call sites ADFA-5231: Pin the live KtFile for the duration of an analysis Aug 25, 2026
@itsaky-adfa
itsaky-adfa force-pushed the ADFA-5231-migrate-call-sites branch from 96b520e to 656e748 Compare August 26, 2026 15:33
@itsaky-adfa
itsaky-adfa requested a review from jatezzz August 26, 2026 15:37
Lands red on purpose: it is the ADFA-4165 regression, reduced to the
smallest sequence that triggers it (acquire an instance, let a second
request install a newer one for identical text, re-analyze the first).
An analysis that started against an older instance saw every declaration
twice, once as its own PSI and once through DeclarationProvider, so FIR
reported the file as conflicting with itself. Acquisition now pins the
path for the whole scope and the raw accessors are private, so an unpinned
analysis no longer compiles.
@itsaky-adfa
itsaky-adfa force-pushed the ADFA-5231-migrate-call-sites branch from 656e748 to f96a860 Compare August 27, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants