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..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 @@ -101,9 +101,26 @@ class AddImportAction : BaseKotlinCodeAction() { } return env.ktSymbolIndex.withLiveKtFile(nioPath) { live -> - live.read { ktFile -> - classifiers.associate { it.fqName to insertImport(ktFile, it.fqName) } + 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() } + + 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 071ae63041..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 @@ -72,27 +72,46 @@ 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 -> - 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) { + // 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() + } + + 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 5333ba8aaa..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 @@ -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,43 @@ 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() + } + + 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( data: ActionData, result: Any, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt index 2ae61fbd44..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 @@ -59,20 +59,39 @@ 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 -> - 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) { + // 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() } + + 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/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..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,11 +174,10 @@ context(env: CompilationEnvironment) internal fun doComplete(params: CompletionParams): CompletionResult { 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() + // 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) @@ -162,9 +193,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/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()) + } +} 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..11aa175f4e --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StalePinEditRefusalTest.kt @@ -0,0 +1,239 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.index + +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction +import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction +import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionRefusal +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractionPlan +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.projects.FileManager +import kotlinx.coroutines.runBlocking +import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo +import org.appdevforall.codeonthego.indexing.jvm.JvmSourceLanguage +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolKind +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import java.nio.file.Path + +/** + * A site whose output is an edit must refuse rather than compute against a pin it joined. + * + * While any scope on a path is open, every other request for that path joins it and gets its + * instance, however old. The action layer stamps its version guard from the live buffer, so a joined + * stale pin passes that guard and then applies offsets measured against older text to the newer + * buffer - a silent wrong edit, in the one place a check exists to prevent exactly that. Each site + * here degrades to its own "nothing to offer" answer instead. + * + * Every test first computes the unpinned result and asserts it is non-empty, so a refusal cannot pass + * for an unrelated reason. + */ +internal class StalePinEditRefusalTest : KtLspTest() { + override val enableParserEventSystem = true + + private val openedPaths = mutableListOf() + + @After + fun closeDocs() { + openedPaths.forEach { FileManager.onDocumentClose(DocumentCloseEvent(it)) } + openedPaths.clear() + } + + private fun openDocument( + relativePath: String, + content: String, + ): Path { + createSourceFile(relativePath, content) + val path = env.sourceRoots.first().resolve(relativePath) + FileManager.onDocumentOpen(DocumentOpenEvent(path, content, 1)) + openedPaths.add(path) + return path + } + + /** + * Runs [block] inside an open scope on [path] whose document has since moved to [newContent]. + * + * This is the production shape: some other feature holds the pin, the user types, and [block]'s + * acquisition joins the frozen instance instead of resolving the current one. Passing the file's + * existing text is enough to make the pin version-stale, which is what the guards test; the + * changed-content case is covered separately below. + */ + private fun whileHoldingAStalePin( + path: Path, + newContent: String, + block: () -> R, + ): R = + env.ktSymbolIndex.withLiveKtFile(path) { live -> + FileManager.onDocumentContentChange( + DocumentChangeEvent(path, newContent, newContent, 2, ChangeType.NEW_TEXT, 0, Range.NONE), + ) + assertTrue("the pin must be stale for this test to mean anything", live.isStale) + block() + }!! + + @Test + fun `extract-method refuses a plan built on a joined stale pin`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + val path = openDocument("Method.kt", content) + val offset = content.indexOf("b * a") + 1 + val plan = { buildExtractMethodPlan(env, path, offset, offset, 2, noopCancelChecker()) } + + assertFalse(plan().candidates.isEmpty()) + val refused = whileHoldingAStalePin(path, content, plan) + + assertEquals(ExtractionRefusal.CouldNotAnalyse, refused.refusal) + assertTrue(refused.candidates.isEmpty()) + } + + @Test + fun `extract-variable returns an empty plan on a joined stale pin`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + val path = openDocument("Variable.kt", content) + val start = content.indexOf("b * a") + val plan = { buildExtractionPlan(env, path, start, start + "b * a".length, 2, noopCancelChecker()) } + + assertFalse(plan().candidates.isEmpty()) + val empty = whileHoldingAStalePin(path, content, plan) + + assertTrue(empty.candidates.isEmpty()) + } + + @Test + fun `organize-imports emits no edit on a joined stale pin`() { + createSourceFile("lib/Lib.kt", "package lib\nclass Used\nclass Unused") + val content = + """ + package p + import lib.Used + import lib.Unused + fun f(x: Used) {} + """.trimIndent() + val path = openDocument("Main.kt", content) + val edits = { OrganizeImportsAction().computeOrganizeEdit(env, path, ICancelChecker.NOOP) } + + assertFalse(edits().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, edits).isEmpty()) + } + + @Test + fun `implement-members emits no edit on a joined stale pin`() { + val content = + """ + package p + interface I { fun foo() } + class C : I + """.trimIndent() + val path = openDocument("Members.kt", content) + val caret = content.indexOf("class C") + 2 + val edits = { ImplementMembersAction().computeImplementMembersEdit(env, path, caret, ICancelChecker.NOOP) } + + assertFalse(edits().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, edits).isEmpty()) + } + + @Test + fun `add-import offers no candidate on a joined stale pin`() { + runBlocking { + env.ktSymbolIndex.sourceIndex.insert( + JvmSymbol( + key = "lib/Foo#CLASS", + sourceId = "test", + name = "lib/Foo", + shortName = "Foo", + packageName = "lib", + kind = JvmSymbolKind.CLASS, + language = JvmSourceLanguage.KOTLIN, + data = JvmClassInfo(), + ), + ) + } + val content = "package p\nfun f(x: Foo) {}" + val path = openDocument("Import.kt", content) + val candidates = { AddImportAction().computeImportCandidates(env, path, "Foo") } + + assertFalse(candidates().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, candidates).isEmpty()) + } + + @Test + fun `null-safety offers no variant on a joined stale pin`() { + val content = + """ + package p + class Box { val prop: Int = 0 } + fun f(b: Box?) { val x = b.prop } + """.trimIndent() + val path = openDocument("NullSafety.kt", content) + val start = content.indexOf("b.prop") + val variants = { NullSafetyAction().computeNullSafetyVariants(env, path, start, start + "b.prop".length) } + + assertFalse(variants().isEmpty()) + + assertTrue(whileHoldingAStalePin(path, content, variants).isEmpty()) + } + + /** + * The version-stale tests above hold the text constant, which is all [LiveKtFile.isStale] looks at. + * This one moves the text too, and shows what the guard is actually for: the plan the site would + * otherwise have produced carries the *old* file text under the *new* version's stamp, so its spans + * name different source in the buffer the edit would be applied to - and the apply-time guard + * compares only the stamp, so nothing downstream can catch it. + */ + @Test + fun `extract-method refuses rather than planning against text the user has replaced`() { + val original = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + // The user adds an import, shifting every offset below it. + val edited = original.replaceFirst("package p\n", "package p\nimport kotlin.math.max\n") + val path = openDocument("Shifted.kt", original) + val offset = original.indexOf("b * a") + 1 + val plan = { buildExtractMethodPlan(env, path, offset, offset, 2, noopCancelChecker()) } + + val stalePlan = plan() + assertFalse(stalePlan.candidates.isEmpty()) + + val refused = whileHoldingAStalePin(path, edited) { plan() } + + assertEquals(ExtractionRefusal.CouldNotAnalyse, refused.refusal) + assertTrue(refused.candidates.isEmpty()) + + // What the suppressed plan would have replaced: a span that names "b * a" in the pinned text and + // something else entirely at the same offsets in the buffer the edit would land in. + val span = stalePlan.candidates.first { it.label == "b * a" }.span + assertEquals(original, stalePlan.fileText) + assertEquals("b * a", original.substring(span.start, span.end)) + assertNotEquals("b * a", edited.substring(span.start, span.end)) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/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) + } +}