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/actions/AddImportAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt index 33ed711df8..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,23 +76,35 @@ 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: 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> { - val ktFile = + /* + * 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 - .getCurrentKtFile(nioPath) - .get() ?: return emptyMap() + .findSymbolBySimpleName(referenceName, limit = 0) + .filter { it.kind.isClassifier } + .toList() - return env.ktSymbolIndex - .findSymbolBySimpleName(referenceName, limit = 0) - .filter { it.kind.isClassifier } - .associate { it.fqName to insertImport(ktFile, it.fqName) } + 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( 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..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 @@ -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,94 @@ 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) + /* + * 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) + } + 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/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/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 new file mode 100644 index 0000000000..eb61aefa32 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt @@ -0,0 +1,102 @@ +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 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 + } + + private fun bumpVersion( + path: Path, + version: Int, + ) { + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, content, content, version, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + } + + @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(diagnostics).isEmpty() + } +} 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) 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.