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