From 98ca990c12e01022d0930b37fbe9db21ee1b7698 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 25 Aug 2026 12:44:42 +0000 Subject: [PATCH 1/4] ADFA-5231: add failing test for stale KtFile instance redeclarations Lands red on purpose: it is the ADFA-4165 regression, reduced to the smallest sequence that triggers it (acquire an instance, let a second request install a newer one for identical text, re-analyze the first). --- .../StaleKtFileInstanceDiagnosticsTest.kt | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.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..e2177d182f --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt @@ -0,0 +1,78 @@ +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 org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter +import org.jetbrains.kotlin.psi.KtFile +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. + */ +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 diagnosticsOf(ktFile: KtFile): List = + env.analyze(ktFile) { + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .map { "${it.factoryName}: ${it.defaultMessage}" } + } + + @Test + fun `an analysis holding a superseded instance does not see the file twice`() { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + FileManager.onDocumentOpen(DocumentOpenEvent(path, content, 1)) + openedPaths.add(path) + + val inFlight = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + assertThat(diagnosticsOf(inFlight)).isEmpty() + + // Another request observes a different document version and installs a second instance for the + // same path - identical text, new identity. In production this is any of the twelve + // getCurrentKtFile call sites (the refresh scheduler, completion, a code action) running while + // the diagnostics pass for `inFlight` is still going. + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, content, content, 2, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + val superseding = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + assertThat(superseding).isNotSameInstanceAs(inFlight) + + assertThat(diagnosticsOf(inFlight)).isEmpty() + } +} From 3e05046cf89dd917968737dfbfb55bf3eb241bfa Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 25 Aug 2026 14:14:18 +0000 Subject: [PATCH 2/4] ADFA-5231: pin the live KtFile for the duration of an analysis An analysis that started against an older instance saw every declaration twice, once as its own PSI and once through DeclarationProvider, so FIR reported the file as conflicting with itself. Acquisition now pins the path for the whole scope and the raw accessors are private, so an unpinned analysis no longer compiles. --- .../lsp/kotlin/actions/AddImportAction.kt | 27 +-- .../kotlin/actions/ImplementMembersAction.kt | 38 ++-- .../lsp/kotlin/actions/NullSafetyAction.kt | 30 ++-- .../kotlin/actions/OrganizeImportsAction.kt | 32 ++-- .../kotlin/compiler/CompilationEnvironment.kt | 3 +- .../kotlin/compiler/index/KtSymbolIndex.kt | 16 +- .../completion/AdvancedKotlinEditHandler.kt | 11 +- .../kotlin/completion/KotlinCompletions.kt | 163 +++++++++--------- .../diagnostic/KotlinDiagnosticProvider.kt | 131 +++++++------- .../lsp/kotlin/navigation/FindUsages.kt | 65 +++---- .../lsp/kotlin/navigation/GoToDefinition.kt | 33 ++-- .../signaturehelp/KotlinSignatureHelp.kt | 25 ++- .../utils/refactor/ExtractMethodPlanner.kt | 69 ++++---- .../utils/refactor/ExtractVariablePlanner.kt | 36 ++-- .../compiler/index/CurrentKtFileCacheTest.kt | 96 +++++++---- .../compiler/index/LiveKtFilePinTest.kt | 21 ++- .../StaleKtFileInstanceDiagnosticsTest.kt | 66 ++++--- .../navigation/FindDefinitionRequestTest.kt | 6 +- 18 files changed, 443 insertions(+), 425 deletions(-) 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..7f95a4bb81 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,25 @@ 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 -> + // 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..071ae63041 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 @@ -77,25 +74,26 @@ class ImplementMembersAction : BaseKotlinCodeAction() { 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 + // 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 -> + 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..5333ba8aaa 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,6 @@ 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.diagnostic.DiagnosticAction import com.itsaky.androidide.lsp.kotlin.utils.NullSafetyKind import com.itsaky.androidide.lsp.kotlin.utils.NullSafetyVariant @@ -67,22 +66,19 @@ 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) { + extra.compilationEnv.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + live.read { ktFile -> + val qe = + findNullableMemberAccess( + ktFile, + diagnostic.range.start.requireIndex(), + diagnostic.range.end.requireIndex(), + ) ?: return@read emptyList() + nullSafetyVariants(qe) + } + } ?: emptyList() } }.getOrElse { e -> if (e is CancellationException) throw e 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..2ae61fbd44 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. */ @@ -64,18 +61,19 @@ class OrganizeImportsAction : BaseKotlinCodeAction() { 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 + // 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 -> + 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 ff9cc58963..84f20c520d 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 @@ -227,7 +227,7 @@ 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 = + private fun getCurrentKtFile(path: Path): CompletableFuture = getCurrentVersionedKtFile(path)?.thenApply { it.ktFile } ?: CompletableFuture.completedFuture(null) /** @@ -327,7 +327,7 @@ 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. @@ -502,9 +502,17 @@ internal class KtSymbolIndex( } } - fun getKtFile(vf: VirtualFile): KtFile? = getKtFile(vf.toNioPath(), vf) + /** [getKtFile] for [vf], keyed by the path it maps to. */ + internal fun getKtFile(vf: VirtualFile): KtFile? = getKtFile(vf.toNioPath(), vf) - fun getKtFile( + /** + * 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. + */ + internal fun getKtFile( path: Path, virtualFile: VirtualFile? = null, ): KtFile? { 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..46693a19d6 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,99 @@ 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 -> + // Completion still parses its own placeholder variant (text differs), anchored to the + // current file. + val originalText = live.read { it.text } + val requestPosition = params.position + val completionOffset = requestPosition.requireIndex() + val prefix = params.requirePrefix() + val partial = partialIdentifier(prefix) - abortIfCancelled() - - // 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..0b9caf210e 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,88 @@ 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. 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..0323ac264f 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 @@ -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,12 +460,8 @@ 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 { + 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 @@ -474,11 +470,14 @@ private suspend fun usagesIn( 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) { @@ -495,22 +494,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..8d74d7c844 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` @@ -207,27 +202,23 @@ internal suspend fun findDefinitionAt(params: DefinitionParams): DefinitionResul // (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 + // 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. - // - // 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 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.) + 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..0c56498c1e 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,14 +93,6 @@ 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 @@ -114,12 +103,18 @@ internal suspend fun doSignatureHelp(params: SignatureHelpParams): SignatureHelp 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..2a5cf2f5c6 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,9 +12,6 @@ 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 @@ -34,47 +29,45 @@ internal fun buildExtractMethodPlan( cancelChecker: ScheduledCancelChecker, ): ExtractMethodPlan = runCatching { - val ktFile = - env.ktSymbolIndex.getCurrentKtFile(nioPath).get() - ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> + live.read { ktFile -> + val fileText = ktFile.text + val region = + resolveExtractionRegion(ktFile, selectionStart, selectionEnd) + ?: return@read ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion, fileText, documentVersion) - env.project.read { - val fileText = ktFile.text - val region = - resolveExtractionRegion(ktFile, selectionStart, selectionEnd) - ?: return@read ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion, fileText, documentVersion) + live.analyzing(AnalysisPriority.INTERACTIVE, cancelChecker) { + val results = + when (region) { + is ExtractionRegion.Expressions -> { + region.candidates.map { buildCandidate(listOf(it), isExpression = true, fileText = fileText) } + } - analyzeMaybeDangling(ktFile, 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..4a25d501b4 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,23 @@ 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 -> + 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..e8042332ef 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,17 +4,27 @@ 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.Assert.assertEquals -import org.junit.Assert.assertNotSame +import org.junit.Assert.assertNotEquals +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 +/** + * 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 identity is compared inside the block, or + * through an identity hash captured inside it. + */ internal class CurrentKtFileCacheTest : KtLspTest() { private val openedPaths = mutableListOf() @@ -48,16 +58,23 @@ internal class CurrentKtFileCacheTest : KtLspTest() { ) } + /** The identity of the instance one pin on [path] resolves to, since the instance itself cannot escape. */ + private fun pinnedIdentity(path: Path): Int? = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.read { System.identityHashCode(it) } + } + @Test fun `same version returns same instance`() { createSourceFile("A.kt", "fun a() {}") val path = sourcePath("A.kt") openDocument(path, "fun a() {}") - val first = env.ktSymbolIndex.getCurrentKtFile(path).get() - val second = env.ktSymbolIndex.getCurrentKtFile(path).get() + val first = pinnedIdentity(path) + val second = pinnedIdentity(path) - assertSame(first, second) + assertNotNull(first) + assertEquals(first, second) } @Test @@ -65,25 +82,28 @@ 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 = pinnedIdentity(path) changeDocument(path, "fun b() {}\nfun c() {}", 2) - val v2 = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + val v2 = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.read { System.identityHashCode(it) to it.text } + }!! - assertNotSame(v1, v2) - assertEquals("fun b() {}\nfun c() {}", v2.text) + assertNotEquals(v1, v2.first) + assertEquals("fun b() {}\nfun c() {}", v2.second) } @Test - fun `concurrent requests at same version parse once`() { + fun `repeated requests at the same version reuse one instance`() { 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 identities = (1..16).map { pinnedIdentity(path) } - results.forEach { assertSame(results.first(), it) } + assertNotNull(identities.first()) + identities.forEach { assertEquals(identities.first(), it) } } @Test @@ -91,69 +111,77 @@ 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() + pinnedIdentity(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 = pinnedIdentity(path) env.ktSymbolIndex.invalidateCurrent(path) - val second = env.ktSymbolIndex.getCurrentKtFile(path).get()!! + val second = pinnedIdentity(path) - assertNotSame(first, second) + assertNotNull(first) + assertNotEquals(first, second) } + @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) @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 index 9e88f8467e..bb113c2217 100644 --- 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 @@ -8,6 +8,7 @@ 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 @@ -64,6 +65,7 @@ internal class LiveKtFilePinTest : KtLspTest() { ) } + @OptIn(UnpinnedKtFileAccess::class) @Test fun `a version bump inside a pin does not install a second instance`() { val path = openDocument() @@ -72,12 +74,13 @@ internal class LiveKtFilePinTest : KtLspTest() { env.ktSymbolIndex.withLiveKtFile(path) { live -> bumpVersion(path, 2) /* - * In production this second request is any other getCurrentKtFile caller - 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. + * 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. */ - val superseding = env.ktSymbolIndex.getCurrentKtFile(path).get() + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } + val superseding = env.ktSymbolIndex.peekLiveKtFile(path) live.read { it === superseding && it === env.ktSymbolIndex.getKtFile(path) } }!! @@ -154,6 +157,7 @@ internal class LiveKtFilePinTest : KtLspTest() { assertThat(stillPinned).isTrue() } + @OptIn(UnpinnedKtFileAccess::class) @Test fun `a refresh owed during a pin is applied after release`() { val path = openDocument() @@ -162,9 +166,9 @@ internal class LiveKtFilePinTest : KtLspTest() { env.ktSymbolIndex.withLiveKtFile(path) { live -> bumpVersion(path, 2) // Answered from the pin, which leaves the refresh for the new version owed. - env.ktSymbolIndex.getCurrentKtFile(path).get() + runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } val id = live.read { System.identityHashCode(it) } - val cached = env.ktSymbolIndex.getCurrentKtFileIfPresent(path) + val cached = env.ktSymbolIndex.peekLiveKtFile(path) assertThat(System.identityHashCode(cached)).isEqualTo(id) id }!! @@ -174,13 +178,14 @@ internal class LiveKtFilePinTest : KtLspTest() { 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.getCurrentKtFileIfPresent(path) + val current = env.ktSymbolIndex.peekLiveKtFile(path) if (current != null && System.identityHashCode(current) != staleId) return true Thread.sleep(POLL_INTERVAL_MILLIS) } 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 index e2177d182f..eb61aefa32 100644 --- 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 @@ -5,11 +5,11 @@ 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 org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter -import org.jetbrains.kotlin.psi.KtFile import org.junit.After import org.junit.Test import java.nio.file.Path @@ -23,6 +23,9 @@ import java.nio.file.Path * 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 @@ -46,33 +49,54 @@ internal class StaleKtFileInstanceDiagnosticsTest : KtLspTest() { private fun extracted(b: Int, a: Int): Int = b * a """.trimIndent() - private fun diagnosticsOf(ktFile: KtFile): List = - env.analyze(ktFile) { - ktFile - .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) - .map { "${it.factoryName}: ${it.defaultMessage}" } - } - - @Test - fun `an analysis holding a superseded instance does not see the file twice`() { + 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 + } - val inFlight = env.ktSymbolIndex.getCurrentKtFile(path).get()!! - assertThat(diagnosticsOf(inFlight)).isEmpty() - - // Another request observes a different document version and installs a second instance for the - // same path - identical text, new identity. In production this is any of the twelve - // getCurrentKtFile call sites (the refresh scheduler, completion, a code action) running while - // the diagnostics pass for `inFlight` is still going. + private fun bumpVersion( + path: Path, + version: Int, + ) { FileManager.onDocumentContentChange( - DocumentChangeEvent(path, content, content, 2, ChangeType.NEW_TEXT, 0, Range.NONE), + DocumentChangeEvent(path, content, content, version, ChangeType.NEW_TEXT, 0, Range.NONE), ) - val superseding = env.ktSymbolIndex.getCurrentKtFile(path).get()!! - assertThat(superseding).isNotSameInstanceAs(inFlight) + } + + @Test + fun `a version bump inside a pin cannot install a second instance`() { + val path = openDocument() + + val sameInstance = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + live.read { pinned -> + bumpVersion(path, 2) + // getKtFile is the door DeclarationProvider takes; unpinned it would answer with the + // instance the bump installs, which is what makes the file conflict with itself. + 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 -> + bumpVersion(path, 2) + live.analyzing(AnalysisPriority.DIAGNOSTICS, noopCancelChecker()) { ktFile -> + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .map { "${it.factoryName}: ${it.defaultMessage}" } + } + }!! - assertThat(diagnosticsOf(inFlight)).isEmpty() + assertThat(diagnostics).isEmpty() } } 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. From db71b9e147d89c8d3123275b6c99049e3ab7ef26 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 26 Aug 2026 14:22:00 +0000 Subject: [PATCH 3/4] ADFA-5231: resolve import candidates before pinning the file --- .../lsp/kotlin/actions/AddImportAction.kt | 29 ++++-- .../actions/AddImportActionPinScopeTest.kt | 89 +++++++++++++++++++ .../kotlin/fixtures/KtLspTestEnvironment.kt | 18 ++++ 3 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt 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 7f95a4bb81..94658ae09f 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,25 +76,36 @@ 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: 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]). + * Blocking: the index query is SQLite-backed and pinning the file resolves that file before handing + * it over, so callers must stay off the main thread ([execAction] wraps it in [Dispatchers.IO]). */ internal fun computeImportCandidates( env: AbstractCompilationEnvironment, nioPath: Path, referenceName: String, - ): Map> = - env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> - // 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 } + ): Map> { + /* + * Resolved before the file is pinned, not inside the pin: this is an unbounded SQLite scan that + * never reads the file, and a pin held across it freezes live-PSI refresh for the path - every + * concurrent acquirer joins the frozen instance and the refresh is only owed on release. + * Materialized here too, so the index's lazy source-active filter cannot trail into the scope. + */ + val classifiers = + env.ktSymbolIndex + .findSymbolBySimpleName(referenceName, limit = 0) + .filter { it.kind.isClassifier } + .toList() + + if (classifiers.isEmpty()) { + return emptyMap() + } + return env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> live.read { ktFile -> classifiers.associate { it.fqName to insertImport(ktFile, it.fqName) } } } ?: emptyMap() + } override fun postExec( data: ActionData, diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt new file mode 100644 index 0000000000..a84745129e --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt @@ -0,0 +1,89 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.compiler.index.UnpinnedKtFileAccess +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +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.JvmSymbolDescriptor +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolKind +import org.junit.After +import org.junit.Test +import java.nio.file.Path + +/** + * The importable-classifier query must not run inside the file's pin scope. + * + * Holding the pin across the query freezes live-PSI refresh for that path: concurrent acquirers join + * the frozen instance and the refresh is only owed on release. The query does not read the file, so + * the pin buys nothing over that window. + */ +internal class AddImportActionPinScopeTest : KtLspTest() { + override val enableParserEventSystem = true + + private val content = "package p\n\nfun f(x: Foo) {}\n" + + private val openedPaths = mutableListOf() + + @After + fun closeDocs() { + openedPaths.forEach { FileManager.onDocumentClose(DocumentCloseEvent(it)) } + openedPaths.clear() + } + + private fun classifier( + pkg: String, + shortName: String, + ): JvmSymbol { + val internalName = "${pkg.replace('.', '/')}/$shortName" + return JvmSymbol( + key = "$internalName#${JvmSymbolKind.CLASS.name}", + sourceId = "test", + name = internalName, + shortName = shortName, + packageName = pkg, + kind = JvmSymbolKind.CLASS, + language = JvmSourceLanguage.KOTLIN, + data = JvmClassInfo(), + ) + } + + 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 + } + + @OptIn(UnpinnedKtFileAccess::class) + @Test + fun `the classifier query runs before the file is pinned`() { + runBlocking { env.ktSymbolIndex.sourceIndex.insert(classifier("lib", "Foo")) } + val path = openDocument() + + /* + * Nothing has resolved the live document yet, so the current-file cache is empty. That is what + * makes it a usable probe below: it becomes non-empty only once something pins or refreshes. + */ + assertThat(env.ktSymbolIndex.peekLiveKtFile(path)).isNull() + + var pinnedAtQueryTime: Boolean? = null + env.onSymbolIndexQuery = { query -> + if (query.exactMatch[JvmSymbolDescriptor.KEY_NAME] == "Foo" && pinnedAtQueryTime == null) { + pinnedAtQueryTime = env.ktSymbolIndex.peekLiveKtFile(path) != null + } + } + + val candidates = AddImportAction().computeImportCandidates(env, path, "Foo") + env.onSymbolIndexQuery = null + + assertThat(candidates.keys).containsExactly("lib.Foo") + assertThat(pinnedAtQueryTime).isFalse() + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestEnvironment.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestEnvironment.kt index 06c220199f..f68d0d1d4e 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestEnvironment.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestEnvironment.kt @@ -11,6 +11,8 @@ import com.itsaky.androidide.lsp.kotlin.compiler.registrar.AnalysisApiServicePro import com.itsaky.androidide.lsp.kotlin.compiler.registrar.LspAnalysisApiServiceRegistrar import com.itsaky.androidide.lsp.kotlin.compiler.services.AnalysisPermissionOptions import org.appdevforall.codeonthego.indexing.InMemoryIndex +import org.appdevforall.codeonthego.indexing.api.IndexQuery +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolDescriptor import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolIndex import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataDescriptor @@ -78,6 +80,17 @@ internal class KtLspTestEnvironment( val sourceRoots: List = moduleSpecs.map { spec -> baseDir.resolve(spec.dirName).createDirectories() } + /** + * Invoked with every query the backing symbol index receives, before it runs. + * + * The fixture aliases one index as both `sourceIndex` and `libraryIndex`, so a single + * `findSymbolBySimpleName` call fires this hook twice. The hook exists so a test can observe + * state *at query time* rather than after the fact, which is the only way to assert what a query + * does or does not run inside. + */ + @Volatile + var onSymbolIndexQuery: ((IndexQuery) -> Unit)? = null + private val rootByModule: Map = moduleSpecs.map { it.name }.zip(sourceRoots).toMap() @@ -210,6 +223,11 @@ internal class KtLspTestEnvironment( object : JvmSymbolIndex(inMemoryJvmBackingIndex, BackgroundIndexer(inMemoryJvmBackingIndex)) { // ensure we're not filtering out anything override fun isActive(sourceId: String) = true + + override fun query(query: IndexQuery): Sequence { + onSymbolIndexQuery?.invoke(query) + return super.query(query) + } } val inMemoryFileMetaBackingIndex = InMemoryIndex(KtFileMetadataDescriptor) From e8e34226bc70137849e9c1ff439cdd19a8093173 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 26 Aug 2026 14:29:04 +0000 Subject: [PATCH 4/4] ADFA-5231: name the diagnostics reschedule as a self-cancelling send --- .../KeyedDebouncingActionSelfScheduleTest.kt | 58 +++++++++++++++++++ .../diagnostic/KotlinDiagnosticProvider.kt | 6 ++ 2 files changed, 64 insertions(+) create mode 100644 common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionSelfScheduleTest.kt diff --git a/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionSelfScheduleTest.kt b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionSelfScheduleTest.kt new file mode 100644 index 0000000000..02ee017a98 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionSelfScheduleTest.kt @@ -0,0 +1,58 @@ +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import org.junit.Test +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicInteger +import kotlin.time.Duration.Companion.milliseconds + +/** + * Scheduling the key an action is *currently running for* cancels that run. + * + * The worker races `actionJob.onJoin` against `channel.onReceive`, so a send from inside the action + * is indistinguishable from a newer key arriving: the receive wins, the in-flight job is cancelled, + * and the key is re-sent. `KotlinDiagnosticProvider` relies on both halves of that - it reschedules + * from inside its own action and expects the analysis it just discarded to run again. + */ +class KeyedDebouncingActionSelfScheduleTest { + private companion object { + const val SECOND_RUN_TIMEOUT_SECONDS = 5L + } + + @Test + fun `scheduling from inside the action cancels that run and re-runs it`() = + runBlocking { + val runs = AtomicInteger(0) + val firstRunFinished = AtomicBoolean(false) + val secondRunStarted = CountDownLatch(1) + lateinit var debouncer: KeyedDebouncingAction + + debouncer = + KeyedDebouncingAction( + scope = CoroutineScope(SupervisorJob()), + debounceDuration = 20.milliseconds, + action = { key, _ -> + if (runs.incrementAndGet() == 1) { + debouncer.schedule(key) + // A suspension point is where the cancellation from the self-send takes effect. + delay(200) + firstRunFinished.set(true) + } else { + secondRunStarted.countDown() + } + }, + ) + + debouncer.schedule("k") + + assertThat(secondRunStarted.await(SECOND_RUN_TIMEOUT_SECONDS, TimeUnit.SECONDS)).isTrue() + assertThat(firstRunFinished.get()).isFalse() + debouncer.cancelAll() + } +} 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 0b9caf210e..cf9f68d7a2 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 @@ -143,6 +143,12 @@ private fun doAnalyze( if (superseded) { logger.debug("dropping superseded diagnostics for {}", file) + /* + * On the debounced path this is a self-send: doAnalyze runs as fileAnalyzer's own action, so the + * send reads to the worker as a newer key and cancels the run it came from. Deliberate - the + * reschedule still lands, and the only casualty is the NO_UPDATE publish below, which had nothing + * to say anyway. Reached from KotlinLanguageServer.analyze() instead, it is a plain reschedule. + */ env.fileAnalyzer.schedule(file) } else { logger.warn("File {} is not accessible", file)