From 498856b74dc6465063e73b8309c6a23fc1d0adc9 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 25 Aug 2026 14:51:55 +0000 Subject: [PATCH 1/5] 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 94658ae09f..116afd56b5 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 @@ -101,6 +101,13 @@ class AddImportAction : BaseKotlinCodeAction() { } return 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() + } + live.read { ktFile -> classifiers.associate { it.fqName to insertImport(ktFile, it.fqName) } } 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 cf9f68d7a2..d74cec13f9 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 dd86d251b6b880bacff51b8d7ecc39df80cc5711 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 25 Aug 2026 15:15:44 +0000 Subject: [PATCH 2/5] 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 7f535e533595a21f08a0ae841f5bc9f170f7dd2f Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 26 Aug 2026 19:19:59 +0000 Subject: [PATCH 3/5] ADFA-5231: measure completion against the live buffer, not the pin The stale-pin refusal this replaces returned before analyzingVariant, so an INTERACTIVE request never reached the scheduler and stopped preempting the older completion whose pin it joined - leaving that older one to publish items for a caret the user had already moved past. --- .../kotlin/completion/KotlinCompletions.kt | 58 +++++---- .../completion/CompletionRequestBufferTest.kt | 121 ++++++++++++++++++ 2 files changed, 157 insertions(+), 22 deletions(-) create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/completion/CompletionRequestBufferTest.kt 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 bb18859145..66acf35355 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 @@ -26,6 +26,7 @@ import com.itsaky.androidide.lsp.models.MatchLevel import com.itsaky.androidide.preferences.utils.indentationString import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.progress.ProgressManager +import com.itsaky.androidide.projects.FileManager import io.github.rosemoe.sora.lang.completion.CompletionCancelledException import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo import org.appdevforall.codeonthego.indexing.jvm.JvmFunctionInfo @@ -133,6 +134,37 @@ internal fun codeComplete(params: CompletionParams): CompletionResult { } } +/** The buffer a completion request was measured against, paired with its offset into it. */ +internal data class CompletionRequestBuffer( + val text: String, + val offset: Int, +) + +/** + * The live buffer for [params] and the request's offset into it, or `null` if the offset is past its + * end. + * + * Deliberately the document rather than the pinned [LiveKtFile]: the pin is process-wide, so a joined + * scope hands over another feature's frozen text while [CompletionParams.position] was measured + * against the buffer. Taking both from the buffer keeps them on one version. Refusing on a stale pin + * instead would be worse than useless - the refusal returns before `analyzingVariant`, so an + * INTERACTIVE request never reaches [AnalysisScheduler] and stops preempting the older completion + * whose pin it joined, leaving that older one to publish items for a caret the user has moved past. + * + * A `null` means the buffer moved between the editor measuring the offset and this read, so the + * request describes text that no longer exists. Clamping the offset into range instead would compute + * items for an unrelated context and insert them at the user's real caret. + */ +internal fun completionRequestBuffer(params: CompletionParams): CompletionRequestBuffer? { + val text = FileManager.getDocumentContents(params.file) + val offset = params.position.requireIndex() + if (offset > text.length) { + logger.debug("skipping completion for {}: request offset is past the live buffer", params.file) + return null + } + return CompletionRequestBuffer(text, offset) +} + /** * Runs at the highest [AnalysisPriority.INTERACTIVE]: preempts in-progress diagnostics/indexing and * is never preempted by lower-priority work, but is superseded (cancelled and discarded) by a newer @@ -142,28 +174,10 @@ 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 } - val requestPosition = params.position - /* - * Clamped because the guard above compares the pin to the current document version, not to the - * version params.position was measured against - CompletionParams carries none. A request - * measured before a deletion, processed against a pin resolved after it, has an offset past the - * end of this text, and the splice below would throw rather than return no items. - */ - val completionOffset = requestPosition.requireIndex().coerceAtMost(originalText.length) + // Completion still parses its own placeholder variant (text differs), anchored by the pin to + // the one instance every door answers with for the path. + val (originalText, completionOffset) = + completionRequestBuffer(params) ?: return@withLiveKtFile CompletionResult.EMPTY val prefix = params.requirePrefix() val partial = partialIdentifier(prefix) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/completion/CompletionRequestBufferTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/completion/CompletionRequestBufferTest.kt new file mode 100644 index 0000000000..6d6cdc3242 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/completion/CompletionRequestBufferTest.kt @@ -0,0 +1,121 @@ +package com.itsaky.androidide.lsp.kotlin.completion + +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.lsp.models.CompletionParams +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.projects.FileManager +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import java.nio.file.Path + +/** + * The editor measures a completion's offset against its own buffer, so completion must read its text + * from that same buffer. + * + * The pin it holds while computing cannot be the source: the pin is process-wide, so a request that + * joins another feature's open scope gets that feature's frozen text, which the offset does not + * describe. Refusing on a stale pin is not the answer either - the refusal returns before the + * analysis, so the request stops preempting the older completion whose pin it joined and that older + * one publishes items for a caret the user has already moved past. + */ +internal class CompletionRequestBufferTest : 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 + } + + private fun changeDocument( + path: Path, + newContent: String, + ) = FileManager.onDocumentContentChange( + DocumentChangeEvent(path, newContent, newContent, 2, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + + private fun paramsAt( + path: Path, + offset: Int, + ) = CompletionParams(Position(0, 0, offset), path, ICancelChecker.NOOP) + + @Test + fun `resolves the buffer, not the text a joined pin froze`() { + val original = + """ + package p + fun f() {} + """.trimIndent() + // The user types a second declaration, which exists only in the buffer. + val edited = "$original\nfun g() {}" + val path = openDocument("Buffer.kt", original) + val offset = edited.indexOf("fun g") + val params = paramsAt(path, offset) + + var buffer: CompletionRequestBuffer? = null + env.ktSymbolIndex.withLiveKtFile(path) { live -> + changeDocument(path, edited) + assertTrue("the pin must be stale for this test to mean anything", live.isStale) + buffer = completionRequestBuffer(params) + } + + assertNotNull("a stale pin must not make completion refuse", buffer) + assertEquals(edited, buffer?.text) + assertEquals(offset, buffer?.offset) + } + + @Test + fun `refuses an offset the buffer no longer has`() { + val original = + """ + package p + fun f() { val someLongName = 0 } + """.trimIndent() + val path = openDocument("Shrunk.kt", original) + val params = paramsAt(path, original.length - 2) + + assertNotNull(completionRequestBuffer(params)) + + // The user deletes most of the file, so nothing is at the requested offset any more. Clamping it + // into range would compute items for an unrelated context and insert them at the real caret. + changeDocument(path, "package p") + + assertNull(completionRequestBuffer(params)) + } + + @Test + fun `accepts an offset at the very end of the buffer`() { + val content = + """ + package p + fun f() { } + """.trimIndent() + val path = openDocument("End.kt", content) + + // Completion at end-of-file is ordinary: the placeholder appends there. + assertEquals(content.length, completionRequestBuffer(paramsAt(path, content.length))?.offset) + } +} From f9c0b1f9aa727add6abbbbf811c1a3d4c93036e3 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 26 Aug 2026 19:20:10 +0000 Subject: [PATCH 4/5] ADFA-5231: re-check staleness after computing an edit The pre-acquisition check only covers a pin that was already stale on acquisition. The wider window is the computation itself: nothing between these sites and performCodeAction re-checks the offsets the edits were measured against. --- .../lsp/kotlin/actions/AddImportAction.kt | 14 ++- .../kotlin/actions/ImplementMembersAction.kt | 36 ++++-- .../lsp/kotlin/actions/NullSafetyAction.kt | 16 ++- .../kotlin/actions/OrganizeImportsAction.kt | 24 ++-- .../index/BufferMovedWhileComputingTest.kt | 116 ++++++++++++++++++ 5 files changed, 181 insertions(+), 25 deletions(-) create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/BufferMovedWhileComputingTest.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 116afd56b5..3d5e845fc1 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 @@ -108,9 +108,19 @@ class AddImportAction : BaseKotlinCodeAction() { return@withLiveKtFile emptyMap() } - live.read { ktFile -> - classifiers.associate { it.fqName to insertImport(ktFile, it.fqName) } + val candidates = + live.read { ktFile -> + classifiers.associate { it.fqName to insertImport(ktFile, it.fqName) } + } + + if (live.isStale) { + // Resolving the file and taking the read lock can both block long enough for the user to + // type, and nothing between here and performCodeAction re-checks the insertion point. + logger.debug("dropping import candidates for {}: buffer moved while computing", nioPath) + return@withLiveKtFile emptyMap() } + + candidates } ?: emptyMap() } 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 5f63d378e0..4e59974342 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 @@ -87,21 +87,31 @@ class ImplementMembersAction : BaseKotlinCodeAction() { return@withLiveKtFile emptyList() } - live.read { ktFile -> - val classOrObject = findEnclosingClassOrObject(ktFile, offset) ?: return@read emptyList() - live.analyzing(AnalysisPriority.COMMAND, checker) { - val classSymbol = classOrObject.symbol as? KaClassSymbol ?: return@analyzing emptyList() - if (!isImplementable(classSymbol)) return@analyzing emptyList() - - val classIndent = classIndentOf(ktFile, classOrObject) - val unit = detectIndentUnit(ktFile.text) - val memberIndent = memberIndentOf(ktFile, classOrObject, classIndent, unit) - val stubs = membersToImplement(classSymbol).mapNotNull { renderOverrideStub(it, memberIndent, unit) } - if (stubs.isEmpty()) return@analyzing emptyList() - - buildInsertionEdit(ktFile, classOrObject, stubs, classIndent) + val edits = + 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) + } } + + if (live.isStale) { + // The analysis above is slow enough for the user to type through, and nothing between + // here and performCodeAction re-checks the offsets these edits were measured against. + logger.debug("dropping implement-members edits for {}: buffer moved while computing", nioPath) + return@withLiveKtFile emptyList() } + + edits } ?: emptyList() } }.getOrElse { e -> 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 7b85356f96..e0812716f2 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 @@ -104,10 +104,20 @@ class NullSafetyAction : BaseKotlinCodeAction() { return@withLiveKtFile emptyList() } - live.read { ktFile -> - val qe = findNullableMemberAccess(ktFile, startOffset, endOffset) ?: return@read emptyList() - nullSafetyVariants(qe) + val variants = + live.read { ktFile -> + val qe = findNullableMemberAccess(ktFile, startOffset, endOffset) ?: return@read emptyList() + nullSafetyVariants(qe) + } + + if (live.isStale) { + // Resolving the file and taking the read lock can both block long enough for the user to + // type, and these variants carry raw PSI offsets that nothing downstream re-checks. + logger.debug("dropping null-safety fixes for {}: buffer moved while computing", nioPath) + return@withLiveKtFile emptyList() } + + variants } ?: emptyList() override fun postExec( 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 d87b45ba7c..7868543116 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 @@ -74,14 +74,24 @@ class OrganizeImportsAction : BaseKotlinCodeAction() { return@withLiveKtFile emptyList() } - live.read { ktFile -> - if (ktFile.importDirectives.isEmpty()) return@read emptyList() - val usage = live.analyzing(AnalysisPriority.COMMAND, checker) { collectImportUsage(it) } - val newText = organizedImportBlock(ktFile, usage) ?: return@read emptyList() - val range = ktFile.importList?.textRange?.toRange(ktFile) ?: return@read emptyList() - if (range == Range.NONE) return@read emptyList() - listOf(TextEdit(range, newText)) + val edits = + 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)) + } + + if (live.isStale) { + // The analysis above is slow enough for the user to type through, and nothing between + // here and performCodeAction re-checks the range these edits were measured against. + logger.debug("dropping organize-imports edits for {}: buffer moved while computing", nioPath) + return@withLiveKtFile emptyList() } + + edits } ?: emptyList() } }.getOrElse { e -> diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/BufferMovedWhileComputingTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/BufferMovedWhileComputingTest.kt new file mode 100644 index 0000000000..c221b2fd09 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/BufferMovedWhileComputingTest.kt @@ -0,0 +1,116 @@ +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.ImplementMembersAction +import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.projects.FileManager +import org.junit.After +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 also refuse when the buffer moves *while* it computes. + * + * [StalePinEditRefusalTest] covers the pin that was already stale when the site joined it. The wider + * window is the computation itself: the pin is fresh when the site checks it, the analysis is slow + * enough for the user to type through, and nothing between the site's return and `performCodeAction` + * re-checks the offsets the edits were measured against. + */ +internal class BufferMovedWhileComputingTest : 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 + } + + /** + * An [ICancelChecker] that moves [path]'s document to [newContent] the first time the analysis + * probes it. + * + * That first probe is the one `AnalysisScheduler.acquire` makes, which lands after the site's + * pre-acquisition staleness check and before it has produced any edit - the window under test. The + * checker reports "not cancelled", so the computation itself runs through untouched, and the pin + * keeps the version bump from refreshing the instance under it. + */ + private fun editingOnFirstProbe( + path: Path, + newContent: String, + ): ICancelChecker = + object : ICancelChecker.Default() { + private var probed = false + + override fun isCancelled(): Boolean { + if (!probed) { + probed = true + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, newContent, newContent, 2, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + } + return super.isCancelled() + } + } + + @Test + fun `organize-imports drops edits when the buffer moves while it computes`() { + 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 action = OrganizeImportsAction() + + assertFalse(action.computeOrganizeEdit(env, path, ICancelChecker.NOOP).isEmpty()) + + // The user adds an import, shifting the import list this edit's range was measured against. + val edited = content.replaceFirst("import lib.Used\n", "import lib.Used\nimport lib.Other\n") + + assertTrue(action.computeOrganizeEdit(env, path, editingOnFirstProbe(path, edited)).isEmpty()) + } + + @Test + fun `implement-members drops edits when the buffer moves while it computes`() { + 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 action = ImplementMembersAction() + + assertFalse(action.computeImplementMembersEdit(env, path, caret, ICancelChecker.NOOP).isEmpty()) + + // The user adds an import, shifting the insertion offset this edit was measured against. + val edited = content.replaceFirst("package p\n", "package p\nimport kotlin.math.max\n") + + assertTrue(action.computeImplementMembersEdit(env, path, caret, editingOnFirstProbe(path, edited)).isEmpty()) + } +} From 031bc8ab7ed1c069adff146d3f565891246194b7 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Thu, 27 Aug 2026 16:12:12 +0000 Subject: [PATCH 5/5] ADFA-5231: tell the user the file changed instead of "no imports found" --- .../lsp/kotlin/actions/AddImportAction.kt | 47 ++++++++++++++----- .../actions/AddImportActionPinScopeTest.kt | 2 +- .../lsp/kotlin/actions/AddImportActionTest.kt | 11 +++-- .../compiler/index/StalePinEditRefusalTest.kt | 5 +- resources/src/main/res/values/strings.xml | 1 + 5 files changed, 46 insertions(+), 20 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 3d5e845fc1..17bdd07f33 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 @@ -24,6 +24,7 @@ import com.itsaky.androidide.lsp.models.TextEdit import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.applyLongPressRecursively import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import java.nio.file.Path @@ -58,10 +59,10 @@ class AddImportAction : BaseKotlinCodeAction() { } } - override suspend fun execAction(data: ActionData): Map> { + override suspend fun execAction(data: ActionData): Any { val (_, extra) = data.findDiagnosticExtra() - ?: return emptyMap() + ?: return ImportCandidates.Found(emptyMap()) val (env, action) = extra val nioPath = data.requireFile().toPath() @@ -83,7 +84,7 @@ class AddImportAction : BaseKotlinCodeAction() { env: AbstractCompilationEnvironment, nioPath: Path, referenceName: String, - ): Map> { + ): ImportCandidates { /* * Resolved before the file is pinned, not inside the pin: this is an unbounded SQLite scan that * never reads the file, and a pin held across it freezes live-PSI refresh for the path - every @@ -97,7 +98,7 @@ class AddImportAction : BaseKotlinCodeAction() { .toList() if (classifiers.isEmpty()) { - return emptyMap() + return ImportCandidates.Found(emptyMap()) } return env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> @@ -105,7 +106,7 @@ class AddImportAction : BaseKotlinCodeAction() { // 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() + return@withLiveKtFile ImportCandidates.FileChanged } val candidates = @@ -117,11 +118,11 @@ class AddImportAction : BaseKotlinCodeAction() { // Resolving the file and taking the read lock can both block long enough for the user to // type, and nothing between here and performCodeAction re-checks the insertion point. logger.debug("dropping import candidates for {}: buffer moved while computing", nioPath) - return@withLiveKtFile emptyMap() + return@withLiveKtFile ImportCandidates.FileChanged } - candidates - } ?: emptyMap() + ImportCandidates.Found(candidates) + } ?: ImportCandidates.Found(emptyMap()) } override fun postExec( @@ -130,14 +131,17 @@ class AddImportAction : BaseKotlinCodeAction() { ) { super.postExec(data, result) - if (result !is Map<*, *>) { + if (result is ImportCandidates.FileChanged) { + flashInfo(R.string.msg_import_file_changed) return } - @Suppress("UNCHECKED_CAST") - result as Map> + if (result !is ImportCandidates.Found) { + return + } - if (result.isEmpty()) { + val candidates = result.edits + if (candidates.isEmpty()) { logger.warn("No classifiers to import.") flashError(R.string.msg_no_imports_found) return @@ -153,7 +157,7 @@ class AddImportAction : BaseKotlinCodeAction() { val file = data.requireFile() val nioPath = file.toPath() val actions = - result + candidates .map { (fqName, edits) -> CodeActionItem( title = fqName, @@ -228,3 +232,20 @@ class AddImportAction : BaseKotlinCodeAction() { ) } } + +/** + * The outcome of resolving import candidates. + * + * The two are distinct at the UI: [Found] with no entries means the reference names nothing + * importable, while [FileChanged] means candidates were found and then discarded because the buffer + * moved out from under the offsets they were measured against. + */ +internal sealed interface ImportCandidates { + /** The import edits for each candidate, keyed by fully-qualified name. */ + data class Found( + val edits: Map>, + ) : ImportCandidates + + /** The buffer moved while the candidates were being computed, so the edits were dropped. */ + data object FileChanged : ImportCandidates +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt index a84745129e..7184286fb8 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionPinScopeTest.kt @@ -83,7 +83,7 @@ internal class AddImportActionPinScopeTest : KtLspTest() { val candidates = AddImportAction().computeImportCandidates(env, path, "Foo") env.onSymbolIndexQuery = null - assertThat(candidates.keys).containsExactly("lib.Foo") + assertThat((candidates as ImportCandidates.Found).edits.keys).containsExactly("lib.Foo") assertThat(pinnedAtQueryTime).isFalse() } } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt index 4edbe11662..bb00c5dbeb 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt @@ -2,6 +2,7 @@ package com.itsaky.androidide.lsp.kotlin.actions import com.itsaky.androidide.lsp.kotlin.compiler.index.findSymbolBySimpleName import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.models.TextEdit import kotlinx.coroutines.runBlocking import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo import org.appdevforall.codeonthego.indexing.jvm.JvmFunctionInfo @@ -51,7 +52,7 @@ class AddImportActionTest : KtLspTest() { index(classSymbol("lib", "Foo")) createSourceFile("Main.kt", "package p\nimport lib.Bar\nfun f(x: Foo) {}") - val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo") + val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo").found() assertEquals(setOf("lib.Foo"), candidates.keys) val edit = candidates.getValue("lib.Foo").single() @@ -63,7 +64,7 @@ class AddImportActionTest : KtLspTest() { index(classSymbol("a", "Foo"), classSymbol("b", "Foo")) createSourceFile("Main.kt", "package p\nfun f(x: Foo) {}") - val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo") + val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo").found() assertEquals(setOf("a.Foo", "b.Foo"), candidates.keys) candidates.values.forEach { assertEquals(1, it.size) } @@ -74,7 +75,7 @@ class AddImportActionTest : KtLspTest() { index(classSymbol("a", "Foo"), funSymbol("b", "Foo")) createSourceFile("Main.kt", "package p\nfun f(x: Foo) {}") - val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo") + val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo").found() assertEquals(setOf("a.Foo"), candidates.keys) } @@ -83,7 +84,7 @@ class AddImportActionTest : KtLspTest() { fun `returns no candidates for an unknown reference`() { createSourceFile("Main.kt", "package p\nfun f() {}") - assertTrue(AddImportAction().computeImportCandidates(env, mainPath, "Nope").isEmpty()) + assertTrue(AddImportAction().computeImportCandidates(env, mainPath, "Nope").found().isEmpty()) } /** @@ -106,3 +107,5 @@ class AddImportActionTest : KtLspTest() { ) } } + +private fun ImportCandidates.found(): Map> = (this as ImportCandidates.Found).edits 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 11aa175f4e..5f95ecbe3c 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.ImportCandidates import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest @@ -177,9 +178,9 @@ internal class StalePinEditRefusalTest : KtLspTest() { val path = openDocument("Import.kt", content) val candidates = { AddImportAction().computeImportCandidates(env, path, "Foo") } - assertFalse(candidates().isEmpty()) + assertFalse((candidates() as ImportCandidates.Found).edits.isEmpty()) - assertTrue(whileHoldingAStalePin(path, content, candidates).isEmpty()) + assertEquals(ImportCandidates.FileChanged, whileHoldingAStalePin(path, content, candidates)) } @Test diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 6bdd55490b..e314225fd8 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -97,6 +97,7 @@ No references found No imports found + The file changed. Try importing again. Diagnostics Installation failed Failed to install assets