diff --git a/docs/adr/0015-one-pinned-ktfile-per-analysis.md b/docs/adr/0015-one-pinned-ktfile-per-analysis.md new file mode 100644 index 0000000000..984d5709dd --- /dev/null +++ b/docs/adr/0015-one-pinned-ktfile-per-analysis.md @@ -0,0 +1,155 @@ +# 0015. One pinned live KtFile per analysis, enforced by the type system + +- **Status:** Proposed +- **Date:** 2026-08-25 +- **Deciders:** Code On The Go team + +## Context + +The K2 Kotlin LSP relies on one live `KtFile` instance per open path. `DeclarationProvider.ktFilesForPackage` +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt`) resolves a +path through `KtSymbolIndex.getKtFile` for anything an analysis session needs to see beyond the file it started on. +If that lookup can answer with a *different* instance than the one the analysis is holding, FIR sees every +top-level declaration twice - once as the analysis's own PSI, once through the provider - and reports the file as +conflicting with itself. That is what reaches the editor as "Redeclaration" / "Conflicting overloads" underlines +on every declaration. + +This is not a new failure. ADFA-4165 established the one-instance invariant: `CompilationEnvironment.onFileContentChanged` +captured the `KtFile` being replaced, then atomically invalidated its FIR session and installed the replacement +under `project.write`, and a companion fix to `KeyedDebouncingAction` stopped two refreshes for the same key from +running concurrently and installing out of order (commit `975d23fdfc`). ADFA-3322 (`Signature help for Kotlin LSP`, +PR #1484) replaced that file-handling path with a per-version `currentFiles` cache +(`KtSymbolIndex.getCurrentVersionedKtFile`) that mints a fresh `KtFile` every time the open document's version +changes, and neither the atomic install nor the serialization carried forward. The regression this ADR fixes is +that gap: `getCurrentVersionedKtFile` and `getKtFile` could each answer a lookup for the same path with a different +instance if a refresh landed between them, and an analysis rooted at the older one saw its own declarations doubled +through the provider. `StaleKtFileInstanceDiagnosticsTest` +(`lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt`) +reproduces it directly. + +The history is the argument for the decision below: a runtime mechanism enforced the invariant once, tied to code +that the next refactor replaced wholesale without carrying the discipline forward. A property that has to be +remembered gets lost the next time someone who does not know the history touches the code. The fix has to be +something the next refactor cannot drop without the code failing to compile. + +## Decision + +**A `KtFile` for an open path may only be obtained as a pinned handle, and only one instance is pinned to a path +at a time.** `LiveKtFile` (`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt`) +is an `internal sealed interface` whose only implementation, `KtSymbolIndex.PinnedKtFile`, is `private`. The only +way to obtain one is `KtSymbolIndex.withLiveKtFile` / `withLiveKtFileAsync` +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`), which: + +1. Acquire the path's `Pin` - join one already open (`joinExistingPin`, reference-counted), or resolve the current + instance and install a new one (`acquirePin` / `acquirePinAsync`, `installPin`). +2. While the pin is open, every door resolves to the pinned instance: `getCurrentVersionedKtFile` returns it + without minting a new one even if the document has moved on, and `getKtFile` - the resolution-side door + `DeclarationProvider.ktFilesForPackage` calls - checks `pins[path]` first. The two doors this bug came from can + no longer disagree. +3. A version bump observed while the pin is open is recorded (`Pin.refreshOwed`) rather than acted on, and applied + once the last scope releases (`releasePin`), so the pin defers the refresh instead of losing it. + +`getCurrentKtFile`, `getCurrentVersionedKtFile` and `getCurrentKtFileIfPresent` are `private`; `getKtFile` stays +`internal` but is gated behind its own `@RequiresOptIn(ERROR)` marker, `ResolutionSideKtFileAccess`, because +`internal` alone still let any file in the module - including the test source set and whatever the next refactor +adds - take the live instance and analyse it, which is exactly the shape of the ADFA-3322 regression. Its three +production opt-ins are the Analysis API service providers that only need to name the PSI for a path +(`DeclarationProvider.ktFilesForPackage`, `AnnotationsResolver.allDeclarations`, +`DirectInheritorsProvider.computeIndex`). `LiveKtFile` never exposes the `KtFile` as a value - `read` and +`analyzing` take a lambda instead of returning the file - so a caller cannot hold a reference past the scope that +pinned it. `analyzing` routes through `analyzeMaybeDangling`, which is `withAnalysisLock` under the hood +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt`), so pinning also +closes the last direct route to `analyze`/`analyzeCopy` that its doc comment could previously only ask callers not +to take. For an open path, using the shared serialization lock is no longer just a convention - it is the only +un-gated way to reach a live `KtFile`, with one known exception: `refreshToCurrent` hands the freshly minted +instance to `queueOnFileChangedAsync`, which carries the raw `KtFile` through `IndexCommand.IndexModifiedFile` to +`SourceFileIndexer.indexSourceFile`, where it is analysed with no pin (pre-existing, tracked as a follow-up). + +**One escape hatch:** `KtSymbolIndex.peekLiveKtFile`, gated behind `@RequiresOptIn(ERROR)` `UnpinnedKtFileAccess`. +Its one production caller is `AdvancedKotlinEditHandler` +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/AdvancedKotlinEditHandler.kt`), which runs +on the UI thread after completion has already returned, does PSI-only work, and opens no analysis session. +Pinning there would block the UI thread on a refresh that a background analysis might be holding up. + +That justification covers *analysis* coherence only, and the hatch is not safe in the sense the `isStale` guards +above address. `AdvancedKotlinEditHandler.performEdits` passes the unpinned instance to +`KotlinAutoImportEditHandler`, which computes offset-based `TextEdit`s from its import-directive text ranges +(`utils/EditExts.kt`, `insertImport`) and applies them to the editor buffer through `RewriteHelper.performEdits`. +Nothing compares that instance's text or version against the `Content` being edited, and `peekLiveKtFile` returns +whatever the current-file cache holds, which lags the buffer by however long the refresh takes - so this site does +hand offsets from possibly-stale PSI into an edit. The behaviour is unchanged by this ADR's change and the fix is +tracked separately; widening the hatch to a second caller has to weigh that, not just the analysis argument. + +## Consequences + +**Positive** + +- The invariant is now enforced by the compiler: code that reaches for a live `KtFile` outside `withLiveKtFile` / + `withLiveKtFileAsync` does not compile without an explicit `@OptIn` on one of the two markers, which makes every + exemption visible in review rather than reachable by autocomplete. The class of bug ADFA-4165 fixed and + ADFA-3322 silently reintroduced cannot come back from a refactor that simply forgets the discipline the old fix + depended on. +- The pin makes explicit what was previously only inferred from two call sites happening to agree: an analysis and + the declaration provider see the same PSI for the whole scope, by construction. + +**Negative / costs** + +- **A pin is process-wide, not per-caller.** A second request for a pinned path joins the pin and sees that + scope's text, which can already be older than the buffer. Pin duration is a cross-request staleness window for + everyone, not just the request that opened it. +- Callers that consult `LiveKtFile.isStale` fall into three buckets, not two. Sites whose output is an edit refuse + rather than compute offsets against frozen text: `ExtractVariablePlanner`, `ExtractMethodPlanner`, + `KotlinCompletions`, `OrganizeImportsAction`, `ImplementMembersAction`, `AddImportAction`, `NullSafetyAction`. A + refusal is recoverable; a wrong edit to the user's source is not. `KotlinDiagnosticProvider.doAnalyze` discards + and reschedules instead: it has nothing safe to hand the user in the moment, so it drops the computed diagnostics + and re-queues the file through `env.fileAnalyzer.schedule` rather than paint the editor with squiggles for text + the user has already replaced. Navigation and info sites - go-to-definition, find usages, signature help - + deliberately tolerate being one edit behind (see the comment at `GoToDefinition.kt:215`) and do not check + `isStale` at all, because their failure mode is a wrong jump, not a corrupted file or a dropped result. +- **Known parked consequence:** while background diagnostics hold a pin and the user keeps typing, a completion + request joins the stale pin and returns no items until the next keystroke closes it. Fixing this needs + acquisition to be priority-aware - an interactive request preempting a lower-priority holder instead of joining + it - which `Pin` cannot do yet: it has no notion of *which* acquirer holds it, and `AnalysisScheduler`'s + `preempt()` (ADR 0011) latches onto whichever scope is active, so signalling "the holder" from here would fire an + `AnalysisPreemptedException` into a nested outer scope that never asked to be cancelled. `Pin` becoming a + per-holder registry is a prerequisite, not scheduled here. +- The escape guard is partial. `PinnedKtFile.guarded` rejects returning the pinned file *directly* from a `read` / + `analyzing` block, but returning it wrapped - inside a collection, or as one of its child PSI elements - escapes + the check undetected and is equally unsafe. +- A narrow window remains between resolving an instance and installing its pin (documented on `withLiveKtFile`): + a request arriving in that window sees no pin yet and can launch a refresh that completes inside the scope, + firing a FIR modification event under it. Instance identity still holds through every door - the pin is stamped + with the resolved instance's own version, so the bump is not lost, only deferred. Closing the window fully would + mean publishing a pin before its file exists, making joiners wait on an unresolved entry in the one path every + caller depends on; that deadlock risk was judged worse than the window. + +## Alternatives considered + +- **A runtime mechanism that keeps the invariant true without a type gate** - what ADFA-4165 did: atomically + invalidate the superseded FIR session and install the replacement under `project.write`, serialized so two + refreshes for the same key cannot race. It worked, until ADFA-3322 replaced the code path it lived in without + carrying the same discipline forward. That is precisely how this regression happened. +- **One mutable `KtFile` per open path, reparsed in place instead of minting a new instance per version** - + strictly the deeper fix: it removes the multiple-identities problem instead of gating access to it. Not taken. + In-place reparse (`BlockSupport.reparseRange` against a `LightVirtualFile`) is unproven in this standalone/mock + Analysis API environment, which has no real `PsiDocumentManager` behind it - real feasibility risk to carry on a + regression fix. It would also still be a construction property a later refactor could quietly undo, rather than + something the compiler holds; the team chose the type gate instead and did not schedule in-place reparse as a + follow-up. +- **A custom lint/detekt rule banning the raw accessors** - the build has no detekt; Spotless's ktlint integration + only formats, it does not carry custom semantic rules, so there is no rule seat to put this in. + +## Related + +- ADFA-4165 - established the one-live-KtFile-per-path invariant, once enforced by an atomic install-and-invalidate + under `project.write` rather than by the type system. +- ADFA-3322 (PR #1484) - introduced the per-version `currentFiles` cache that reintroduced the bug. +- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) - why navigation resolves through the Analysis API, + the pipeline this pin protects. +- [ADR 0011](0011-command-analysis-priority.md) - `AnalysisScheduler` priorities and `preempt()`, referenced above + as the reason acquisition cannot yet be made priority-aware. +- `lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt` - the pinned handle. +- `lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` - `pins`, `Pin`, + `withLiveKtFile`, `withLiveKtFileAsync`, `getKtFile`. +- `lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt` - + reproduces the regression this ADR documents the fix for. diff --git a/docs/adr/README.md b/docs/adr/README.md index 5b7b7fa226..0ba00dda75 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -26,5 +26,6 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0010](0010-navigation-resolves-via-analysis-api.md) | Kotlin navigation resolves via the Analysis API, not the symbol index | Proposed | | [0011](0011-command-analysis-priority.md) | User-invoked commands get their own analysis priority | Proposed | | [0012](0012-volatile-build-metadata-out-of-abis.md) | Keep volatile build metadata out of module ABIs | Proposed | -| [0013](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | -| [0014](0013-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | +| [0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | +| [0014](0014-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | +| [0015](0015-one-pinned-ktfile-per-analysis.md) | One pinned live KtFile per analysis, enforced by the type system | Proposed | diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt index 73a0c87cdb..e7d65d9a08 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt @@ -104,12 +104,15 @@ import kotlinx.coroutines.cancel import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode import org.slf4j.LoggerFactory import java.io.File +import java.util.concurrent.atomic.AtomicInteger import kotlin.coroutines.resume fun interface OnEditorLongPressListener { @@ -141,7 +144,17 @@ open class IDEEditor private var actionsMenu: EditorActionsMenu? = null private var _signatureHelpWindow: SignatureHelpWindow? = null private var _diagnosticWindow: DiagnosticWindow? = null - private var fileVersion = 0 + + /** + * [documentChangeMutex] only serialises change dispatches against each other; the resets in + * [release] and [dispatchDocumentOpenEvent] run outside it, so a reset can race an in-flight + * [dispatchDocumentChangeEvent]'s `incrementAndGet()` and stamp a low version right after a + * newly-opened file's counter is zeroed. This is tolerated: it is bounded (self-heals on the + * next edit) and distinct from the same-document backwards-version bug this ticket fixes, + * which `ActiveDocument.update` now guards regardless of how `fileVersion` got there. + */ + private val fileVersion = AtomicInteger(0) + private val documentChangeMutex = Mutex() internal var isModified = false // Length and content hash of the content the last time the file was loaded or saved. @@ -570,7 +583,7 @@ open class IDEEditor languageClient = null _file = null - fileVersion = 0 + fileVersion.set(0) markUnmodified() editorFeatures.editor = null @@ -960,7 +973,9 @@ open class IDEEditor file ?: return@subscribeEvent editorScope.launch { - dispatchDocumentChangeEvent(event) + // Serialised so the version a change is stamped with is never older than the text + // snapshot taken with it: two edits in one frame land here as two coroutines. + documentChangeMutex.withLock { dispatchDocumentChangeEvent(event) } checkForSignatureHelp(event) handleCustomTextReplacement(event) } @@ -1242,9 +1257,9 @@ open class IDEEditor val file = this.file ?: return - this.fileVersion = 0 + this.fileVersion.set(0) - val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion) + val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion.get()) eventDispatcher.dispatch(openEvent) } @@ -1278,7 +1293,7 @@ open class IDEEditor file, changedText, text.toString(), - ++fileVersion, + fileVersion.incrementAndGet(), type, changeDelta, changeRange, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt index 33ed711df8..a35fc37056 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt @@ -76,24 +76,32 @@ class AddImportAction : BaseKotlinCodeAction() { * [postExec] shows in the chooser -- so two index entries for the same class collapse into one * entry instead of duplicating it. * - * Blocking: does the `getCurrentKtFile` `.get()` and a SQLite-backed index query, so callers must - * stay off the main thread ([execAction] wraps it in [Dispatchers.IO]). + * Blocking: pinning the file resolves it first, and the index query is SQLite-backed, so callers + * must stay off the main thread ([execAction] wraps it in [Dispatchers.IO]). */ internal fun computeImportCandidates( env: AbstractCompilationEnvironment, nioPath: Path, referenceName: String, - ): Map> { - val ktFile = - env.ktSymbolIndex - .getCurrentKtFile(nioPath) - .get() ?: return emptyMap() - - return env.ktSymbolIndex - .findSymbolBySimpleName(referenceName, limit = 0) - .filter { it.kind.isClassifier } - .associate { it.fqName to insertImport(ktFile, it.fqName) } - } + ): Map> = + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + if (live.isStale) { + // Joining another feature's scope hands over text older than the buffer, so the import + // insertion point computed from it would land in the wrong place. + logger.debug("skipping import candidates for {}: pinned text is behind the buffer", nioPath) + return@withLiveKtFile emptyMap() + } + + // The index query stays outside `read`, so the disk hit does not hold the project read lock. + val classifiers = + env.ktSymbolIndex + .findSymbolBySimpleName(referenceName, limit = 0) + .filter { it.kind.isClassifier } + + live.read { ktFile -> + classifiers.associate { it.fqName to insertImport(ktFile, it.fqName) } + } + } ?: emptyMap() override fun postExec( data: ActionData, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt index 6c103772bf..5f63d378e0 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt @@ -8,10 +8,8 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.membersToImplement import com.itsaky.androidide.lsp.kotlin.utils.renderOverrideStub import com.itsaky.androidide.lsp.kotlin.utils.toRange @@ -59,8 +57,7 @@ class ImplementMembersAction : BaseKotlinCodeAction() { /** * Computes the edit that inserts stubs for the abstract members left unimplemented by the class or - * object enclosing [offset] in the file at [nioPath]. The current [KtFile] is fetched BEFORE - * entering [read] (deadlock rule: never block on `getCurrentKtFile(...).get()` inside `project.read`). + * object enclosing [offset] in the file at [nioPath]. * * Returns an empty list when there is nothing to do (cursor not in a class/object, the declaration * is abstract/an interface/enum, or every required member is already implemented) *and* whenever @@ -75,27 +72,37 @@ class ImplementMembersAction : BaseKotlinCodeAction() { cancelChecker: ICancelChecker, ): List = runCatching { - // A user-invoked command: AnalysisPriority.COMMAND, retried once if keystroke-driven work - // preempts it (ADR 0011). Without the retry a preemption fell into the getOrElse below and the - // action silently inserted nothing. The file is re-fetched per attempt because the preemptor - // also refreshed the live PSI. + /* + * A user-invoked command: AnalysisPriority.COMMAND, retried once if keystroke-driven work + * preempts it (ADR 0011). Without the retry a preemption fell into the getOrElse below and the + * action silently inserted nothing. The file is re-pinned per attempt because the preemptor + * also refreshed the live PSI. + */ retryingOnPreemption(cancelChecker, "Implement members for $nioPath") { checker -> - val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return@retryingOnPreemption emptyList() - env.project.read { - val classOrObject = findEnclosingClassOrObject(ktFile, offset) ?: return@read emptyList() - analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, checker) { - val classSymbol = classOrObject.symbol as? KaClassSymbol ?: return@analyzeMaybeDangling emptyList() - if (!isImplementable(classSymbol)) return@analyzeMaybeDangling emptyList() - - val classIndent = classIndentOf(ktFile, classOrObject) - val unit = detectIndentUnit(ktFile.text) - val memberIndent = memberIndentOf(ktFile, classOrObject, classIndent, unit) - val stubs = membersToImplement(classSymbol).mapNotNull { renderOverrideStub(it, memberIndent, unit) } - if (stubs.isEmpty()) return@analyzeMaybeDangling emptyList() - - buildInsertionEdit(ktFile, classOrObject, stubs, classIndent) + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + if (live.isStale) { + // Joining another feature's scope hands over text older than the buffer, so both the + // caret offset and the computed insertion point would land in the wrong place. + logger.debug("skipping implement-members for {}: pinned text is behind the buffer", nioPath) + return@withLiveKtFile emptyList() } - } + + live.read { ktFile -> + val classOrObject = findEnclosingClassOrObject(ktFile, offset) ?: return@read emptyList() + live.analyzing(AnalysisPriority.COMMAND, checker) { + val classSymbol = classOrObject.symbol as? KaClassSymbol ?: return@analyzing emptyList() + if (!isImplementable(classSymbol)) return@analyzing emptyList() + + val classIndent = classIndentOf(ktFile, classOrObject) + val unit = detectIndentUnit(ktFile.text) + val memberIndent = memberIndentOf(ktFile, classOrObject, classIndent, unit) + val stubs = membersToImplement(classSymbol).mapNotNull { renderOverrideStub(it, memberIndent, unit) } + if (stubs.isEmpty()) return@analyzing emptyList() + + buildInsertionEdit(ktFile, classOrObject, stubs, classIndent) + } + } + } ?: emptyList() } }.getOrElse { e -> if (e.isAnalysisCancellation()) { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt index 85f4702a2c..7b85356f96 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt @@ -11,7 +11,7 @@ import com.itsaky.androidide.actions.requireFile import com.itsaky.androidide.idetooltips.TooltipManager import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.lsp.api.ILanguageClient -import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.diagnostic.DiagnosticAction import com.itsaky.androidide.lsp.kotlin.utils.NullSafetyKind import com.itsaky.androidide.lsp.kotlin.utils.NullSafetyVariant @@ -26,6 +26,7 @@ import com.itsaky.androidide.utils.applyLongPressRecursively import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext +import java.nio.file.Path /** * Offers null-safety quick fixes on an UNSAFE_CALL diagnostic (`receiver.selector` where `receiver` @@ -67,22 +68,14 @@ class NullSafetyAction : BaseKotlinCodeAction() { val nioPath = data.requireFile().toPath() - // Fetch the live KtFile BEFORE entering `read` (deadlock rule: its refresh needs write access). - val ktFile = - withContext(Dispatchers.IO) { - extra.compilationEnv.ktSymbolIndex - .getCurrentKtFile(nioPath) - .get() - } ?: return emptyList() - - extra.compilationEnv.project.read { - val qe = - findNullableMemberAccess( - ktFile, - diagnostic.range.start.requireIndex(), - diagnostic.range.end.requireIndex(), - ) ?: return@read emptyList() - nullSafetyVariants(qe) + // Off the main thread: acquiring the pin resolves the file first, which can block on a refresh. + withContext(Dispatchers.IO) { + computeNullSafetyVariants( + extra.compilationEnv, + nioPath, + diagnostic.range.start.requireIndex(), + diagnostic.range.end.requireIndex(), + ) } }.getOrElse { e -> if (e is CancellationException) throw e @@ -90,6 +83,33 @@ class NullSafetyAction : BaseKotlinCodeAction() { emptyList() } + /** + * The null-safety rewrites for the nullable member access spanning [startOffset] to [endOffset]. + * + * Blocking: pinning the file resolves it first, so callers must stay off the main thread + * ([execAction] wraps it in [Dispatchers.IO]). Returns an empty list when the span names no + * nullable access, and when the pinned text is behind the buffer. + */ + internal fun computeNullSafetyVariants( + env: AbstractCompilationEnvironment, + nioPath: Path, + startOffset: Int, + endOffset: Int, + ): List = + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + if (live.isStale) { + // Joining another feature's scope hands over text older than the buffer, and these variants + // carry raw PSI offsets that nothing downstream re-checks against the document. + logger.debug("skipping null-safety fixes for {}: pinned text is behind the buffer", nioPath) + return@withLiveKtFile emptyList() + } + + live.read { ktFile -> + val qe = findNullableMemberAccess(ktFile, startOffset, endOffset) ?: return@read emptyList() + nullSafetyVariants(qe) + } + } ?: emptyList() + override fun postExec( data: ActionData, result: Any, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt index 4f2012bef2..d87b45ba7c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt @@ -7,10 +7,8 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.collectImportUsage import com.itsaky.androidide.lsp.kotlin.utils.organizedImportBlock import com.itsaky.androidide.lsp.kotlin.utils.toRange @@ -48,11 +46,10 @@ class OrganizeImportsAction : BaseKotlinCodeAction() { /** * Computes the text edits that organize the imports of the file at [nioPath] within [env]. - * The current [org.jetbrains.kotlin.psi.KtFile] is fetched BEFORE entering [read] (deadlock - * rule: never block on `getCurrentKtFile(...).get()` inside `project.read`). Returns an empty - * list when there is nothing to do (no imports, already organized, or no usable range) *and* - * whenever anything in this pipeline (the `.get()`, analysis, or PSI access) throws: the action - * framework only catches [IllegalArgumentException] and this runs on a coroutine scope with no + * + * Returns an empty list when there is nothing to do (no imports, already organized, or no usable + * range) *and* whenever anything in this pipeline (acquisition, analysis, or PSI access) throws: the + * action framework only catches [IllegalArgumentException] and this runs on a coroutine scope with no * exception handler, so an uncaught throw here would crash the app. Degrading to zero edits is * always safe -- it just leaves the imports as-is, never produces a partial/incorrect rewrite. */ @@ -62,20 +59,30 @@ class OrganizeImportsAction : BaseKotlinCodeAction() { cancelChecker: ICancelChecker, ): List = runCatching { - // A user-invoked command: AnalysisPriority.COMMAND, retried once if keystroke-driven work - // preempts it (ADR 0011). Without the retry a preemption fell into the getOrElse below and - // organize-imports silently did nothing. The file is re-fetched per attempt because the - // preemptor also refreshed the live PSI. + /* + * A user-invoked command: AnalysisPriority.COMMAND, retried once if keystroke-driven work + * preempts it (ADR 0011). Without the retry a preemption fell into the getOrElse below and + * organize-imports silently did nothing. The file is re-pinned per attempt because the + * preemptor also refreshed the live PSI. + */ retryingOnPreemption(cancelChecker, "Organize imports for $nioPath") { checker -> - val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return@retryingOnPreemption emptyList() - if (ktFile.importDirectives.isEmpty()) return@retryingOnPreemption emptyList() - env.project.read { - val usage = analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, checker) { collectImportUsage(ktFile) } - val newText = organizedImportBlock(ktFile, usage) ?: return@read emptyList() - val range = ktFile.importList?.textRange?.toRange(ktFile) ?: return@read emptyList() - if (range == Range.NONE) return@read emptyList() - listOf(TextEdit(range, newText)) - } + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + if (live.isStale) { + // Joining another feature's scope hands over text older than the buffer, and the + // import-list range computed from it would replace the wrong span. + logger.debug("skipping organize-imports for {}: pinned text is behind the buffer", nioPath) + return@withLiveKtFile emptyList() + } + + live.read { ktFile -> + if (ktFile.importDirectives.isEmpty()) return@read emptyList() + val usage = live.analyzing(AnalysisPriority.COMMAND, checker) { collectImportUsage(it) } + val newText = organizedImportBlock(ktFile, usage) ?: return@read emptyList() + val range = ktFile.importList?.textRange?.toRange(ktFile) ?: return@read emptyList() + if (range == Range.NONE) return@read emptyList() + listOf(TextEdit(range, newText)) + } + } ?: emptyList() } }.getOrElse { e -> if (e.isAnalysisCancellation()) { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt index 84079924fa..74adf94b54 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt @@ -26,7 +26,6 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancelAndJoin -import kotlinx.coroutines.future.await import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull @@ -210,7 +209,7 @@ internal class CompilationEnvironment( ) { path, _ -> // Pull through the cache so a refresh (and its reindex) happens after every edit, // independent of whether diagnostics run. - ktSymbolIndex.getCurrentKtFile(path).await() + ktSymbolIndex.refreshCurrentKtFile(path) } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt index 2b36a45cd7..f5e9144a32 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt @@ -2,7 +2,10 @@ package com.itsaky.androidide.lsp.kotlin.compiler.index import com.github.benmanes.caffeine.cache.Caffeine import com.itsaky.androidide.lsp.kotlin.compiler.CompilationKind +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.compiler.services.ProjectStructureProvider @@ -17,17 +20,21 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.future.await import kotlinx.coroutines.launch import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolIndex import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataIndex import org.appdevforall.codeonthego.indexing.service.IndexKey import org.checkerframework.checker.index.qual.NonNegative +import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.platform.modification.KaElementModificationType import org.jetbrains.kotlin.analysis.api.platform.modification.KaSourceModificationService +import org.jetbrains.kotlin.analysis.low.level.api.fir.util.originalKtFile import org.jetbrains.kotlin.com.intellij.openapi.application.ApplicationManager import org.jetbrains.kotlin.com.intellij.openapi.project.Project import org.jetbrains.kotlin.com.intellij.openapi.vfs.VirtualFile import org.jetbrains.kotlin.com.intellij.psi.PsiManager +import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.KtPsiFactory import org.slf4j.LoggerFactory @@ -68,6 +75,9 @@ internal class KtSymbolIndex( private val logger = LoggerFactory.getLogger(KtSymbolIndex::class.java) const val DEFAULT_CACHE_SIZE = 100L private const val CLOSE_DRAIN_TIMEOUT_SECONDS = 5L + + /** Pin version stamp for a path with no open document, which no real version can equal. */ + private const val NO_DOCUMENT_VERSION = -1 } private val workerQueue = WorkerQueue() @@ -114,6 +124,32 @@ internal class KtSymbolIndex( /** path -> last-launched version; read/written only inside that same `compute` section. */ private val currentVersions = ConcurrentHashMap() + /** + * path -> the instance pinned for the duration of one or more open [LiveKtFile] scopes. + * + * A pinned path is frozen: [getCurrentKtFile] hands back the pinned instance rather than minting a + * new one for a newer document version, and [getKtFile] resolves to it too, so an analysis and the + * declaration provider cannot disagree about which instance is the file. The refresh a version bump + * would have triggered is recorded and launched when the last scope closes. + * + * That deferral is best-effort, not a guarantee: [getCurrentKtFile] reads the pin outside the map's + * atomic section, so a bump observed exactly as the last scope releases can be recorded on an entry + * that has already been removed, and lost. It self-heals - [currentVersions] still holds the older + * version, so the next request for the path refreshes. + */ + private val pins = ConcurrentHashMap() + + private class Pin( + val file: KtFile, + val version: Int, + ) { + var count: Int = 0 + + /** Written outside the map's `compute` section, by whichever thread observes the version bump. */ + @Volatile + var refreshOwed: Boolean = false + } + fun syncIndexInBackground() { indexingJob?.cancel() startIndexing() @@ -191,12 +227,36 @@ internal class KtSymbolIndex( * version miss. For non-open paths (no active document) falls back to the disk [getKtFile]. * Single-flight: concurrent callers at the same version share one parse. */ - fun getCurrentKtFile(path: Path): CompletableFuture { - if (!DocumentUtils.isKotlinFile(path)) return CompletableFuture.completedFuture(null) + private fun getCurrentKtFile(path: Path): CompletableFuture = + getCurrentVersionedKtFile(path)?.thenApply { it.ktFile } ?: CompletableFuture.completedFuture(null) + + /** + * [getCurrentKtFile] with the document version the instance was parsed from, or `null` if [path] + * has no Kotlin PSI at all. + * + * Pin acquisition needs the version *of the resolved instance*, not the one the document happens + * to be at once the parse finishes - re-reading [FileManager] after a blocking resolve stamps a + * pin with a version its PSI does not have, which makes [LiveKtFile.isStale] claim a superseded + * instance is current. + */ + @OptIn(ResolutionSideKtFileAccess::class) + private fun getCurrentVersionedKtFile(path: Path): CompletableFuture? { + if (!DocumentUtils.isKotlinFile(path)) return null + + pins[path]?.let { pin -> + val current = FileManager.getActiveDocument(path)?.version + if (current != null && current != pin.version) { + pin.refreshOwed = true + } + return CompletableFuture.completedFuture(VersionedKtFile(pin.version, pin.file)) + } val doc = FileManager.getActiveDocument(path) - ?: return CompletableFuture.completedFuture(getKtFile(path)) // not open -> disk path + ?: return getKtFile(path)?.let { + // not open -> disk path + CompletableFuture.completedFuture(VersionedKtFile(NO_DOCUMENT_VERSION, it)) + } val version = doc.version val future = @@ -218,7 +278,7 @@ internal class KtSymbolIndex( }, refreshExecutor) } }!! - return future.thenApply { it.ktFile } + return future } /** @@ -268,20 +328,216 @@ internal class KtSymbolIndex( * else `null`. Safe to call while holding `project.read` (unlike [getCurrentKtFile], which may * trigger a blocking refresh that needs `project.write`). */ - fun getCurrentKtFileIfPresent(path: Path): KtFile? = currentFiles[path]?.getNow(null)?.ktFile + private fun getCurrentKtFileIfPresent(path: Path): KtFile? = currentFiles[path]?.getNow(null)?.ktFile + + /** + * Runs [block] with the file at [path] pinned, or returns `null` if the path has no Kotlin PSI. + * + * Blocking: resolves the current instance before pinning, and that refresh needs `project.write`. + * Never call this while holding `project.read` - it deadlocks. Acquire the scope first, then use + * [LiveKtFile.read] / [LiveKtFile.analyzing] inside it, which take the read lock for you. + * + * The pin is process-wide, not per-caller: while any scope on [path] is open, *every* request for + * that path joins it and sees the same instance and the same text, including requests from unrelated + * features. So a scope's duration is a staleness window for everyone else - a caller that joins a + * long-running scope can get text older than the buffer the user is looking at. Any site whose + * output is an edit, or that indexes into the text with coordinates from its own request, must + * therefore check [LiveKtFile.isStale] and degrade rather than compute against frozen text. + * + * Known gap: the instance is resolved *before* the pin is installed, so a request arriving in that + * window sees no pin and can launch a refresh that completes inside this scope, firing + * `registerInMemoryFile` and a FIR modification event underneath it. Instance identity still holds - + * every door answers with the pinned instance for the whole scope - and the pin is stamped with the + * resolved instance's own version, so the bump is not lost. Closing the window entirely would mean + * publishing a pin before its file exists, making joiners wait on an unresolved entry inside the one + * path every caller depends on; that deadlock risk is worse than the window. + */ + fun withLiveKtFile( + path: Path, + block: (LiveKtFile) -> R, + ): R? { + val pin = acquirePin(path) { getCurrentVersionedKtFile(path)?.get() } ?: return null + try { + return block(PinnedKtFile(path, pin)) + } finally { + releasePin(path) + } + } + + /** Suspending [withLiveKtFile], for callers that must not block a dispatcher thread. */ + suspend fun withLiveKtFileAsync( + path: Path, + block: (LiveKtFile) -> R, + ): R? { + val pin = acquirePinAsync(path) ?: return null + try { + return block(PinnedKtFile(path, pin)) + } finally { + releasePin(path) + } + } + + /** + * Pulls [path] through the current-file cache so a refresh (and its reindex) happens, without + * handing the instance to the caller. + * + * This is the door for callers that want the refresh side effect only, so wanting a refresh never + * becomes a reason to hold a live instance. + */ + suspend fun refreshCurrentKtFile(path: Path) { + getCurrentKtFile(path).await() + } + + /** + * The current instance for [path] with no pin, or `null` if none is cached. + * + * Non-blocking and PSI-only. See [UnpinnedKtFileAccess] for why this is opt-in. + */ + @UnpinnedKtFileAccess + fun peekLiveKtFile(path: Path): KtFile? = getCurrentKtFileIfPresent(path) + + private inline fun acquirePin( + path: Path, + resolve: () -> VersionedKtFile?, + ): Pin? { + joinExistingPin(path)?.let { return it } + // Resolved outside the map mutation: it can block on a refresh, and holding a ConcurrentHashMap + // bin lock across that would stall every other path. + val resolved = resolve() ?: return null + return installPin(path, resolved) + } + + private suspend fun acquirePinAsync(path: Path): Pin? { + joinExistingPin(path)?.let { return it } + val resolved = getCurrentVersionedKtFile(path)?.await() ?: return null + return installPin(path, resolved) + } + + private fun joinExistingPin(path: Path): Pin? = pins.compute(path) { _, existing -> existing?.also { it.count++ } } + + private fun installPin( + path: Path, + resolved: VersionedKtFile, + ): Pin { + val pin = + pins.compute(path) { _, existing -> + // A concurrent acquirer may have won the race; join its pin and let this file go. Both + // resolved through the same single-flight future, so they are the same instance anyway. + existing?.also { it.count++ } + ?: Pin(resolved.ktFile, resolved.version).also { it.count = 1 } + }!! + + /* + * The document can move on while the resolve is still parsing, so the pinned instance may already + * be behind by the time it is installed. That bump has no pinned instance left to refresh into, + * hence record it as owed here rather than let it fall between the resolve and the pin. Safe to + * write outside the section above: this thread holds a count, so no release can be reading it. + */ + val current = FileManager.getActiveDocument(path)?.version + if (current != null && current != pin.version) { + pin.refreshOwed = true + } + return pin + } + + private fun releasePin(path: Path) { + var refreshOwed = false + pins.compute(path) { _, pin -> + if (pin == null) return@compute null + if (--pin.count > 0) return@compute pin + refreshOwed = pin.refreshOwed + null + } + + // Applied on the way out rather than during the pin: the version bump that arrived while the path + // was frozen still has to reach the FIR session. Skipped once the document is gone, since + // invalidateCurrent already unregistered it. + if (refreshOwed && FileManager.isActive(path)) { + scope.launch { refreshCurrentKtFile(path) } + } + } + + private inner class PinnedKtFile( + override val path: Path, + private val pin: Pin, + ) : LiveKtFile { + override val isStale: Boolean + get() { + val current = FileManager.getActiveDocument(path)?.version ?: return false + return current != pin.version + } + + override fun read(block: (KtFile) -> R): R = project.read { guarded(block(pin.file)) } + + override fun analyzing( + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + useSite: KtElement?, + block: KaSession.(KtFile) -> R, + ): R = + project.read { + guarded( + analyzeMaybeDangling(useSite ?: pin.file, priority, cancelChecker) { block(pin.file) }, + ) + } - fun getKtFile(vf: VirtualFile): KtFile? = getKtFile(vf.toNioPath(), vf) + override fun analyzingVariant( + name: String, + text: String, + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + block: KaSession.(KtFile) -> R, + ): R { + val variant = + project.read { + parser.createFile(fileName = name, text = text).apply { + originalFile = pin.file + originalKtFile = pin.file + } + } + // No guard here: the block only ever sees the variant, so it cannot return the pinned file. + return project.read { + analyzeMaybeDangling(variant, priority, cancelChecker) { block(variant) } + } + } - fun getKtFile( + /** Catches `read { it }`: returning the pinned file outlives the pin that made it safe to use. */ + private fun guarded(result: R): R { + check(result !== pin.file) { + "The pinned KtFile for $path must not escape its LiveKtFile scope." + } + return result + } + } + + /** [getKtFile] for [vf], keyed by the path it maps to. */ + @ResolutionSideKtFileAccess + internal fun getKtFile(vf: VirtualFile): KtFile? = getKtFile(vf.toNioPath(), vf) + + /** + * The resolution-side door: what the Analysis API service providers answer a path lookup with. + * + * A pinned path resolves to the pinned instance, so an open analysis and the declaration provider + * cannot disagree about which instance is the file. Otherwise the live cache is peeked, then the + * on-disk instance is loaded. See [ResolutionSideKtFileAccess] for why this is opt-in. + */ + @ResolutionSideKtFileAccess + internal fun getKtFile( path: Path, virtualFile: VirtualFile? = null, ): KtFile? { if (!DocumentUtils.isKotlinFile(path)) return null + // A pinned path resolves to the pinned instance for every door, which is the whole point of the + // pin: this is the branch the Analysis API declaration providers take while an analysis is open. + pins[path]?.let { return it.file } + if (FileManager.isActive(path)) { - // Peek, never block: getKtFile runs under project.read inside Analysis-API services, so a - // blocking getCurrentKtFile().get() (its refresh needs project.write) would deadlock. A miss - // falls through to the disk instance; the edit already scheduled a refresh for next time. + /* + * Peek, never block: getKtFile runs under project.read inside Analysis-API services, so a + * blocking refresh (which needs project.write) would deadlock. A miss falls through to the disk + * instance; the edit already scheduled a refresh for next time. + */ getCurrentKtFileIfPresent(path)?.let { return it } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt new file mode 100644 index 0000000000..d69fcbb54e --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt @@ -0,0 +1,98 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.index + +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.psi.KtElement +import org.jetbrains.kotlin.psi.KtFile +import java.nio.file.Path + +/** + * Marks the one door that hands out a live [KtFile] without pinning it. + * + * An unpinned instance can be superseded while it is in use, which is what makes FIR report a file as + * conflicting with itself (ADFA-4165, ADFA-5231). Opting in is only defensible for PSI-only work that + * opens no analysis session; anything that analyses must use [KtSymbolIndex.withLiveKtFile]. + */ +@RequiresOptIn( + level = RequiresOptIn.Level.ERROR, + message = "Unpinned live KtFile access. Use KtSymbolIndex.withLiveKtFile unless this is PSI-only work.", +) +@Retention(AnnotationRetention.BINARY) +internal annotation class UnpinnedKtFileAccess + +/** + * Marks the resolution-side door: what the Analysis API service providers answer "what PSI is at this + * path" with. + * + * For an open path it hands back the live instance (the pinned one while a pin is held, otherwise + * whatever the current-file cache holds), so it is a reference that can be superseded. Analysing what + * it returns without a pin is exactly what ADFA-3322 did, and it makes FIR see every top-level + * declaration twice. Opting in is for service providers that only need to name the PSI for a path; + * anything that analyses must use [KtSymbolIndex.withLiveKtFile]. + */ +@RequiresOptIn( + level = RequiresOptIn.Level.ERROR, + message = "Resolution-side KtFile access. Use KtSymbolIndex.withLiveKtFile for anything that analyses.", +) +@Retention(AnnotationRetention.BINARY) +internal annotation class ResolutionSideKtFileAccess + +/** + * A [KtFile] pinned to its path for the lifetime of the scope that produced it. + * + * Every door into the index - the analysis root here, and `getKtFile` as used by the Analysis API + * service providers - resolves the pinned path to this one instance while the scope is open, so an + * analysis can never see its own declarations twice. + * + * The file is deliberately not exposed as a value: obtain it for the duration of a [read] or + * [analyzing] block instead. Returning it *directly* from such a block defeats the pin and is rejected. + * Only that shape is detected - returning it wrapped (in a collection, or as one of its child elements) + * escapes the check and is just as unsafe. + * + * Only [KtSymbolIndex.withLiveKtFile] and [KtSymbolIndex.withLiveKtFileAsync] can produce one. + */ +internal sealed interface LiveKtFile { + /** The path this instance is pinned to. */ + val path: Path + + /** + * True once the open document has moved past the version this instance was parsed from. + * + * A result computed from a stale instance describes text the user has already replaced; publish it + * and the editor shows diagnostics for the wrong content. Always false for a path with no open + * document. + */ + val isStale: Boolean + + /** Runs [block] with the pinned file under the project read lock. */ + fun read(block: (KtFile) -> R): R + + /** + * Analyses [useSite] (the pinned file by default) and runs [block] with the pinned file. + * + * Holds the project read lock and the global analysis lock at [priority]; [useSite] must be the + * pinned file or an element inside it. + */ + fun analyzing( + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + useSite: KtElement? = null, + block: KaSession.(KtFile) -> R, + ): R + + /** + * Analyses a dangling copy of the pinned file whose text is [text], named [name]. + * + * Completion parses a placeholder variant of the buffer; the copy's `originalFile` must point at the + * pinned instance or its resolution goes through a file the provider does not know about. Wiring that + * up here is why callers never build the copy themselves. + */ + fun analyzingVariant( + name: String, + text: String, + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + block: KaSession.(KtFile) -> R, + ): R +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index 7bf3f7f473..343a0920d3 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -31,7 +31,11 @@ private val logger = LoggerFactory.getLogger("KtFileExts") * (`KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.`). * [AnalysisScheduler] serializes access; it is priority-aware, preemptive (via [cancelChecker]) and * reentrant. **All** Analysis API access must go through this helper (or [analyzeMaybeDangling]); never - * call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. + * call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. For an open file this + * is no longer only a convention: every route to a live `KtFile` is either `LiveKtFile.analyzing`, which + * calls this helper for you, or gated behind an opt-in marker - with one known exception, the + * modified-file indexer, which is handed a raw instance through `IndexCommand.IndexModifiedFile` and + * analyses it unpinned (tracked as a follow-up). * * **Cancellation.** [action] runs with a [kotlinx.coroutines.Job] installed in the thread's IntelliJ * context; the compiler's dense `checkCanceled()` calls throw once that Job is cancelled, aborting diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt index 0d04ab5888..f0637296f6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.services import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex +import com.itsaky.androidide.lsp.kotlin.compiler.index.ResolutionSideKtFileAccess import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import org.jetbrains.kotlin.analysis.api.platform.declarations.KotlinAnnotationsResolver import org.jetbrains.kotlin.analysis.api.platform.declarations.KotlinAnnotationsResolverFactory @@ -25,8 +26,9 @@ import org.jetbrains.kotlin.psi.KtUserType import org.jetbrains.kotlin.psi.declarationRecursiveVisitor import org.jetbrains.kotlin.util.collectionUtils.filterIsInstanceAnd -internal class AnnotationsResolverFactory : KtLspService, KotlinAnnotationsResolverFactory { - +internal class AnnotationsResolverFactory : + KtLspService, + KotlinAnnotationsResolverFactory { private lateinit var project: Project private lateinit var index: KtSymbolIndex @@ -34,15 +36,14 @@ internal class AnnotationsResolverFactory : KtLspService, KotlinAnnotationsResol project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.project = project this.index = index } - override fun createAnnotationResolver(searchScope: GlobalSearchScope): KotlinAnnotationsResolver { - return AnnotationsResolver(project, searchScope, index) - } + override fun createAnnotationResolver(searchScope: GlobalSearchScope): KotlinAnnotationsResolver = + AnnotationsResolver(project, searchScope, index) } @Suppress("UnstableApiUsage") @@ -51,58 +52,59 @@ internal class AnnotationsResolver( private val scope: GlobalSearchScope, private val index: KtSymbolIndex, ) : KotlinAnnotationsResolver { - private val declarationProvider by lazy { project.createDeclarationProvider(scope, contextualModule = null) } + @OptIn(ResolutionSideKtFileAccess::class) private fun allDeclarations(): List { val virtualFiles = VirtualFileEnumeration.extract(scope) ?: return emptyList() - val filesInScope = virtualFiles - .filesIfCollection - .orEmpty() - .asSequence() - .filter { it in scope } - .mapNotNull { index.getKtFile(it) } + val filesInScope = + virtualFiles + .filesIfCollection + .orEmpty() + .asSequence() + .filter { it in scope } + .mapNotNull { index.getKtFile(it) } return buildList { - val visitor = declarationRecursiveVisitor visit@{ - val isLocal = when (it) { - is KtClassOrObject -> it.isLocal - is KtFunction -> it.isLocal - is KtProperty -> it.isLocal - else -> return@visit - } - - if (!isLocal) { - add(it) + val visitor = + declarationRecursiveVisitor visit@{ + val isLocal = + when (it) { + is KtClassOrObject -> it.isLocal + is KtFunction -> it.isLocal + is KtProperty -> it.isLocal + else -> return@visit + } + + if (!isLocal) { + add(it) + } } - } filesInScope.forEach { it.accept(visitor) } } } - override fun declarationsByAnnotation(annotationClassId: ClassId): Set { - return allDeclarations() + override fun declarationsByAnnotation(annotationClassId: ClassId): Set = + allDeclarations() .asSequence() .filter { annotationClassId in annotationsOnDeclaration(it) } .toSet() - } - override fun annotationsOnDeclaration(declaration: KtAnnotated): Set { - return declaration + override fun annotationsOnDeclaration(declaration: KtAnnotated): Set = + declaration .annotationEntries .asSequence() .flatMap { it.typeReference?.resolveAnnotationClassIds(declarationProvider).orEmpty() } .toSet() - } } private fun KtTypeReference.resolveAnnotationClassIds( declarationProvider: KotlinDeclarationProvider, - candidates: MutableSet = mutableSetOf() + candidates: MutableSet = mutableSetOf(), ): Set { val annotationTypeElement = typeElement as? KtUserType val referencedName = annotationTypeElement?.referencedFqName ?: return emptySet() @@ -132,8 +134,10 @@ private val KtUserType.referencedFqName: FqName? return FqName.fromSegments(allQualifiers) } - -private fun FqName.resolveToClassIds(to: MutableSet, declarationProvider: KotlinDeclarationProvider) { +private fun FqName.resolveToClassIds( + to: MutableSet, + declarationProvider: KotlinDeclarationProvider, +) { toClassIdSequence().mapNotNullTo(to) { classId -> val classes = declarationProvider.getAllClassesByClassId(classId) val typeAliases = declarationProvider.getAllTypeAliasesByClassId(classId) @@ -162,4 +166,3 @@ private fun FqName.toClassIdSequence(): Sequence { } } } - diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt index b8e3498a21..fddf89d779 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.services import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex +import com.itsaky.androidide.lsp.kotlin.compiler.index.ResolutionSideKtFileAccess import com.itsaky.androidide.lsp.kotlin.compiler.index.filesForPackage import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.read @@ -33,8 +34,9 @@ import org.jetbrains.kotlin.psi.KtTypeAlias import org.jetbrains.kotlin.psi.psiUtil.isTopLevelKtOrJavaMember import java.nio.file.Paths -internal class DeclarationProviderFactory : KtLspService, KotlinDeclarationProviderFactory { - +internal class DeclarationProviderFactory : + KtLspService, + KotlinDeclarationProviderFactory { private lateinit var project: Project private lateinit var index: KtSymbolIndex @@ -42,7 +44,7 @@ internal class DeclarationProviderFactory : KtLspService, KotlinDeclarationProvi project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.project = project this.index = index @@ -50,13 +52,13 @@ internal class DeclarationProviderFactory : KtLspService, KotlinDeclarationProvi override fun createDeclarationProvider( scope: GlobalSearchScope, - contextualModule: KaModule? - ): KotlinDeclarationProvider { - return DeclarationProvider(scope, project, index) - } + contextualModule: KaModule?, + ): KotlinDeclarationProvider = DeclarationProvider(scope, project, index) } -class DeclarationProviderMerger(private val project: Project) : KotlinDeclarationProviderMerger { +class DeclarationProviderMerger( + private val project: Project, +) : KotlinDeclarationProviderMerger { override fun merge(providers: List): KotlinDeclarationProvider = providers.mergeSpecificProviders<_, DeclarationProvider>(KotlinCompositeDeclarationProvider.factory) { targetProviders -> val combinedScope = GlobalSearchScope.union(targetProviders.map { it.scope }) @@ -81,13 +83,12 @@ internal abstract class AbstractDeclarationProvider( } override fun findInternalFilesForFacade(facadeFqName: FqName): Collection = - // We don't deserialize libraries from stubs so we can return empty here safely - // We don't take the KaBuiltinsModule into account for simplicity, + // We don't deserialize libraries from stubs so we can return empty here safely + // We don't take the KaBuiltinsModule into account for simplicity, // that means we expect the kotlin stdlib to be included on the project emptyList() - override fun findFilesForFacadeByPackage(packageFqName: FqName): Collection = - ktFilesForPackage(packageFqName).toList() + override fun findFilesForFacadeByPackage(packageFqName: FqName): Collection = ktFilesForPackage(packageFqName).toList() override fun findFilesForScript(scriptFqName: FqName): Collection = ktFilesForPackage(scriptFqName).mapNotNull { it.script }.toList() @@ -98,8 +99,7 @@ internal abstract class AbstractDeclarationProvider( project.read { PsiTreeUtil.collectElementsOfType(it, KtClassOrObject::class.java).asSequence() } - } - .filter { it.getClassId() == classId } + }.filter { it.getClassId() == classId } .toList() override fun getAllTypeAliasesByClassId(classId: ClassId): Collection = @@ -108,8 +108,7 @@ internal abstract class AbstractDeclarationProvider( project.read { PsiTreeUtil.collectElementsOfType(it, KtTypeAlias::class.java).asSequence() } - } - .filter { it.getClassId() == classId } + }.filter { it.getClassId() == classId } .toList() override fun getClassLikeDeclarationByClassId(classId: ClassId): KtClassLikeDeclaration? = @@ -126,11 +125,11 @@ internal abstract class AbstractDeclarationProvider( ktFilesForPackage(callableId.packageName) .flatMap { project.read { - PsiTreeUtil.collectElementsOfType(it, KtNamedFunction::class.java) + PsiTreeUtil + .collectElementsOfType(it, KtNamedFunction::class.java) .asSequence() } - } - .filter { it.isTopLevel } + }.filter { it.isTopLevel } .filter { it.nameAsName == callableId.callableName } .toList() @@ -138,11 +137,11 @@ internal abstract class AbstractDeclarationProvider( ktFilesForPackage(packageFqName) .flatMap { project.read { - PsiTreeUtil.collectElementsOfType(it, KtClassLikeDeclaration::class.java) + PsiTreeUtil + .collectElementsOfType(it, KtClassLikeDeclaration::class.java) .asSequence() } - } - .filter { it.isTopLevelKtOrJavaMember() } + }.filter { it.isTopLevelKtOrJavaMember() } .mapNotNull { it.nameAsName } .toSet() @@ -150,11 +149,11 @@ internal abstract class AbstractDeclarationProvider( ktFilesForPackage(packageFqName) .flatMap { project.read { - PsiTreeUtil.collectElementsOfType(it, KtCallableDeclaration::class.java) + PsiTreeUtil + .collectElementsOfType(it, KtCallableDeclaration::class.java) .asSequence() } - } - .filter { it.isTopLevelKtOrJavaMember() } + }.filter { it.isTopLevelKtOrJavaMember() } .mapNotNull { it.nameAsName } .toSet() @@ -164,8 +163,7 @@ internal abstract class AbstractDeclarationProvider( project.read { PsiTreeUtil.collectElementsOfType(it, KtProperty::class.java).asSequence() } - } - .filter { it.isTopLevel } + }.filter { it.isTopLevel } .filter { it.nameAsName == callableId.callableName } .toList() } @@ -173,16 +171,16 @@ internal abstract class AbstractDeclarationProvider( internal class DeclarationProvider( val scope: GlobalSearchScope, project: Project, - private val index: KtSymbolIndex + private val index: KtSymbolIndex, ) : AbstractDeclarationProvider(project) { - override val hasSpecificCallablePackageNamesComputation = false override val hasSpecificClassifierPackageNamesComputation = false - override fun ktFilesForPackage(fqName: FqName): Sequence { - return index.filesForPackage(fqName.asString()) + @OptIn(ResolutionSideKtFileAccess::class) + override fun ktFilesForPackage(fqName: FqName): Sequence = + index + .filesForPackage(fqName.asString()) .mapNotNull { VirtualFileManager.getInstance().findFileByNioPath(Paths.get(it.filePath)) } .filter { it in scope } .mapNotNull { index.getKtFile(it) } - } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt index df24ee2918..bd69c5758c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.services import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex +import com.itsaky.androidide.lsp.kotlin.compiler.index.ResolutionSideKtFileAccess import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence import com.itsaky.androidide.lsp.kotlin.compiler.modules.isSourceModule @@ -32,7 +33,9 @@ import org.jetbrains.kotlin.psi.psiUtil.contains import org.jetbrains.kotlin.psi.psiUtil.getImportedSimpleNameByImportAlias import org.jetbrains.kotlin.psi.psiUtil.getSuperNames -internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsProvider { +internal class DirectInheritorsProvider : + KtLspService, + KotlinDirectInheritorsProvider { private lateinit var index: KtSymbolIndex private lateinit var modules: List private lateinit var project: Project @@ -44,7 +47,7 @@ internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsPro project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.project = project this.index = index @@ -55,7 +58,7 @@ internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsPro override fun getDirectKotlinInheritors( ktClass: KtClass, scope: GlobalSearchScope, - includeLocalInheritors: Boolean + includeLocalInheritors: Boolean, ): Iterable { computeIndex() @@ -75,41 +78,48 @@ internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsPro } // Let's say this operation is not frequently called, if we discover it's not the case we should cache it + @OptIn(ResolutionSideKtFileAccess::class) private fun computeIndex() { classesBySupertypeName.clear() inheritableTypeAliasesByAliasedName.clear() modules .asFlatSequence() - .filter { it.isSourceModule }.flatMap { it.computeFiles(extended = true) } + .filter { it.isSourceModule } + .flatMap { it.computeFiles(extended = true) } .mapNotNull { index.getKtFile(it) } .forEach { ktFile -> - ktFile.accept(object : KtTreeVisitorVoid() { - override fun visitClassOrObject(classOrObject: KtClassOrObject) { - classOrObject.getSuperNames().forEach { superName -> - classesBySupertypeName - .computeIfAbsent(Name.identifier(superName)) { mutableSetOf() } - .add(classOrObject) + ktFile.accept( + object : KtTreeVisitorVoid() { + override fun visitClassOrObject(classOrObject: KtClassOrObject) { + classOrObject.getSuperNames().forEach { superName -> + classesBySupertypeName + .computeIfAbsent(Name.identifier(superName)) { mutableSetOf() } + .add(classOrObject) + } + super.visitClassOrObject(classOrObject) } - super.visitClassOrObject(classOrObject) - } - override fun visitTypeAlias(typeAlias: KtTypeAlias) { - val typeElement = typeAlias.getTypeReference()?.typeElement ?: return + override fun visitTypeAlias(typeAlias: KtTypeAlias) { + val typeElement = typeAlias.getTypeReference()?.typeElement ?: return - findInheritableSimpleNames(typeElement).forEach { expandedName -> - inheritableTypeAliasesByAliasedName - .computeIfAbsent(Name.identifier(expandedName)) { mutableSetOf() } - .add(typeAlias) - } + findInheritableSimpleNames(typeElement).forEach { expandedName -> + inheritableTypeAliasesByAliasedName + .computeIfAbsent(Name.identifier(expandedName)) { mutableSetOf() } + .add(typeAlias) + } - super.visitTypeAlias(typeAlias) - } - }) + super.visitTypeAlias(typeAlias) + } + }, + ) } } - private fun calculateAliases(aliasedName: Name, aliases: MutableSet) { + private fun calculateAliases( + aliasedName: Name, + aliases: MutableSet, + ) { inheritableTypeAliasesByAliasedName[aliasedName].orEmpty().forEach { alias -> val aliasName = alias.nameAsSafeName val isNewAliasName = aliases.add(aliasName) @@ -166,7 +176,13 @@ private fun findInheritableSimpleNames(typeElement: KtTypeElement): List } } } - is KtNullableType -> typeElement.innerType?.let(::findInheritableSimpleNames) ?: emptyList() - else -> emptyList() + + is KtNullableType -> { + typeElement.innerType?.let(::findInheritableSimpleNames) ?: emptyList() + } + + else -> { + emptyList() + } } -} \ No newline at end of file +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/AdvancedKotlinEditHandler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/AdvancedKotlinEditHandler.kt index 2fd8342f52..72903f627e 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/AdvancedKotlinEditHandler.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/AdvancedKotlinEditHandler.kt @@ -1,5 +1,6 @@ package com.itsaky.androidide.lsp.kotlin.completion +import com.itsaky.androidide.lsp.kotlin.compiler.index.UnpinnedKtFileAccess import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext import com.itsaky.androidide.lsp.models.CompletionItem import io.github.rosemoe.sora.text.Content @@ -10,20 +11,22 @@ import org.slf4j.LoggerFactory internal abstract class AdvancedKotlinEditHandler( protected val analysisContext: AnalysisContext, ) : BaseKotlinEditHandler() { - companion object { private val logger = LoggerFactory.getLogger(AdvancedKotlinEditHandler::class.java) } + @OptIn(UnpinnedKtFileAccess::class) override fun performEdits( item: CompletionItem, editor: CodeEditor, text: Content, line: Int, column: Int, - index: Int + index: Int, ) { - val managedFile = analysisContext.env.ktSymbolIndex.getCurrentKtFileIfPresent(analysisContext.file) + // PSI-only, on the UI thread, after completion has already returned: there is no analysis to + // keep coherent, and pinning here would block the UI thread on a refresh. + val managedFile = analysisContext.env.ktSymbolIndex.peekLiveKtFile(analysisContext.file) if (managedFile == null) { logger.error("Unable to perform edit. File not open.") return @@ -42,6 +45,6 @@ internal abstract class AdvancedKotlinEditHandler( abstract fun performEdits( ktFile: KtFile, editor: CodeEditor, - item: CompletionItem + item: CompletionItem, ) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index 74e11ba22a..bb18859145 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -8,9 +8,7 @@ import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext import com.itsaky.androidide.lsp.kotlin.utils.ContextKeywords import com.itsaky.androidide.lsp.kotlin.utils.ModifierFilter @@ -57,7 +55,6 @@ import org.jetbrains.kotlin.analysis.api.symbols.name import org.jetbrains.kotlin.analysis.api.symbols.receiverType import org.jetbrains.kotlin.analysis.api.types.KaClassType import org.jetbrains.kotlin.analysis.api.types.KaType -import org.jetbrains.kotlin.analysis.low.level.api.fir.util.originalKtFile import org.jetbrains.kotlin.com.intellij.psi.PsiElement import org.jetbrains.kotlin.name.ClassId import org.jetbrains.kotlin.name.FqName @@ -143,105 +140,118 @@ internal fun codeComplete(params: CompletionParams): CompletionResult { */ context(env: CompilationEnvironment) internal fun doComplete(params: CompletionParams): CompletionResult { - val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).get() - if (ktFile == null) { - logger.warn("File {} is not open", params.file) - return CompletionResult.EMPTY - } - - // Completion still parses its own placeholder variant (text differs), anchored to the - // current file. - val originalText = ktFile.text - val requestPosition = params.position - val completionOffset = requestPosition.requireIndex() - val prefix = params.requirePrefix() - val partial = partialIdentifier(prefix) + val result = + env.ktSymbolIndex.withLiveKtFile(params.file) { live -> + if (live.isStale) { + /* + * Joining another feature's scope hands over its text, which can be older than the buffer the + * request's offset was measured against. Splicing the placeholder at that offset would insert + * it in the wrong place, and past the end of the older text it throws outright. The next + * keystroke's completion supersedes this one anyway. + */ + logger.debug("skipping completion for {}: pinned text is behind the buffer", params.file) + return@withLiveKtFile CompletionResult.EMPTY + } - abortIfCancelled() + // Completion still parses its own placeholder variant (text differs), anchored to the + // current file. + val originalText = live.read { it.text } + val requestPosition = params.position + /* + * Clamped because the guard above compares the pin to the current document version, not to the + * version params.position was measured against - CompletionParams carries none. A request + * measured before a deletion, processed against a pin resolved after it, has an offset past the + * end of this text, and the splice below would throw rather than return no items. + */ + val completionOffset = requestPosition.requireIndex().coerceAtMost(originalText.length) + val prefix = params.requirePrefix() + val partial = partialIdentifier(prefix) - // insert placeholder to fix broken trees - val textWithPlaceholder = - buildString { - append(originalText, 0, completionOffset) - append(KT_COMPLETION_PLACEHOLDER) - append(originalText, completionOffset, originalText.length) - } + abortIfCancelled() - val completionKtFile = - env.project.read { - env.parser - .createFile( - fileName = params.file.name, - text = textWithPlaceholder, - ).apply { - originalFile = ktFile - originalKtFile = ktFile + // insert placeholder to fix broken trees + val textWithPlaceholder = + buildString { + append(originalText, 0, completionOffset) + append(KT_COMPLETION_PLACEHOLDER) + append(originalText, completionOffset, originalText.length) } - } - abortIfCancelled() - - // Use the request-scoped checker on params, not the global Lookup: Lookup holds one ICancelChecker - // updated per request, so with concurrent completions an older request could read a newer request's - // checker and never observe its own cancellation. Fall back to Lookup only for a NOOP checker (tests). - val delegate = - params.cancelChecker.takeUnless { it === ICancelChecker.NOOP } - ?: Lookup.getDefault().lookup(ICancelChecker::class.java) - ?: ICancelChecker.NOOP - val cancelChecker = ScheduledCancelChecker(delegate) - currentCancelChecker.set(cancelChecker) - - return try { - env.project.read { abortIfCancelled() - analyzeMaybeDangling(completionKtFile, AnalysisPriority.INTERACTIVE, cancelChecker) { - val ctx = - resolveAnalysisContext( - env = env, - file = params.file, - ktFile = completionKtFile, - offset = completionOffset, - partial = partial, - ) - - if (ctx == null) { - logger.error( - "Unable to determine context at offset {} in file {}", - completionOffset, - params.file, - ) - return@analyzeMaybeDangling CompletionResult.EMPTY - } + /* + * Use the request-scoped checker on params, not the global Lookup: Lookup holds one ICancelChecker + * updated per request, so with concurrent completions an older request could read a newer request's + * checker and never observe its own cancellation. Fall back to Lookup only for a NOOP checker (tests). + */ + val delegate = + params.cancelChecker.takeUnless { it === ICancelChecker.NOOP } + ?: Lookup.getDefault().lookup(ICancelChecker::class.java) + ?: ICancelChecker.NOOP + val cancelChecker = ScheduledCancelChecker(delegate) + currentCancelChecker.set(cancelChecker) + + try { + live.analyzingVariant( + name = params.file.name, + text = textWithPlaceholder, + priority = AnalysisPriority.INTERACTIVE, + cancelChecker = cancelChecker, + ) { completionKtFile -> + abortIfCancelled() + + val ctx = + resolveAnalysisContext( + env = env, + file = params.file, + ktFile = completionKtFile, + offset = completionOffset, + partial = partial, + ) + + if (ctx == null) { + logger.error( + "Unable to determine context at offset {} in file {}", + completionOffset, + params.file, + ) + return@analyzingVariant CompletionResult.EMPTY + } - abortIfCancelled() - context(ctx) { - val items = mutableListOf() - val completionContext = determineCompletionContext(ctx.psiElement) - when (completionContext) { - CompletionContext.Scope -> { - collectScopeCompletions(to = items) + abortIfCancelled() + context(ctx) { + val items = mutableListOf() + val completionContext = determineCompletionContext(ctx.psiElement) + when (completionContext) { + CompletionContext.Scope -> { + collectScopeCompletions(to = items) + } + + CompletionContext.Member -> { + collectMemberCompletions(to = items) + } } - CompletionContext.Member -> { - collectMemberCompletions(to = items) - } + CompletionResult(items) } - - CompletionResult(items) } + } catch (e: Throwable) { + if (e.isCancellation()) { + throw e + } + + logger.warn("An error occurred while computing completions for {}", params.file, e) + CompletionResult.EMPTY + } finally { + currentCancelChecker.remove() } } - } catch (e: Throwable) { - if (e.isCancellation()) { - throw e - } - logger.warn("An error occurred while computing completions for {}", params.file, e) + if (result == null) { + logger.warn("File {} is not open", params.file) return CompletionResult.EMPTY - } finally { - currentCancelChecker.remove() } + return result } context(ctx: AnalysisContext) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt index 901b5c0c04..d1f6a4bc6c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt @@ -3,8 +3,6 @@ package com.itsaky.androidide.lsp.kotlin.diagnostic import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.toRange import com.itsaky.androidide.lsp.models.DiagnosticItem import com.itsaky.androidide.lsp.models.DiagnosticResult @@ -68,77 +66,92 @@ private fun doAnalyze( file: Path, cancelChecker: ICancelChecker, ): DiagnosticResult { - val ktFile = env.ktSymbolIndex.getCurrentKtFile(file).get() - if (ktFile == null) { - logger.warn("File {} is not accessible", file) - return DiagnosticResult.NO_UPDATE - } - - // Diagnostics yield to completion but preempt indexing. The wrapped checker turns a scheduler - // preemption into an AnalysisPreemptedException, which CompilationEnvironment's fileAnalyzer catches - // to re-schedule this run once the higher-priority work finishes. + /* + * Diagnostics yield to completion but preempt indexing. The wrapped checker turns a scheduler + * preemption into an AnalysisPreemptedException, which CompilationEnvironment's fileAnalyzer catches + * to re-schedule this run once the higher-priority work finishes. + */ val checker = ScheduledCancelChecker(cancelChecker) - val diagnostics = - env.project.read { - buildList { - PsiTreeUtil - .collectElementsOfType(ktFile, PsiErrorElement::class.java) - .forEach { errorElement -> - checker.abortIfCancelled() - add( - diagnosticItem( - file = ktFile, - message = errorElement.errorDescription, - range = errorElement.textRange, - severity = DiagnosticSeverity.ERROR, - ), - ) - } - - // analyzeMaybeDangling installs a CancelCheckerProgressIndicator, so this is cancellable - // mid-`analyze`: it aborts at the compiler's internal checkCanceled() once `checker` reports - // preemption/cancellation. (Previously this analysis was not cancellable at all.) - analyzeMaybeDangling(ktFile, AnalysisPriority.DIAGNOSTICS, checker) { - ktFile - .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) - .forEach { diagnostic -> - checker.abortIfCancelled() - // Extract plain data while still inside the analyze context; never let - // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). - val action = - when (diagnostic) { - is KaFirDiagnostic.UnresolvedReference -> { - DiagnosticAction.ResolveReference( - diagnostic.reference, - ) - } - - is KaFirDiagnostic.UnsafeCall -> { - DiagnosticAction.NullSafetyFix + var superseded = false + val result = + env.ktSymbolIndex.withLiveKtFile(file) { live -> + val diagnostics = + live.analyzing(AnalysisPriority.DIAGNOSTICS, checker) { ktFile -> + buildList { + PsiTreeUtil + .collectElementsOfType(ktFile, PsiErrorElement::class.java) + .forEach { errorElement -> + checker.abortIfCancelled() + add( + diagnosticItem( + file = ktFile, + message = errorElement.errorDescription, + range = errorElement.textRange, + severity = DiagnosticSeverity.ERROR, + ), + ) + } + + /* + * analyzeMaybeDangling installs a CancelCheckerProgressIndicator, so this is cancellable + * mid-`analyze`: it aborts at the compiler's internal checkCanceled() once `checker` reports + * preemption/cancellation. (Previously this analysis was not cancellable at all.) + */ + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .forEach { diagnostic -> + checker.abortIfCancelled() + // Extract plain data while still inside the analyze context; never let + // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). + val action = + when (diagnostic) { + is KaFirDiagnostic.UnresolvedReference -> { + DiagnosticAction.ResolveReference( + diagnostic.reference, + ) + } + + is KaFirDiagnostic.UnsafeCall -> { + DiagnosticAction.NullSafetyFix + } + + else -> { + DiagnosticAction.None + } } - else -> { - DiagnosticAction.None - } - } - - add( - diagnostic.toDiagnosticItem().apply { - extra = KotlinDiagnosticExtra(env, action) - }, - ) - } + add( + diagnostic.toDiagnosticItem().apply { + extra = KotlinDiagnosticExtra(env, action) + }, + ) + } + } } + + if (live.isStale) { + // The document moved on while this ran, so these diagnostics describe text the user has + // already replaced. Publishing them would paint the editor with stale squiggles. + superseded = true + null + } else { + logger.info("Found {} diagnostics", diagnostics.size) + DiagnosticResult(file = file, diagnostics = diagnostics) } } - logger.info("Found {} diagnostics", diagnostics.size) + if (result != null) { + return result + } - return DiagnosticResult( - file = file, - diagnostics = diagnostics, - ) + if (superseded) { + logger.debug("dropping superseded diagnostics for {}", file) + env.fileAnalyzer.schedule(file) + } else { + logger.warn("File {} is not accessible", file) + } + return DiagnosticResult.NO_UPDATE } private fun KaDiagnosticWithPsi<*>.toDiagnosticItem(): DiagnosticItem { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt index afadb2c4ad..a4ec1bac93 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt @@ -5,13 +5,11 @@ import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedExcept import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation import com.itsaky.androidide.lsp.kotlin.compiler.modules.isSourceModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.compiler.services.ProjectStructureProvider import com.itsaky.androidide.lsp.kotlin.utils.rangeOf import com.itsaky.androidide.lsp.models.ReferenceParams @@ -20,7 +18,6 @@ import com.itsaky.androidide.models.Location import com.itsaky.androidide.models.Range import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.projects.FileManager -import kotlinx.coroutines.future.await import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.platform.projectStructure.KotlinModuleDependentsProvider import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol @@ -82,7 +79,7 @@ internal class SearchPlan( /** * Computes the usage result for [params]. * - * Structured so that no lock spans the whole search (R9): the target is resolved under one short + * Structured so that no lock spans the whole search: the target is resolved under one short * `project.read`, candidate selection holds nothing across the pass (`computeFiles` takes `project.read` * per file, for one path lookup), and each candidate then takes its own read lock and analysis session. * A whole-workspace search holding either for its full duration would block index refresh (which needs @@ -158,21 +155,24 @@ internal suspend fun planAt(params: ReferenceParams): SearchPlan? { val offset = params.position.requireIndex() return retryingOnPreemption(params.cancelChecker, "Usage search target for ${params.file}") { cancelChecker -> - // Awaited per attempt and outside project.read, exactly as in findDefinitionAt: the refresh this - // waits on needs project.write, and a preemption invalidates the KtFile it returned. - val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await() - if (ktFile == null) { - logger.warn("File {} cannot be loaded for usage search", params.file) - null - } else { - cancelChecker.abortIfCancelled() - env.project.read { - val target = targetAtCaret(ktFile, offset) ?: return@read null - analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { - planFor(target) + // Pinned per attempt, exactly as in findDefinitionAt: a preemption refreshes the live PSI, so the + // instance the previous attempt held is no longer the one the file resolves to. + var pinned = false + val plan = + env.ktSymbolIndex.withLiveKtFileAsync(params.file) { live -> + pinned = true + cancelChecker.abortIfCancelled() + live.read { ktFile -> + val target = targetAtCaret(ktFile, offset) ?: return@read null + live.analyzing(AnalysisPriority.COMMAND, cancelChecker) { + planFor(target) + } } } + if (!pinned) { + logger.warn("File {} cannot be loaded for usage search", params.file) } + plan } } @@ -387,7 +387,7 @@ internal fun candidateFiles( .asSequence() .filter { it.isSourceModule } .flatMap { it.computeFiles(extended = true) } - // A source module's files are .kt *and* .java, and `ktFileFor` rejects a non-Kotlin path + // A source module's files are .kt *and* .java, and acquisition rejects a non-Kotlin path // anyway (searching .java is a non-goal). Dropping them here, on the extension alone, // stops a Java-heavy workspace spending most of the prefilter's I/O - the part the user // waits on - reading files whose result is already known to be nothing. The extensions @@ -449,8 +449,8 @@ private fun Char.isIdentifierChar(): Boolean = isLetterOrDigit() || this == '_' /** * Every usage of [plan]'s target in the file at [path]. * - * One analysis session per file, so a preemption costs this file rather than the whole search, and the - * live-PSI await stays outside `project.read` (R9). + * One analysis session per file, so a preemption costs this file rather than the whole search. The pin + * covers both the open case (the live editor buffer) and the closed one (the indexed on-disk instance). */ context(env: AbstractCompilationEnvironment) private suspend fun usagesIn( @@ -460,32 +460,35 @@ private suspend fun usagesIn( ): List = try { retryingOnPreemption(delegate, "Usage search in $path") { cancelChecker -> - val ktFile = ktFileFor(path) - if (ktFile == null) { - logger.debug("Skipping candidate {}: no PSI", path) - emptyList() - } else { - env.project.read { - // The name filter is pure PSI, so it runs before the analysis session opens. A text - // prefilter hit whose only mention is a comment or a string literal must not cost an - // analysis-lock acquisition, a FIR session and a match-set restore to rule out - and on a - // short, common name most candidates are exactly that. + env.ktSymbolIndex.withLiveKtFileAsync(path) { live -> + live.read { ktFile -> + /* + * The name filter is pure PSI, so it runs before the analysis session opens. A text + * prefilter hit whose only mention is a comment or a string literal must not cost an + * analysis-lock acquisition, a FIR session and a match-set restore to rule out - and on a + * short, common name most candidates are exactly that. + */ val named = namedReferences(ktFile, plan.simpleName, cancelChecker) if (named.isEmpty()) { emptyList() } else { - analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { + live.analyzing(AnalysisPriority.COMMAND, cancelChecker) { matchingReferences(named, plan, ktFile, path, cancelChecker) } } } + } ?: run { + logger.debug("Skipping candidate {}: no PSI", path) + emptyList() } } } catch (e: AnalysisPreemptedException) { - // A preemption that outlived retryingOnPreemption's single retry is keystroke-driven work winning - // the lock, not the user cancelling. Rethrowing it would discard every location collected so far - // and report "no references" for a symbol with plenty, so it costs this file like any other - // failure. Genuine cancellation still propagates below (R12). + /* + * A preemption that outlived retryingOnPreemption's single retry is keystroke-driven work winning + * the lock, not the user cancelling. Rethrowing it would discard every location collected so far + * and report "no references" for a symbol with plenty, so it costs this file like any other + * failure. Genuine cancellation still propagates below. + */ logger.debug("Usage search gave up on candidate {}: preempted twice", path) emptyList() } catch (e: Throwable) { @@ -495,22 +498,6 @@ private suspend fun usagesIn( emptyList() } -/** - * PSI for a candidate file: refreshed to the live editor buffer when the file is open, the indexed - * on-disk instance otherwise. - * - * The open case must be awaited here, outside `project.read`, because the refresh it waits on needs - * `project.write`. `getKtFile` cannot do it - it runs under `project.read` inside Analysis API - * services, so it only ever peeks the live cache. - */ -context(env: AbstractCompilationEnvironment) -private suspend fun ktFileFor(path: Path): KtFile? = - if (FileManager.isActive(path)) { - env.ktSymbolIndex.getCurrentKtFile(path).await() - } else { - env.ktSymbolIndex.getKtFile(path) - } - /** * The simple-name references in [ktFile] written as [simpleName]. * diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.kt index da360c5536..c58258ee28 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.kt @@ -3,11 +3,9 @@ package com.itsaky.androidide.lsp.kotlin.navigation import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.rangeOf import com.itsaky.androidide.lsp.kotlin.utils.toRange import com.itsaky.androidide.lsp.models.DefinitionParams @@ -15,7 +13,6 @@ import com.itsaky.androidide.lsp.models.DefinitionResult import com.itsaky.androidide.models.Location import com.itsaky.androidide.models.Range import com.itsaky.androidide.progress.ICancelChecker -import kotlinx.coroutines.future.await import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.components.containingDeclaration import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull @@ -179,9 +176,7 @@ private fun locationOfPsi(declaration: PsiElement): Location? { /** * Computes the definition result for [params]. * - * Mirrors `doSignatureHelp`: the live-PSI await happens outside `project.read`, because the refresh - * it waits on needs `project.write` and awaiting it under the read lock would deadlock. Every - * failure short of cancellation collapses to an empty result, which the editor renders as + * Every failure short of cancellation collapses to an empty result, which the editor renders as * "Definition not found". * * The context is [AbstractCompilationEnvironment] rather than the concrete `CompilationEnvironment` @@ -200,34 +195,36 @@ internal suspend fun findDefinitionAt(params: DefinitionParams): DefinitionResul return try { val offset = params.position.requireIndex() - // Navigation is a user-invoked command: AnalysisPriority.COMMAND preempts background - // diagnostics/indexing but yields to keystroke-driven completion, and is never discarded by - // another command. It can still be preempted by INTERACTIVE, so it retries once (see - // retryingOnPreemption, and ADR 0011). params.cancelChecker is request-scoped - // (CancellableRequestParams), so it is the delegate the per-attempt checker wraps. + /* + * Navigation is a user-invoked command: AnalysisPriority.COMMAND preempts background + * diagnostics/indexing but yields to keystroke-driven completion, and is never discarded by + * another command. It can still be preempted by INTERACTIVE, so it retries once (see + * retryingOnPreemption, and ADR 0011). params.cancelChecker is request-scoped + * (CancellableRequestParams), so it is the delegate the per-attempt checker wraps. + */ val locations = retryingOnPreemption(params.cancelChecker, "Definition lookup for ${params.file}") { cancelChecker -> - // Awaited per attempt, not once: whatever preempted the first attempt also refreshed the - // live PSI, unregistering the KtFile that attempt held, and analyzing it again would fail. - // - // Safe to await a (possibly blocking) refresh here: this runs outside any project.read/write - // block, so it can't deadlock against the refresh's project.write. Refreshed to the open - // document's current version, so the caret offset and the PSI it indexes into come from the - // same text - a stale snapshot points at the wrong element. (params.position is fixed by the - // request, so a retry after the user typed can still be one edit behind; that resolves to - // the wrong element or to nothing, never to a crash.) - val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await() - if (ktFile == null) { - logger.warn("File {} cannot be loaded for definition lookup", params.file) - emptyList() - } else { + /* + * Pinned per attempt, not once: whatever preempted the first attempt also refreshed the + * live PSI, unregistering the KtFile that attempt held, and analyzing it again would fail. + * + * The pinned text is not guaranteed to be the buffer's: joining another feature's open scope + * hands over its instance, however old, and params.position is fixed by the request anyway, + * so the caret offset can index into text one or more edits behind. Deliberately tolerated + * here - the worst outcome is resolving to the wrong element or to nothing, never a bad edit. + * Sites that emit edits check LiveKtFile.isStale instead. + */ + env.ktSymbolIndex.withLiveKtFileAsync(params.file) { live -> cancelChecker.abortIfCancelled() - env.project.read { + live.read { ktFile -> val element = referenceAtCaret(ktFile, offset) ?: return@read emptyList() - analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { + live.analyzing(AnalysisPriority.COMMAND, cancelChecker, useSite = element) { definitionLocations(element, cancelChecker) } } + } ?: run { + logger.warn("File {} cannot be loaded for definition lookup", params.file) + emptyList() } } logger.debug("Definition result for {}: {} location(s)", params.file, locations.size) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/signaturehelp/KotlinSignatureHelp.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/signaturehelp/KotlinSignatureHelp.kt index e2d151cdeb..c7d40c881f 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/signaturehelp/KotlinSignatureHelp.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/signaturehelp/KotlinSignatureHelp.kt @@ -4,13 +4,10 @@ import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.models.SignatureHelp import com.itsaky.androidide.lsp.models.SignatureHelpParams import com.itsaky.androidide.lsp.models.SignatureInformation -import kotlinx.coroutines.future.await import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.resolution.KaFunctionCall import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull @@ -96,30 +93,30 @@ internal suspend fun doSignatureHelp(params: SignatureHelpParams): SignatureHelp return SignatureHelp.empty() } - // Safe to await a (possibly blocking) refresh here: this runs outside any project.read/write - // block, so it can't deadlock against the refresh's project.write (unlike KtSymbolIndex.getKtFile). - val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await() - if (ktFile == null) { - logger.warn("File {} is not open", params.file) - return SignatureHelp.empty() - } - - // Signature help is interactive (the user is typing arguments): run at INTERACTIVE priority so it - // preempts background diagnostics/indexing and is discarded when a newer interactive request wins. - // params.cancelChecker is request-scoped (CancellableRequestParams), so wrap it directly — no - // global Lookup fallback needed. + /* + * Signature help is interactive (the user is typing arguments): run at INTERACTIVE priority so it + * preempts background diagnostics/indexing and is discarded when a newer interactive request wins. + * params.cancelChecker is request-scoped (CancellableRequestParams), so wrap it directly - no + * global Lookup fallback needed. + */ val cancelChecker = ScheduledCancelChecker(params.cancelChecker) return try { val offset = params.position.requireIndex() cancelChecker.abortIfCancelled() val result = - env.project.read { - val call = findEnclosingCall(ktFile, offset) ?: return@read SignatureHelp.empty() - analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { - buildSignatureHelp(call, offset) + env.ktSymbolIndex.withLiveKtFileAsync(params.file) { live -> + live.read { ktFile -> + val call = findEnclosingCall(ktFile, offset) ?: return@read SignatureHelp.empty() + live.analyzing(AnalysisPriority.INTERACTIVE, cancelChecker) { + buildSignatureHelp(call, offset) + } } } + if (result == null) { + logger.warn("File {} is not open", params.file) + return SignatureHelp.empty() + } logger.debug( "Signature help result for {}: {} signature(s), activeSignature={}, activeParameter={}", params.file, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt index 53a61777ed..5af774574e 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt @@ -3,8 +3,6 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling -import com.itsaky.androidide.lsp.kotlin.compiler.read import org.slf4j.LoggerFactory import java.nio.file.Path import kotlin.coroutines.cancellation.CancellationException @@ -14,12 +12,9 @@ private val logger = LoggerFactory.getLogger("ExtractMethodPlanner") /** * Computes the whole [ExtractMethodPlan] in one background analysis pass. * - * The current `KtFile` is fetched *before* entering [read] -- blocking on `getCurrentKtFile(...).get()` - * inside `project.read` deadlocks. - * * Anything thrown in this pipeline degrades to a refusal plus a log line: the action framework * catches only `IllegalArgumentException` and this runs on a scope with no exception handler, so an - * uncaught throw would crash the app (R16). Cancellation is the exception -- it is re-thrown, since a + * uncaught throw would crash the app. Cancellation is the exception -- it is re-thrown, since a * cancelled action has no result to report and the coroutine machinery already handles it. * * Everything that is not "your selection is not one region" refuses with [ExtractionRefusal.CouldNotAnalyse]: @@ -34,47 +29,58 @@ internal fun buildExtractMethodPlan( cancelChecker: ScheduledCancelChecker, ): ExtractMethodPlan = runCatching { - val ktFile = - env.ktSymbolIndex.getCurrentKtFile(nioPath).get() - ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + if (live.isStale) { + /* + * Joining another feature's scope hands over its text, which can be older than the buffer. + * The caller stamps `documentVersion` from the live buffer, so the apply-time version guard + * would compare an honest stamp against text one edit behind and pass - and offsets computed + * here would replace the wrong span. Refusing is the only safe answer. + */ + logger.debug("refusing extract-method plan for {}: pinned text is behind the buffer", nioPath) + return@withLiveKtFile ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + } - env.project.read { - val fileText = ktFile.text - val region = - resolveExtractionRegion(ktFile, selectionStart, selectionEnd) - ?: return@read ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion, fileText, documentVersion) + live.read { ktFile -> + val fileText = ktFile.text + val region = + resolveExtractionRegion(ktFile, selectionStart, selectionEnd) + ?: return@read ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion, fileText, documentVersion) - analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { - val results = - when (region) { - is ExtractionRegion.Expressions -> { - region.candidates.map { buildCandidate(listOf(it), isExpression = true, fileText = fileText) } - } + live.analyzing(AnalysisPriority.INTERACTIVE, cancelChecker) { + val results = + when (region) { + is ExtractionRegion.Expressions -> { + region.candidates.map { buildCandidate(listOf(it), isExpression = true, fileText = fileText) } + } - is ExtractionRegion.Statements -> { - listOf(buildCandidate(region.statements, isExpression = false, fileText = fileText)) + is ExtractionRegion.Statements -> { + listOf(buildCandidate(region.statements, isExpression = false, fileText = fileText)) + } } + + val candidates = results.filterIsInstance().map { it.candidate } + if (candidates.isEmpty()) { + /* + * The innermost region is the one the user pointed at, so its reason is the one to show. + * A region with no reason at all cannot happen; if it does, saying nothing useful beats + * blaming the selection. + */ + val refusal = + results.filterIsInstance().firstOrNull()?.refusal + ?: ExtractionRefusal.CouldNotAnalyse + return@analyzing ExtractMethodPlan.refused(refusal, fileText, documentVersion) } - val candidates = results.filterIsInstance().map { it.candidate } - if (candidates.isEmpty()) { - // The innermost region is the one the user pointed at, so its reason is the one to show. - // A region with no reason at all cannot happen; if it does, saying nothing useful beats - // blaming the selection. - val refusal = - results.filterIsInstance().firstOrNull()?.refusal - ?: ExtractionRefusal.CouldNotAnalyse - return@analyzeMaybeDangling ExtractMethodPlan.refused(refusal, fileText, documentVersion) + ExtractMethodPlan( + fileText = fileText, + documentVersion = documentVersion, + candidates = candidates, + refusal = null, + ) } - - ExtractMethodPlan( - fileText = fileText, - documentVersion = documentVersion, - candidates = candidates, - refusal = null, - ) } - } + } ?: ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) }.getOrElse { error -> if (error is CancellationException) throw error logger.warn("Failed to build extract-method plan for {}", nioPath, error) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt index 1270709429..99d83380e9 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt @@ -3,8 +3,6 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker -import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling -import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.renderName import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.KaSession @@ -24,9 +22,6 @@ private val logger = LoggerFactory.getLogger("ExtractVariablePlanner") /** * Computes the whole [ExtractionPlan] in one background analysis pass. * - * The current [KtFile] is fetched *before* entering [read] -- blocking on - * `getCurrentKtFile(...).get()` inside `project.read` deadlocks. - * * Returns an empty plan both when there is genuinely nothing to extract and whenever anything in * this pipeline throws: the action framework only catches [IllegalArgumentException] and this runs on * a scope with no exception handler, so an uncaught throw would crash the app. Degrading to an empty @@ -41,22 +36,34 @@ internal fun buildExtractionPlan( cancelChecker: ScheduledCancelChecker, ): ExtractionPlan = runCatching { - val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return ExtractionPlan.empty() - env.project.read { - val syntax = candidateExpressionsAt(ktFile, selectionStart, selectionEnd) - if (syntax.expressions.isEmpty()) return@read ExtractionPlan.empty(ktFile.text, documentVersion) - - /* PsiFileImpl.getText() allocates a fresh String each call, so the plan pass reads it once and - * threads it down to every candidate and rung. */ - val fileText = ktFile.text - analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { - ExtractionPlan( - fileText = fileText, - documentVersion = documentVersion, - candidates = syntax.expressions.mapNotNull { candidateFor(it, fileText) }, - ) + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + if (live.isStale) { + /* + * Joining another feature's scope hands over its text, which can be older than the buffer. + * The caller stamps `documentVersion` from the live buffer, so the apply-time version guard + * would compare an honest stamp against text one edit behind and pass - and offsets computed + * here would replace the wrong span. Refusing is the only safe answer. + */ + logger.debug("refusing extract-variable plan for {}: pinned text is behind the buffer", nioPath) + return@withLiveKtFile ExtractionPlan.empty() } - } + + live.read { ktFile -> + val syntax = candidateExpressionsAt(ktFile, selectionStart, selectionEnd) + if (syntax.expressions.isEmpty()) return@read ExtractionPlan.empty(ktFile.text, documentVersion) + + /* PsiFileImpl.getText() allocates a fresh String each call, so the plan pass reads it once and + * threads it down to every candidate and rung. */ + val fileText = ktFile.text + live.analyzing(AnalysisPriority.INTERACTIVE, cancelChecker) { + ExtractionPlan( + fileText = fileText, + documentVersion = documentVersion, + candidates = syntax.expressions.mapNotNull { candidateFor(it, fileText) }, + ) + } + } + } ?: ExtractionPlan.empty() }.getOrElse { error -> logger.warn("Failed to build extract-variable plan for {}", nioPath, error) ExtractionPlan.empty() diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt index 51c864316b..0b9e253042 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt @@ -4,18 +4,37 @@ import com.itsaky.androidide.eventbus.events.editor.ChangeType import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest import com.itsaky.androidide.models.Range import com.itsaky.androidide.projects.FileManager +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.runBlocking +import org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter +import org.jetbrains.kotlin.psi.KtFile import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertNotSame +import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue import org.junit.Test import java.nio.file.Path - +import java.util.Collections +import java.util.IdentityHashMap + +/** + * The current-file cache, exercised through the pin API that is now the only way to acquire an + * instance. The pinned file may not leave its scope, so every identity comparison happens inside a + * `read` block, against a reference obtained from the one door that hands one out. + */ internal class CurrentKtFileCacheTest : KtLspTest() { + companion object { + private const val CONCURRENT_REQUESTS = 16 + } + private val openedPaths = mutableListOf() @After @@ -48,16 +67,40 @@ internal class CurrentKtFileCacheTest : KtLspTest() { ) } + /** + * The instance the current-file cache holds for [path], forcing a refresh first. + * + * [KtSymbolIndex.peekLiveKtFile] is the one door that hands out a reference, which is what lets the + * assertions below be real identity comparisons rather than identity-hash comparisons. + */ + @OptIn(UnpinnedKtFileAccess::class) + private fun currentInstance(path: Path): KtFile? { + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } + return env.ktSymbolIndex.peekLiveKtFile(path) + } + + /** Whether one pin on [path] resolves to [expected], compared inside the block since the pinned file cannot escape. */ + private fun pinResolvesTo( + path: Path, + expected: KtFile?, + ): Boolean? = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.read { it === expected } + } + @Test fun `same version returns same instance`() { createSourceFile("A.kt", "fun a() {}") val path = sourcePath("A.kt") openDocument(path, "fun a() {}") + val instance = currentInstance(path) - val first = env.ktSymbolIndex.getCurrentKtFile(path).get() - val second = env.ktSymbolIndex.getCurrentKtFile(path).get() + val first = pinResolvesTo(path, instance) + val second = pinResolvesTo(path, instance) - assertSame(first, second) + assertNotNull(instance) + assertTrue(first!!) + assertTrue(second!!) } @Test @@ -65,25 +108,56 @@ internal class CurrentKtFileCacheTest : KtLspTest() { createSourceFile("B.kt", "fun b() {}") val path = sourcePath("B.kt") openDocument(path, "fun b() {}") - val v1 = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + val v1 = currentInstance(path) changeDocument(path, "fun b() {}\nfun c() {}", 2) - val v2 = env.ktSymbolIndex.getCurrentKtFile(path).get()!! - - assertNotSame(v1, v2) - assertEquals("fun b() {}\nfun c() {}", v2.text) + val v2 = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.read { (it !== v1) to it.text } + }!! + + assertNotNull(v1) + assertTrue(v2.first) + assertEquals("fun b() {}\nfun c() {}", v2.second) } + /** + * Requests that overlap the very first parse must share it. + * + * The parse runs on the index's own executor, so requests issued before it completes hit an + * *incomplete* cache entry - the window a per-version single-flight exists for. Genuinely + * concurrent, because the only remaining acquisition door blocks until its instance is resolved: + * issuing the requests sequentially would only ever see a settled entry. + * + * Identity is captured into an identity set from inside each block. The references outlive their + * scopes, which is not safe for analysis, but counting distinct instances is all that happens to + * them and it is the only exact way to compare instances acquired on different threads. + */ + @OptIn(UnpinnedKtFileAccess::class) @Test fun `concurrent requests at same version parse once`() { createSourceFile("D.kt", "fun d() {}") val path = sourcePath("D.kt") openDocument(path, "fun d() {}") - val futures = (1..16).map { env.ktSymbolIndex.getCurrentKtFile(path) } - val results = futures.map { it.get() } + val seen = Collections.newSetFromMap(IdentityHashMap()) + val acquired = + runBlocking { + (1..CONCURRENT_REQUESTS) + .map { + async(Dispatchers.Default) { + env.ktSymbolIndex.withLiveKtFileAsync(path) { live -> + live.read { synchronized(seen) { seen.add(it) } } + } + } + }.awaitAll() + } - results.forEach { assertSame(results.first(), it) } + assertEquals(CONCURRENT_REQUESTS, acquired.count { it != null }) + assertEquals(1, seen.size) + // The instance every request resolved to is also the one the cache settled on: a second parse + // would leave the cache holding an instance no pin ever saw. + assertTrue(seen.contains(env.ktSymbolIndex.peekLiveKtFile(path))) } @Test @@ -91,69 +165,80 @@ internal class CurrentKtFileCacheTest : KtLspTest() { createSourceFile("E.kt", "fun e(): Int = 1") val path = sourcePath("E.kt") openDocument(path, "fun e(): Int = 1") - env.ktSymbolIndex.getCurrentKtFile(path).get() + currentInstance(path) changeDocument(path, "fun e(): Int = 1\nfun f(): Int = e()", 2) - val v2 = env.ktSymbolIndex.getCurrentKtFile(path).get()!! - // `f` calling `e` must resolve (no UNRESOLVED_REFERENCE). Keep `.defaultMessage` inside - // `env.analyze {}`: reading a diagnostic outside its analysis session throws - // KaInaccessibleLifetimeOwnerAccessException instead of a clean assertion diff. + // `f` calling `e` must resolve (no UNRESOLVED_REFERENCE). Keep `.defaultMessage` inside the + // analysis: reading a diagnostic outside its session throws KaInaccessibleLifetimeOwnerAccessException + // instead of a clean assertion diff. val diagnosticMessages = - env.analyze(v2) { - v2 - .collectDiagnostics( - org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS, - ).map { it.defaultMessage } + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.analyzing(AnalysisPriority.DIAGNOSTICS, noopCancelChecker()) { ktFile -> + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .map { it.defaultMessage } + } } assertEquals(emptyList(), diagnosticMessages) } @Test - fun `invalidateCurrent then getCurrentKtFile reparses`() { + fun `invalidateCurrent then a new pin reparses`() { createSourceFile("G.kt", "fun g() {}") val path = sourcePath("G.kt") openDocument(path, "fun g() {}") - val first = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + val first = currentInstance(path) env.ktSymbolIndex.invalidateCurrent(path) - val second = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + val reparsed = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.read { it !== first } + }!! - assertNotSame(first, second) + assertNotNull(first) + assertTrue(reparsed) } + @OptIn(UnpinnedKtFileAccess::class) @Test - fun `getCurrentKtFileIfPresent returns the same instance after a completed refresh`() { + fun `peekLiveKtFile returns the same instance after a completed refresh`() { createSourceFile("H.kt", "fun h() {}") val path = sourcePath("H.kt") openDocument(path, "fun h() {}") - val current = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } - val peeked = env.ktSymbolIndex.getCurrentKtFileIfPresent(path) + val peeked = env.ktSymbolIndex.peekLiveKtFile(path) - assertSame(current, peeked) + assertNotNull(peeked) + val samePinnedInstance = env.ktSymbolIndex.withLiveKtFile(path) { live -> live.read { it === peeked } } + assertTrue(samePinnedInstance!!) } + @OptIn(UnpinnedKtFileAccess::class, ResolutionSideKtFileAccess::class) @Test fun `getKtFile returns the current cached instance for an active document instead of reloading from disk`() { createSourceFile("I.kt", "fun i() {}") val path = sourcePath("I.kt") openDocument(path, "fun i() {}") - val current = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } + val current = env.ktSymbolIndex.peekLiveKtFile(path) val viaGetKtFile = env.ktSymbolIndex.getKtFile(path) + assertNotNull(current) assertSame(current, viaGetKtFile) } + @OptIn(UnpinnedKtFileAccess::class) @Test - fun `getCurrentKtFileIfPresent returns null for an active document whose refresh has not been triggered`() { + fun `peekLiveKtFile returns null for an active document whose refresh has not been triggered`() { createSourceFile("J.kt", "fun j() {}") val path = sourcePath("J.kt") openDocument(path, "fun j() {}") - // getCurrentKtFile is deliberately never called, so no refresh has been launched for this path. + // Nothing acquires or refreshes this path, so no refresh has been launched for it. - val peeked = env.ktSymbolIndex.getCurrentKtFileIfPresent(path) + val peeked = env.ktSymbolIndex.peekLiveKtFile(path) assertNull(peeked) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt new file mode 100644 index 0000000000..25a766672e --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt @@ -0,0 +1,218 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.index + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.projects.FileManager +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Test +import java.nio.file.Path +import java.util.concurrent.TimeUnit + +/** + * A pinned path resolves to one `KtFile` instance for the whole scope, whichever door asks. + * + * The pinned instance is deliberately never carried out of a `read` block - the scope guard rejects + * that - so these tests compare identity inside the block, or through an identity hash captured + * inside it. + */ +internal class LiveKtFilePinTest : KtLspTest() { + companion object { + private const val REFRESH_TIMEOUT_SECONDS = 10L + private const val POLL_INTERVAL_MILLIS = 20L + } + + override val enableParserEventSystem = true + + private val openedPaths = mutableListOf() + + @After + fun closeDocs() { + openedPaths.forEach { FileManager.onDocumentClose(DocumentCloseEvent(it)) } + openedPaths.clear() + } + + private val content = + """ + package p + + class Widget + + fun render(a: Int, b: Int): Int = extracted(b, a) + a + + private fun extracted(b: Int, a: Int): Int = b * a + """.trimIndent() + + private fun openDocument(): Path { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + FileManager.onDocumentOpen(DocumentOpenEvent(path, content, 1)) + openedPaths.add(path) + return path + } + + private fun bumpVersion( + path: Path, + version: Int, + ) { + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, content, content, version, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + } + + @OptIn(UnpinnedKtFileAccess::class, ResolutionSideKtFileAccess::class) + @Test + fun `a version bump inside a pin does not install a second instance`() { + val path = openDocument() + + val doorsAgree = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + bumpVersion(path, 2) + /* + * In production this second request is any other acquisition - the refresh scheduler, + * completion, a code action - running while the pinned analysis is still going; unpinned it + * installs a superseding instance for the same path. getKtFile is the resolution-side door + * DeclarationProvider takes. Both must answer with the pinned instance. + */ + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } + val superseding = env.ktSymbolIndex.peekLiveKtFile(path) + live.read { it === superseding && it === env.ktSymbolIndex.getKtFile(path) } + }!! + + assertThat(doorsAgree).isTrue() + } + + @OptIn(ResolutionSideKtFileAccess::class) + @Test + fun `the resolution door keeps the pinned instance after the document is closed`() { + val path = openDocument() + + val doorAgrees = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + /* + * Closing a tab mid-analysis is CompilationEnvironment.onFileClosed, which drops the + * current-file cache for the path. Unpinned, the resolution door then falls through to a + * freshly loaded disk instance while the analysis is still holding the live one. + */ + FileManager.onDocumentClose(DocumentCloseEvent(path)) + openedPaths.remove(path) + env.ktSymbolIndex.invalidateCurrent(path) + live.read { it === env.ktSymbolIndex.getKtFile(path) } + }!! + + assertThat(doorAgrees).isTrue() + } + + @Test + fun `isStale reports a version bump that happened during the pin`() { + val path = openDocument() + + val staleness = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + val before = live.isStale + bumpVersion(path, 2) + before to live.isStale + }!! + + assertThat(staleness).isEqualTo(false to true) + } + + @Test + fun `a nested pin on the same path reuses the outer instance`() { + val path = openDocument() + + val instances = + env.ktSymbolIndex.withLiveKtFile(path) { outer -> + val innerId = + env.ktSymbolIndex.withLiveKtFile(path) { inner -> + inner.read { System.identityHashCode(it) } + }!! + outer.read { System.identityHashCode(it) } to innerId + }!! + + assertThat(instances.second).isEqualTo(instances.first) + } + + @OptIn(ResolutionSideKtFileAccess::class) + @Test + fun `an inner scope release does not unpin the path for the outer scope`() { + val path = openDocument() + + val stillPinned = + env.ktSymbolIndex.withLiveKtFile(path) { outer -> + /* + * Dropping the current-file cache (what CompilationEnvironment does on a close or a move) is + * what makes the registry entry observable from the resolution side at all: while that cache + * still holds the instance, its peek answers with the pinned object whether or not the path + * is pinned. With it gone, an unpinned door loads a separate disk instance instead. + */ + env.ktSymbolIndex.invalidateCurrent(path) + env.ktSymbolIndex.withLiveKtFile(path) { inner -> inner.read { it.name } } + outer.read { it === env.ktSymbolIndex.getKtFile(path) } + }!! + + assertThat(stillPinned).isTrue() + } + + @OptIn(UnpinnedKtFileAccess::class) + @Test + fun `a refresh owed during a pin is applied after release`() { + val path = openDocument() + + val pinnedId = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + bumpVersion(path, 2) + // Answered from the pin, which leaves the refresh for the new version owed. + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } + val id = live.read { System.identityHashCode(it) } + val cached = env.ktSymbolIndex.peekLiveKtFile(path) + assertThat(System.identityHashCode(cached)).isEqualTo(id) + id + }!! + + // Nothing below asks for the current file, so only the release's own deferred refresh can + // replace the cached instance. + assertThat(awaitInstanceChange(path, pinnedId)).isTrue() + } + + @OptIn(UnpinnedKtFileAccess::class) + private fun awaitInstanceChange( + path: Path, + staleId: Int, + ): Boolean { + val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(REFRESH_TIMEOUT_SECONDS) + while (System.nanoTime() < deadline) { + val current = env.ktSymbolIndex.peekLiveKtFile(path) + if (current != null && System.identityHashCode(current) != staleId) return true + Thread.sleep(POLL_INTERVAL_MILLIS) + } + return false + } + + @Test + fun `the pinned file must not escape its scope`() { + val path = openDocument() + + val failure = + runCatching { + env.ktSymbolIndex.withLiveKtFile(path) { live -> live.read { it } } + }.exceptionOrNull() + + assertThat(failure).isInstanceOf(IllegalStateException::class.java) + } + + @Test + fun `a pin on a path with no open document still yields the disk instance`() { + createSourceFile("Closed.kt", content) + val path = env.sourceRoots.first().resolve("Closed.kt") + + val file = env.ktSymbolIndex.withLiveKtFile(path) { live -> live.read { it.name } } + + assertThat(file).isEqualTo("Closed.kt") + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt new file mode 100644 index 0000000000..1aa91a7be1 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt @@ -0,0 +1,112 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.index + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.projects.FileManager +import kotlinx.coroutines.runBlocking +import org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter +import org.junit.After +import org.junit.Test +import java.nio.file.Path + +/** + * A `KtFile` instance for an open path must not be reported as a redeclaration of itself once a + * newer instance for the same path has been registered. + * + * `KtSymbolIndex.currentFiles` mints a fresh instance per observed document version, and + * `DeclarationProvider.ktFilesForPackage` resolves the path to whatever the newest one is. An + * analysis that started against an older instance therefore sees every declaration in the file + * twice - once as its own PSI, once through the provider - and reports the whole file as + * conflicting. That is what reaches the editor as red squiggles over every declaration. + * + * Pinning the path for the duration of the analysis is what closes that: while a scope is open, no + * second instance can be installed, so both doors answer with the same PSI. + */ +internal class StaleKtFileInstanceDiagnosticsTest : KtLspTest() { + override val enableParserEventSystem = true + + private val openedPaths = mutableListOf() + + @After + fun closeDocs() { + openedPaths.forEach { FileManager.onDocumentClose(DocumentCloseEvent(it)) } + openedPaths.clear() + } + + private val content = + """ + package p + + class Widget + + fun render(a: Int, b: Int): Int = extracted(b, a) + a + + private fun extracted(b: Int, a: Int): Int = b * a + """.trimIndent() + + private fun openDocument(): Path { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + FileManager.onDocumentOpen(DocumentOpenEvent(path, content, 1)) + openedPaths.add(path) + return path + } + + /** + * Moves the document to [version] and lets a competing request observe it. + * + * The bump alone only updates [FileManager]: a second `KtFile` for the path is installed by the + * index's own current-file refresh, so without that request there is nothing for the pin to hold + * back and both tests below would pass unpinned. + */ + private fun bumpVersionAndRefresh( + path: Path, + version: Int, + ) { + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, content, content, version, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } + } + + @OptIn(ResolutionSideKtFileAccess::class) + @Test + fun `a version bump inside a pin cannot install a second instance`() { + val path = openDocument() + + val sameInstance = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + // Outside `read`: unpinned, the competing refresh needs project.write, which cannot be + // granted while this thread holds the read lock. + bumpVersionAndRefresh(path, 2) + // getKtFile is the door DeclarationProvider takes; unpinned it would answer with the + // instance the competing refresh installs, which is what makes the file conflict with itself. + live.read { pinned -> env.ktSymbolIndex.getKtFile(path) === pinned } + }!! + + assertThat(sameInstance).isTrue() + } + + @Test + fun `diagnostics stay clean across a version bump during analysis`() { + val path = openDocument() + + val diagnostics = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + bumpVersionAndRefresh(path, 2) + live.analyzing(AnalysisPriority.DIAGNOSTICS, noopCancelChecker()) { ktFile -> + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .map { "${it.factoryName}: ${it.defaultMessage}" } + } + }!! + + assertThat(diagnostics).isEmpty() + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StalePinEditRefusalTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StalePinEditRefusalTest.kt new file mode 100644 index 0000000000..11aa175f4e --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StalePinEditRefusalTest.kt @@ -0,0 +1,239 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.index + +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction +import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction +import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionRefusal +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractionPlan +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.projects.FileManager +import kotlinx.coroutines.runBlocking +import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo +import org.appdevforall.codeonthego.indexing.jvm.JvmSourceLanguage +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolKind +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import java.nio.file.Path + +/** + * A site whose output is an edit must refuse rather than compute against a pin it joined. + * + * While any scope on a path is open, every other request for that path joins it and gets its + * instance, however old. The action layer stamps its version guard from the live buffer, so a joined + * stale pin passes that guard and then applies offsets measured against older text to the newer + * buffer - a silent wrong edit, in the one place a check exists to prevent exactly that. Each site + * here degrades to its own "nothing to offer" answer instead. + * + * Every test first computes the unpinned result and asserts it is non-empty, so a refusal cannot pass + * for an unrelated reason. + */ +internal class StalePinEditRefusalTest : KtLspTest() { + override val enableParserEventSystem = true + + private val openedPaths = mutableListOf() + + @After + fun closeDocs() { + openedPaths.forEach { FileManager.onDocumentClose(DocumentCloseEvent(it)) } + openedPaths.clear() + } + + private fun openDocument( + relativePath: String, + content: String, + ): Path { + createSourceFile(relativePath, content) + val path = env.sourceRoots.first().resolve(relativePath) + FileManager.onDocumentOpen(DocumentOpenEvent(path, content, 1)) + openedPaths.add(path) + return path + } + + /** + * Runs [block] inside an open scope on [path] whose document has since moved to [newContent]. + * + * This is the production shape: some other feature holds the pin, the user types, and [block]'s + * acquisition joins the frozen instance instead of resolving the current one. Passing the file's + * existing text is enough to make the pin version-stale, which is what the guards test; the + * changed-content case is covered separately below. + */ + private fun whileHoldingAStalePin( + path: Path, + newContent: String, + block: () -> R, + ): R = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, newContent, newContent, 2, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + assertTrue("the pin must be stale for this test to mean anything", live.isStale) + block() + }!! + + @Test + fun `extract-method refuses a plan built on a joined stale pin`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + val path = openDocument("Method.kt", content) + val offset = content.indexOf("b * a") + 1 + val plan = { buildExtractMethodPlan(env, path, offset, offset, 2, noopCancelChecker()) } + + assertFalse(plan().candidates.isEmpty()) + val refused = whileHoldingAStalePin(path, content, plan) + + assertEquals(ExtractionRefusal.CouldNotAnalyse, refused.refusal) + assertTrue(refused.candidates.isEmpty()) + } + + @Test + fun `extract-variable returns an empty plan on a joined stale pin`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + val path = openDocument("Variable.kt", content) + val start = content.indexOf("b * a") + val plan = { buildExtractionPlan(env, path, start, start + "b * a".length, 2, noopCancelChecker()) } + + assertFalse(plan().candidates.isEmpty()) + val empty = whileHoldingAStalePin(path, content, plan) + + assertTrue(empty.candidates.isEmpty()) + } + + @Test + fun `organize-imports emits no edit on a joined stale pin`() { + createSourceFile("lib/Lib.kt", "package lib\nclass Used\nclass Unused") + val content = + """ + package p + import lib.Used + import lib.Unused + fun f(x: Used) {} + """.trimIndent() + val path = openDocument("Main.kt", content) + val edits = { OrganizeImportsAction().computeOrganizeEdit(env, path, ICancelChecker.NOOP) } + + assertFalse(edits().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, edits).isEmpty()) + } + + @Test + fun `implement-members emits no edit on a joined stale pin`() { + val content = + """ + package p + interface I { fun foo() } + class C : I + """.trimIndent() + val path = openDocument("Members.kt", content) + val caret = content.indexOf("class C") + 2 + val edits = { ImplementMembersAction().computeImplementMembersEdit(env, path, caret, ICancelChecker.NOOP) } + + assertFalse(edits().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, edits).isEmpty()) + } + + @Test + fun `add-import offers no candidate on a joined stale pin`() { + runBlocking { + env.ktSymbolIndex.sourceIndex.insert( + JvmSymbol( + key = "lib/Foo#CLASS", + sourceId = "test", + name = "lib/Foo", + shortName = "Foo", + packageName = "lib", + kind = JvmSymbolKind.CLASS, + language = JvmSourceLanguage.KOTLIN, + data = JvmClassInfo(), + ), + ) + } + val content = "package p\nfun f(x: Foo) {}" + val path = openDocument("Import.kt", content) + val candidates = { AddImportAction().computeImportCandidates(env, path, "Foo") } + + assertFalse(candidates().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, candidates).isEmpty()) + } + + @Test + fun `null-safety offers no variant on a joined stale pin`() { + val content = + """ + package p + class Box { val prop: Int = 0 } + fun f(b: Box?) { val x = b.prop } + """.trimIndent() + val path = openDocument("NullSafety.kt", content) + val start = content.indexOf("b.prop") + val variants = { NullSafetyAction().computeNullSafetyVariants(env, path, start, start + "b.prop".length) } + + assertFalse(variants().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, variants).isEmpty()) + } + + /** + * The version-stale tests above hold the text constant, which is all [LiveKtFile.isStale] looks at. + * This one moves the text too, and shows what the guard is actually for: the plan the site would + * otherwise have produced carries the *old* file text under the *new* version's stamp, so its spans + * name different source in the buffer the edit would be applied to - and the apply-time guard + * compares only the stamp, so nothing downstream can catch it. + */ + @Test + fun `extract-method refuses rather than planning against text the user has replaced`() { + val original = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + // The user adds an import, shifting every offset below it. + val edited = original.replaceFirst("package p\n", "package p\nimport kotlin.math.max\n") + val path = openDocument("Shifted.kt", original) + val offset = original.indexOf("b * a") + 1 + val plan = { buildExtractMethodPlan(env, path, offset, offset, 2, noopCancelChecker()) } + + val stalePlan = plan() + assertFalse(stalePlan.candidates.isEmpty()) + + val refused = whileHoldingAStalePin(path, edited) { plan() } + + assertEquals(ExtractionRefusal.CouldNotAnalyse, refused.refusal) + assertTrue(refused.candidates.isEmpty()) + + // What the suppressed plan would have replaced: a span that names "b * a" in the pinned text and + // something else entirely at the same offsets in the buffer the edit would land in. + val span = stalePlan.candidates.first { it.label == "b * a" }.span + assertEquals(original, stalePlan.fileText) + assertEquals("b * a", original.substring(span.start, span.end)) + assertNotEquals("b * a", edited.substring(span.start, span.end)) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindDefinitionRequestTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindDefinitionRequestTest.kt index fb77ac74eb..1bce72799d 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindDefinitionRequestTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindDefinitionRequestTest.kt @@ -112,10 +112,10 @@ class FindDefinitionRequestTest : KtLspTest() { @Test fun `a same-file target found through the active document still resolves`() { - // Every other test in this file leaves the file un-opened, so getCurrentKtFile takes the - // disk fallback - a real CoreLocalFileSystem-backed KtFile whose virtualFile has protocol + // Every other test in this file leaves the file un-opened, so acquisition takes the disk + // fallback - a real CoreLocalFileSystem-backed KtFile whose virtualFile has protocol // "file". That's exactly the path the production bug (ADFA-4823 finding 1) does NOT hit: - // opening the file makes getCurrentKtFile refresh a live KtFile instead + // opening the file makes acquisition refresh a live KtFile instead // (KtSymbolIndex.refreshToCurrent), whose virtualFile is a non-physical LightVirtualFile - // locationOfPsi must resolve a path from backingFilePath instead, which is exactly what this // test exercises. diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt index 59d6242568..c19d05f1d9 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt @@ -43,28 +43,19 @@ import java.util.concurrent.ConcurrentHashMap * @author Akash Yadav */ object FileManager { - private val log = LoggerFactory.getLogger(FileManager::class.java) private val _activeDocuments = ConcurrentHashMap() val activeDocuments: Collection get() = _activeDocuments.values.toSet() - fun isActive(uri: URI): Boolean { - return isActive(Paths.get(uri)) - } + fun isActive(uri: URI): Boolean = isActive(Paths.get(uri)) - fun isActive(file: Path): Boolean { - return this._activeDocuments.containsKey(file.normalize()) - } + fun isActive(file: Path): Boolean = this._activeDocuments.containsKey(file.normalize()) - fun getActiveDocument(file: Path): ActiveDocument? { - return this._activeDocuments[file.normalize()] - } + fun getActiveDocument(file: Path): ActiveDocument? = this._activeDocuments[file.normalize()] - fun getActiveDocumentCount(): Int { - return this._activeDocuments.size - } + fun getActiveDocumentCount(): Int = this._activeDocuments.size fun getDocumentContents(file: Path): String { val document = getActiveDocument(file) @@ -115,14 +106,19 @@ object FileManager { _activeDocuments[event.changedFile.normalize()] = createDocument(event) log.warn( "Document change event received before open event for file {}", - event.changedFile + event.changedFile, ) return } - document.version = event.version - document.modified = Instant.now() - document.content = event.newText!! + if (!document.update(event.version, event.newText!!)) { + log.debug( + "Ignoring out-of-order change for {}: event version {} is older than {}", + event.changedFile, + event.version, + document.version, + ) + } event.newText = null } @@ -142,26 +138,24 @@ object FileManager { _activeDocuments.remove(event.file.toPath().normalize()) } - private fun createDocument(event: DocumentOpenEvent): ActiveDocument { - return ActiveDocument( + private fun createDocument(event: DocumentOpenEvent): ActiveDocument = + ActiveDocument( file = event.openedFile, version = event.version, modified = Instant.now(), - content = event.text + content = event.text, ) - } - private fun createDocument(event: DocumentChangeEvent): ActiveDocument { - return ActiveDocument( + private fun createDocument(event: DocumentChangeEvent): ActiveDocument = + ActiveDocument( file = event.changedFile, version = event.version, modified = Instant.now(), - content = event.changedText + content = event.changedText, ) - } - private fun createFileReader(file: Path): BufferedReader { - return try { + private fun createFileReader(file: Path): BufferedReader = + try { Files.newBufferedReader(file) } catch (noFile: java.nio.file.NoSuchFileException) { log.warn("No such file", noFile) @@ -169,10 +163,9 @@ object FileManager { } catch (cancelled: CancellationException) { "".reader().buffered() } - } - private fun createFileInputStream(file: Path): InputStream { - return try { + private fun createFileInputStream(file: Path): InputStream = + try { Files.newInputStream(file) } catch (noFile: java.nio.file.NoSuchFileException) { log.warn("No such file", noFile) @@ -180,14 +173,11 @@ object FileManager { } catch (cancelled: CancellationException) { "".byteInputStream() } - } - private fun getLastModifiedFromDisk(file: Path): Instant { - return Files.getLastModifiedTime(file).toInstant() - } + private fun getLastModifiedFromDisk(file: Path): Instant = Files.getLastModifiedTime(file).toInstant() - private fun getFileContents(file: Path): String { - return try { + private fun getFileContents(file: Path): String = + try { ProgressManager.abortIfCancelled() FileUtils.readFileToString(file.toFile(), Charset.defaultCharset()) } catch (noFile: java.nio.file.NoSuchFileException) { @@ -196,5 +186,4 @@ object FileManager { } catch (cancelled: CancellationException) { "" } - } } diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt index 42b0b7e6cc..09b28ce6f6 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt @@ -29,19 +29,60 @@ import java.time.Instant */ open class ActiveDocument( val file: Path, - var version: Int, - var modified: Instant, - content: String = "" + version: Int, + modified: Instant, + content: String = "", ) { + private data class Snapshot( + val version: Int, + val modified: Instant, + val content: String, + ) - var content: String = content - internal set + /* + * One volatile reference, so a reader can never pair a new version with the old content. The editor + * dispatches change events from a background coroutine per edit, so two edits in one frame do reach + * this concurrently. + */ + @Volatile + private var snapshot = Snapshot(version, modified, content) - fun inputStream(): BufferedInputStream { - return content.byteInputStream().buffered() - } + /** The version last published via [update]. Always consistent with [content] and [modified]. */ + val version: Int + get() = snapshot.version + + /** The timestamp of the last [update]. Always consistent with [version] and [content]. */ + val modified: Instant + get() = snapshot.modified + + /** The content last published via [update]. Always consistent with [version] and [modified]. */ + val content: String + get() = snapshot.content - fun reader(): BufferedReader { - return content.reader().buffered() + /** + * Publishes [content] at [version], or returns false if [version] is older than what is already + * published. + * + * A version that moves backwards makes the Kotlin index mint a second `KtFile` for text that never + * changed, which is what surfaced as redeclaration errors across a whole file (ADFA-5231). + * + * An equal version is accepted and overwrites, rather than being rejected like an older one. The + * only writer, `IDEEditor`, stamps versions from a single serialised `AtomicInteger.incrementAndGet()` + * per document, so distinct edits never share a version - an equal version is a re-delivery of the + * same edit, and taking its (identical) content is harmless. + */ + internal fun update( + version: Int, + content: String, + ): Boolean { + synchronized(this) { + if (version < snapshot.version) return false + snapshot = Snapshot(version, Instant.now(), content) + return true + } } + + fun inputStream(): BufferedInputStream = content.byteInputStream().buffered() + + fun reader(): BufferedReader = content.reader().buffered() } diff --git a/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt new file mode 100644 index 0000000000..b6efed4f33 --- /dev/null +++ b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt @@ -0,0 +1,47 @@ +package com.itsaky.androidide.projects + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.models.Range +import org.junit.After +import org.junit.Test +import java.nio.file.Paths + +/** A document's version and content always move forward together. */ +class ActiveDocumentVersionTest { + private val path = Paths.get("/tmp/adfa5231/Main.kt") + + @After + fun close() { + FileManager.onDocumentClose(DocumentCloseEvent(path)) + } + + private fun change( + text: String, + version: Int, + ) = DocumentChangeEvent(path, text, text, version, ChangeType.NEW_TEXT, 0, Range.NONE) + + @Test + fun `a backwards version is rejected and leaves the newer content in place`() { + FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1)) + FileManager.onDocumentContentChange(change("v3", 3)) + + FileManager.onDocumentContentChange(change("v2", 2)) + + val document = FileManager.getActiveDocument(path)!! + assertThat(document.version).isEqualTo(3) + assertThat(document.content).isEqualTo("v3") + } + + @Test + fun `a version and its content are never observed apart`() { + FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1)) + FileManager.onDocumentContentChange(change("v2", 2)) + + val document = FileManager.getActiveDocument(path)!! + assertThat(document.version to document.content).isEqualTo(2 to "v2") + } +}