diff --git a/docs/adr/0015-one-pinned-ktfile-per-analysis.md b/docs/adr/0015-one-pinned-ktfile-per-analysis.md new file mode 100644 index 0000000000..ac52d16947 --- /dev/null +++ b/docs/adr/0015-one-pinned-ktfile-per-analysis.md @@ -0,0 +1,177 @@ +# 0015. One pinned live KtFile per analysis, enforced by the type system + +- **Status:** Proposed +- **Date:** 2026-08-25 +- **Deciders:** Code On The Go team + +## Context + +The K2 Kotlin LSP relies on one live `KtFile` instance per open path. `DeclarationProvider.ktFilesForPackage` +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt`) resolves a +path through `KtSymbolIndex.getKtFile` for anything an analysis session needs to see beyond the file it started on. +If that lookup can answer with a *different* instance than the one the analysis is holding, FIR sees every +top-level declaration twice - once as the analysis's own PSI, once through the provider - and reports the file as +conflicting with itself. That is what reaches the editor as "Redeclaration" / "Conflicting overloads" underlines +on every declaration. + +This is not a new failure. ADFA-4165 established the one-instance invariant: `CompilationEnvironment.onFileContentChanged` +captured the `KtFile` being replaced, then atomically invalidated its FIR session and installed the replacement +under `project.write`, and a companion fix to `KeyedDebouncingAction` stopped two refreshes for the same key from +running concurrently and installing out of order (commit `975d23fdfc`). ADFA-3322 (`Signature help for Kotlin LSP`, +PR #1484) replaced that file-handling path with a per-version `currentFiles` cache +(`KtSymbolIndex.getCurrentVersionedKtFile`) that mints a fresh `KtFile` every time the open document's version +changes, and neither the atomic install nor the serialization carried forward. The regression this ADR fixes is +that gap: `getCurrentVersionedKtFile` and `getKtFile` could each answer a lookup for the same path with a different +instance if a refresh landed between them, and an analysis rooted at the older one saw its own declarations doubled +through the provider. `StaleKtFileInstanceDiagnosticsTest` +(`lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt`) +reproduces it directly. + +The history is the argument for the decision below: a runtime mechanism enforced the invariant once, tied to code +that the next refactor replaced wholesale without carrying the discipline forward. A property that has to be +remembered gets lost the next time someone who does not know the history touches the code. The fix has to be +something the next refactor cannot drop without the code failing to compile. + +## Decision + +**A `KtFile` for an open path may only be obtained as a pinned handle, and only one instance is pinned to a path +at a time.** `LiveKtFile` (`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt`) +is an `internal sealed interface` whose only implementation, `KtSymbolIndex.PinnedKtFile`, is `private`. The only +way to obtain one is `KtSymbolIndex.withLiveKtFile` / `withLiveKtFileAsync` +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`), which: + +1. Acquire the path's `Pin` - join one already open (`joinExistingPin`, reference-counted), or resolve the current + instance and install a new one (`acquirePin` / `acquirePinAsync`, `installPin`). +2. While the pin is open, every door resolves to the pinned instance: `getCurrentVersionedKtFile` returns it + without minting a new one even if the document has moved on, and `getKtFile` - the resolution-side door + `DeclarationProvider.ktFilesForPackage` calls - checks `pins[path]` first. The two doors this bug came from can + no longer disagree. +3. A version bump observed while the pin is open is recorded (`Pin.refreshOwed`) rather than acted on, and applied + once the last scope releases (`releasePin`), so the pin defers the refresh instead of losing it. + +`getCurrentKtFile`, `getCurrentVersionedKtFile` and `getCurrentKtFileIfPresent` are `private`; `getKtFile` stays +`internal` but is gated behind its own `@RequiresOptIn(ERROR)` marker, `ResolutionSideKtFileAccess`, because +`internal` alone still let any file in the module - including the test source set and whatever the next refactor +adds - take the live instance and analyse it, which is exactly the shape of the ADFA-3322 regression. Its three +production opt-ins are the Analysis API service providers that only need to name the PSI for a path +(`DeclarationProvider.ktFilesForPackage`, `AnnotationsResolver.allDeclarations`, +`DirectInheritorsProvider.computeIndex`). `LiveKtFile` never exposes the `KtFile` as a value - `read` and +`analyzing` take a lambda instead of returning the file - so a caller cannot hold a reference past the scope that +pinned it. `analyzing` routes through `analyzeMaybeDangling`, which is `withAnalysisLock` under the hood +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt`). + +**Gating the sources is not sufficient, so the sink is gated too.** A third `@RequiresOptIn(ERROR)` marker, +`UnpinnedAnalysis`, sits on `analyzeMaybeDangling` itself. The reason is that not every source of a live `KtFile` +*can* be marked. `DeclarationProvider.ktFilesForPackage` is `protected` and opted in, but the Analysis API +interface it feeds - `KotlinDeclarationProvider`, implemented here by `AbstractDeclarationProvider` - re-exports +those same instances through its own public members (`findFilesForFacade`, `findFilesForFacadeByPackage`, +`findFilesForScript`, `getTopLevelCallableFiles`), and `AnnotationsResolver.declarationsByAnnotation` does the +same one hop out via `KtAnnotated.containingKtFile`. Those members cannot carry the marker: Kotlin rejects an +opt-in marker on an override whose base declaration lacks it (`OPT_IN_MARKER_ON_OVERRIDE`, an error), and the base +declarations belong to the Analysis API. Marking the implementing class instead would not help either, because +`project.createDeclarationProvider(scope, null)` is typed as the platform interface, so no marker on our +classifier is ever consulted. Gating the sink catches every such route at the one point they all arrive at. +`analyzeMaybeDangling` has three production opt-ins: `PinnedKtFile.analyzing` and `analyzingVariant` (the +sanctioned implementation) and `SourceFileIndexer.indexSourceFile` - the known exception, reached when +`refreshToCurrent` hands a freshly minted instance to `queueOnFileChangedAsync`, which carries the raw `KtFile` +through `IndexCommand.IndexModifiedFile` and analyses it with no pin (pre-existing, tracked as a follow-up). + +What stays convention rather than compiler-enforced is the Analysis API's own `analyze` / `analyzeCopy`: both are +public functions of an external module, reachable from anywhere with any `KtFile`, and no marker of ours can +cover them. `withAnalysisLock`'s doc comment asks callers not to take that route; that ask is all there is. + +**One escape hatch:** `KtSymbolIndex.peekLiveKtFile`, gated behind `@RequiresOptIn(ERROR)` `UnpinnedKtFileAccess`. +Its one production caller is `AdvancedKotlinEditHandler` +(`lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/AdvancedKotlinEditHandler.kt`), which runs +on the UI thread after completion has already returned, does PSI-only work, and opens no analysis session. +Pinning there would block the UI thread on a refresh that a background analysis might be holding up. + +That justification covers *analysis* coherence only, and the hatch is not safe in the sense the `isStale` guards +above address. `AdvancedKotlinEditHandler.performEdits` passes the unpinned instance to +`KotlinAutoImportEditHandler`, which computes offset-based `TextEdit`s from its import-directive text ranges +(`utils/EditExts.kt`, `insertImport`) and applies them to the editor buffer through `RewriteHelper.performEdits`. +Nothing compares that instance's text or version against the `Content` being edited, and `peekLiveKtFile` returns +whatever the current-file cache holds, which lags the buffer by however long the refresh takes - so this site does +hand offsets from possibly-stale PSI into an edit. The behaviour is unchanged by this ADR's change and the fix is +tracked separately; widening the hatch to a second caller has to weigh that, not just the analysis argument. + +## Consequences + +**Positive** + +- *Analysing* a `KtFile` obtained outside `withLiveKtFile` / `withLiveKtFileAsync` does not compile without an + explicit `@OptIn` on one of the three markers, which makes every exemption visible in review rather than + reachable by autocomplete. That is the step the bug needs: the ADFA-3322 shape is a superseded instance handed + to `analyze`, and the sink gate rejects it however the instance was obtained. *Obtaining* one is only partly + gated - the declaration-provider and annotations-resolver re-exports above are ungatable - so a refactor can + still get a live instance without a diagnostic; it just cannot analyse it silently. +- The pin makes explicit what was previously only inferred from two call sites happening to agree: an analysis and + the declaration provider see the same PSI for the whole scope, by construction. + +**Negative / costs** + +- **A pin is process-wide, not per-caller.** A second request for a pinned path joins the pin and sees that + scope's text, which can already be older than the buffer. Pin duration is a cross-request staleness window for + everyone, not just the request that opened it. +- Callers that consult `LiveKtFile.isStale` fall into three buckets, not two. Sites whose output is an edit refuse + rather than compute offsets against frozen text: `ExtractVariablePlanner`, `ExtractMethodPlanner`, + `KotlinCompletions`, `OrganizeImportsAction`, `ImplementMembersAction`, `AddImportAction`, `NullSafetyAction`. A + refusal is recoverable; a wrong edit to the user's source is not. `KotlinDiagnosticProvider.doAnalyze` discards + and reschedules instead: it has nothing safe to hand the user in the moment, so it drops the computed diagnostics + and re-queues the file through `env.fileAnalyzer.schedule` rather than paint the editor with squiggles for text + the user has already replaced. When it runs as `fileAnalyzer`'s own action - the debounced path - that reschedule + is a self-send: the send reads to the worker as a newer key and cancels the run it came from. Intended - the key + is still re-sent, and the cancelled tail was only going to publish `NO_UPDATE`. Navigation and info sites - + go-to-definition, find usages, signature help - deliberately tolerate being one edit behind (see the comment at + `GoToDefinition.kt:215`) and do not check `isStale` at all, because their failure mode is a wrong jump, not a + corrupted file or a dropped result. +- **Known parked consequence:** while background diagnostics hold a pin and the user keeps typing, a completion + request joins the stale pin and returns no items until the next keystroke closes it. Fixing this needs + acquisition to be priority-aware - an interactive request preempting a lower-priority holder instead of joining + it - which `Pin` cannot do yet: it has no notion of *which* acquirer holds it, and `AnalysisScheduler`'s + `preempt()` (ADR 0011) latches onto whichever scope is active, so signalling "the holder" from here would fire an + `AnalysisPreemptedException` into a nested outer scope that never asked to be cancelled. `Pin` becoming a + per-holder registry is a prerequisite, not scheduled here. +- The escape guard is partial. `PinnedKtFile.guarded` rejects returning the pinned file *directly* from a `read` / + `analyzing` block, but returning it wrapped - inside a collection, or as one of its child PSI elements - escapes + the check undetected and is equally unsafe. +- A narrow window remains between resolving an instance and installing its pin (documented on `withLiveKtFile`): + a request arriving in that window sees no pin yet and can launch a refresh that completes inside the scope, + firing a FIR modification event under it. Instance identity still holds through every door - the pin is stamped + with the resolved instance's own version, so the bump is not lost, only deferred. Closing the window fully would + mean publishing a pin before its file exists, making joiners wait on an unresolved entry in the one path every + caller depends on; that deadlock risk was judged worse than the window. + +## Alternatives considered + +- **A runtime mechanism that keeps the invariant true without a type gate** - what ADFA-4165 did: atomically + invalidate the superseded FIR session and install the replacement under `project.write`, serialized so two + refreshes for the same key cannot race. It worked, until ADFA-3322 replaced the code path it lived in without + carrying the same discipline forward. That is precisely how this regression happened. +- **One mutable `KtFile` per open path, reparsed in place instead of minting a new instance per version** - + strictly the deeper fix: it removes the multiple-identities problem instead of gating access to it. Not taken. + In-place reparse (`BlockSupport.reparseRange` against a `LightVirtualFile`) is unproven in this standalone/mock + Analysis API environment, which has no real `PsiDocumentManager` behind it - real feasibility risk to carry on a + regression fix. It would also still be a construction property a later refactor could quietly undo, rather than + something the compiler holds; the team chose the type gate instead and did not schedule in-place reparse as a + follow-up. +- **A custom lint/detekt rule banning the raw accessors** - the build has no detekt; Spotless's ktlint integration + only formats, it does not carry custom semantic rules, so there is no rule seat to put this in. + +## Related + +- ADFA-4165 - established the one-live-KtFile-per-path invariant, once enforced by an atomic install-and-invalidate + under `project.write` rather than by the type system. +- ADFA-3322 (PR #1484) - introduced the per-version `currentFiles` cache that reintroduced the bug. +- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) - why navigation resolves through the Analysis API, + the pipeline this pin protects. +- [ADR 0011](0011-command-analysis-priority.md) - `AnalysisScheduler` priorities and `preempt()`, referenced above + as the reason acquisition cannot yet be made priority-aware. +- `lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt` - the pinned handle, + `UnpinnedKtFileAccess` and `ResolutionSideKtFileAccess`. +- `lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt` - `UnpinnedAnalysis` + and the `analyzeMaybeDangling` sink it gates. +- `lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` - `pins`, `Pin`, + `withLiveKtFile`, `withLiveKtFileAsync`, `getKtFile`. +- `lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt` - + reproduces the regression this ADR documents the fix for. diff --git a/docs/adr/README.md b/docs/adr/README.md index 5b7b7fa226..0ba00dda75 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -26,5 +26,6 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0010](0010-navigation-resolves-via-analysis-api.md) | Kotlin navigation resolves via the Analysis API, not the symbol index | Proposed | | [0011](0011-command-analysis-priority.md) | User-invoked commands get their own analysis priority | Proposed | | [0012](0012-volatile-build-metadata-out-of-abis.md) | Keep volatile build metadata out of module ABIs | Proposed | -| [0013](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | -| [0014](0013-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | +| [0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | +| [0014](0014-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | +| [0015](0015-one-pinned-ktfile-per-analysis.md) | One pinned live KtFile per analysis, enforced by the type system | Proposed | diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt index 52f5ad7b7c..4a5ba33456 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 @@ -5,6 +5,7 @@ import com.itsaky.androidide.lsp.kotlin.compiler.CompilationKind import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.UnpinnedAnalysis import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read @@ -239,6 +240,7 @@ internal class KtSymbolIndex( * pin with a version its PSI does not have, which makes [LiveKtFile.isStale] claim a superseded * instance is current. */ + @OptIn(ResolutionSideKtFileAccess::class) private fun getCurrentVersionedKtFile(path: Path): CompletableFuture? { if (!DocumentUtils.isKotlinFile(path)) return null @@ -468,6 +470,7 @@ internal class KtSymbolIndex( override fun read(block: (KtFile) -> R): R = project.read { guarded(block(pin.file)) } + @OptIn(UnpinnedAnalysis::class) override fun analyzing( priority: AnalysisPriority, cancelChecker: ScheduledCancelChecker, @@ -480,6 +483,7 @@ internal class KtSymbolIndex( ) } + @OptIn(UnpinnedAnalysis::class) override fun analyzingVariant( name: String, text: String, @@ -510,6 +514,7 @@ internal class KtSymbolIndex( } /** [getKtFile] for [vf], keyed by the path it maps to. */ + @ResolutionSideKtFileAccess internal fun getKtFile(vf: VirtualFile): KtFile? = getKtFile(vf.toNioPath(), vf) /** @@ -517,8 +522,9 @@ internal class KtSymbolIndex( * * A pinned path resolves to the pinned instance, so an open analysis and the declaration provider * cannot disagree about which instance is the file. Otherwise the live cache is peeked, then the - * on-disk instance is loaded. + * on-disk instance is loaded. See [ResolutionSideKtFileAccess] for why this is opt-in. */ + @ResolutionSideKtFileAccess internal fun getKtFile( path: Path, virtualFile: VirtualFile? = null, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt index 1154f7d49b..5d468f9f72 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFile.kt @@ -21,6 +21,28 @@ import java.nio.file.Path @Retention(AnnotationRetention.BINARY) internal annotation class UnpinnedKtFileAccess +/** + * Marks the resolution-side door: what the Analysis API service providers answer "what PSI is at this + * path" with. + * + * For an open path it hands back the live instance (the pinned one while a pin is held, otherwise + * whatever the current-file cache holds), so it is a reference that can be superseded. Analysing what + * it returns without a pin is exactly what ADFA-3322 did, and it makes FIR see every top-level + * declaration twice. Opting in is for service providers that only need to name the PSI for a path; + * anything that analyses must use [KtSymbolIndex.withLiveKtFile]. + * + * This marker cannot cover the whole door: the Analysis API interfaces the service providers implement + * re-export the instances through their own public members, which may not carry an opt-in marker + * (`OPT_IN_MARKER_ON_OVERRIDE`). Those routes are caught at the sink instead, by + * [com.itsaky.androidide.lsp.kotlin.compiler.modules.UnpinnedAnalysis]. + */ +@RequiresOptIn( + level = RequiresOptIn.Level.ERROR, + message = "Resolution-side KtFile access. Use KtSymbolIndex.withLiveKtFile for anything that analyses.", +) +@Retention(AnnotationRetention.BINARY) +internal annotation class ResolutionSideKtFileAccess + /** * A [KtFile] pinned to its path for the lifetime of the scope that produced it. * diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt index d4d0a1df10..313b9acc49 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt @@ -2,6 +2,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.index import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.UnpinnedAnalysis import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read @@ -96,6 +97,7 @@ internal fun KtFile.toMetadata( ) } +@OptIn(UnpinnedAnalysis::class) internal suspend fun indexSourceFile( project: Project, ktFile: KtFile, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index 7bf3f7f473..afaf57b50e 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -31,7 +31,10 @@ private val logger = LoggerFactory.getLogger("KtFileExts") * (`KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.`). * [AnalysisScheduler] serializes access; it is priority-aware, preemptive (via [cancelChecker]) and * reentrant. **All** Analysis API access must go through this helper (or [analyzeMaybeDangling]); never - * call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. + * call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. That much is still + * convention - both are public Analysis API functions this module cannot gate. What *is* enforced is the + * step after: [analyzeMaybeDangling] requires opting in to [UnpinnedAnalysis], so analysing PSI that did + * not come from a pin is a compile error until someone says so explicitly. * * **Cancellation.** [action] runs with a [kotlinx.coroutines.Job] installed in the thread's IntelliJ * context; the compiler's dense `checkCanceled()` calls throw once that Job is cancelled, aborting @@ -94,6 +97,28 @@ internal inline fun withAnalysisLock( } } +/** + * Marks direct analysis of PSI that carries no pin. + * + * For an open path the pinned instance is the only one an analysis may see - a second, superseded + * instance of the same file makes FIR report every top-level declaration twice (ADFA-3322, ADFA-4165, + * ADFA-5231). `LiveKtFile.analyzing` guarantees that; analysing a `KtFile` obtained any other way does + * not, and the sources it can be obtained from are not all gateable (the Analysis API service provider + * interfaces this module implements re-export live instances through their own public members). So the + * requirement sits here, on the sink, where every route arrives. + */ +@RequiresOptIn( + level = RequiresOptIn.Level.ERROR, + message = "Analysing unpinned PSI. Use KtSymbolIndex.withLiveKtFile for an open path.", +) +@Retention(AnnotationRetention.BINARY) +internal annotation class UnpinnedAnalysis + +/** + * Analyses [useSiteElement] under the shared analysis lock, routing dangling copies through + * `analyzeCopy` so a completion variant resolves against itself rather than its origin. + */ +@UnpinnedAnalysis internal inline fun analyzeMaybeDangling( useSiteElement: KtElement, priority: AnalysisPriority, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt index 0d04ab5888..f0637296f6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/AnnotationsResolver.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.services import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex +import com.itsaky.androidide.lsp.kotlin.compiler.index.ResolutionSideKtFileAccess import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import org.jetbrains.kotlin.analysis.api.platform.declarations.KotlinAnnotationsResolver import org.jetbrains.kotlin.analysis.api.platform.declarations.KotlinAnnotationsResolverFactory @@ -25,8 +26,9 @@ import org.jetbrains.kotlin.psi.KtUserType import org.jetbrains.kotlin.psi.declarationRecursiveVisitor import org.jetbrains.kotlin.util.collectionUtils.filterIsInstanceAnd -internal class AnnotationsResolverFactory : KtLspService, KotlinAnnotationsResolverFactory { - +internal class AnnotationsResolverFactory : + KtLspService, + KotlinAnnotationsResolverFactory { private lateinit var project: Project private lateinit var index: KtSymbolIndex @@ -34,15 +36,14 @@ internal class AnnotationsResolverFactory : KtLspService, KotlinAnnotationsResol project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.project = project this.index = index } - override fun createAnnotationResolver(searchScope: GlobalSearchScope): KotlinAnnotationsResolver { - return AnnotationsResolver(project, searchScope, index) - } + override fun createAnnotationResolver(searchScope: GlobalSearchScope): KotlinAnnotationsResolver = + AnnotationsResolver(project, searchScope, index) } @Suppress("UnstableApiUsage") @@ -51,58 +52,59 @@ internal class AnnotationsResolver( private val scope: GlobalSearchScope, private val index: KtSymbolIndex, ) : KotlinAnnotationsResolver { - private val declarationProvider by lazy { project.createDeclarationProvider(scope, contextualModule = null) } + @OptIn(ResolutionSideKtFileAccess::class) private fun allDeclarations(): List { val virtualFiles = VirtualFileEnumeration.extract(scope) ?: return emptyList() - val filesInScope = virtualFiles - .filesIfCollection - .orEmpty() - .asSequence() - .filter { it in scope } - .mapNotNull { index.getKtFile(it) } + val filesInScope = + virtualFiles + .filesIfCollection + .orEmpty() + .asSequence() + .filter { it in scope } + .mapNotNull { index.getKtFile(it) } return buildList { - val visitor = declarationRecursiveVisitor visit@{ - val isLocal = when (it) { - is KtClassOrObject -> it.isLocal - is KtFunction -> it.isLocal - is KtProperty -> it.isLocal - else -> return@visit - } - - if (!isLocal) { - add(it) + val visitor = + declarationRecursiveVisitor visit@{ + val isLocal = + when (it) { + is KtClassOrObject -> it.isLocal + is KtFunction -> it.isLocal + is KtProperty -> it.isLocal + else -> return@visit + } + + if (!isLocal) { + add(it) + } } - } filesInScope.forEach { it.accept(visitor) } } } - override fun declarationsByAnnotation(annotationClassId: ClassId): Set { - return allDeclarations() + override fun declarationsByAnnotation(annotationClassId: ClassId): Set = + allDeclarations() .asSequence() .filter { annotationClassId in annotationsOnDeclaration(it) } .toSet() - } - override fun annotationsOnDeclaration(declaration: KtAnnotated): Set { - return declaration + override fun annotationsOnDeclaration(declaration: KtAnnotated): Set = + declaration .annotationEntries .asSequence() .flatMap { it.typeReference?.resolveAnnotationClassIds(declarationProvider).orEmpty() } .toSet() - } } private fun KtTypeReference.resolveAnnotationClassIds( declarationProvider: KotlinDeclarationProvider, - candidates: MutableSet = mutableSetOf() + candidates: MutableSet = mutableSetOf(), ): Set { val annotationTypeElement = typeElement as? KtUserType val referencedName = annotationTypeElement?.referencedFqName ?: return emptySet() @@ -132,8 +134,10 @@ private val KtUserType.referencedFqName: FqName? return FqName.fromSegments(allQualifiers) } - -private fun FqName.resolveToClassIds(to: MutableSet, declarationProvider: KotlinDeclarationProvider) { +private fun FqName.resolveToClassIds( + to: MutableSet, + declarationProvider: KotlinDeclarationProvider, +) { toClassIdSequence().mapNotNullTo(to) { classId -> val classes = declarationProvider.getAllClassesByClassId(classId) val typeAliases = declarationProvider.getAllTypeAliasesByClassId(classId) @@ -162,4 +166,3 @@ private fun FqName.toClassIdSequence(): Sequence { } } } - diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt index b8e3498a21..fddf89d779 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.services import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex +import com.itsaky.androidide.lsp.kotlin.compiler.index.ResolutionSideKtFileAccess import com.itsaky.androidide.lsp.kotlin.compiler.index.filesForPackage import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.read @@ -33,8 +34,9 @@ import org.jetbrains.kotlin.psi.KtTypeAlias import org.jetbrains.kotlin.psi.psiUtil.isTopLevelKtOrJavaMember import java.nio.file.Paths -internal class DeclarationProviderFactory : KtLspService, KotlinDeclarationProviderFactory { - +internal class DeclarationProviderFactory : + KtLspService, + KotlinDeclarationProviderFactory { private lateinit var project: Project private lateinit var index: KtSymbolIndex @@ -42,7 +44,7 @@ internal class DeclarationProviderFactory : KtLspService, KotlinDeclarationProvi project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.project = project this.index = index @@ -50,13 +52,13 @@ internal class DeclarationProviderFactory : KtLspService, KotlinDeclarationProvi override fun createDeclarationProvider( scope: GlobalSearchScope, - contextualModule: KaModule? - ): KotlinDeclarationProvider { - return DeclarationProvider(scope, project, index) - } + contextualModule: KaModule?, + ): KotlinDeclarationProvider = DeclarationProvider(scope, project, index) } -class DeclarationProviderMerger(private val project: Project) : KotlinDeclarationProviderMerger { +class DeclarationProviderMerger( + private val project: Project, +) : KotlinDeclarationProviderMerger { override fun merge(providers: List): KotlinDeclarationProvider = providers.mergeSpecificProviders<_, DeclarationProvider>(KotlinCompositeDeclarationProvider.factory) { targetProviders -> val combinedScope = GlobalSearchScope.union(targetProviders.map { it.scope }) @@ -81,13 +83,12 @@ internal abstract class AbstractDeclarationProvider( } override fun findInternalFilesForFacade(facadeFqName: FqName): Collection = - // We don't deserialize libraries from stubs so we can return empty here safely - // We don't take the KaBuiltinsModule into account for simplicity, + // We don't deserialize libraries from stubs so we can return empty here safely + // We don't take the KaBuiltinsModule into account for simplicity, // that means we expect the kotlin stdlib to be included on the project emptyList() - override fun findFilesForFacadeByPackage(packageFqName: FqName): Collection = - ktFilesForPackage(packageFqName).toList() + override fun findFilesForFacadeByPackage(packageFqName: FqName): Collection = ktFilesForPackage(packageFqName).toList() override fun findFilesForScript(scriptFqName: FqName): Collection = ktFilesForPackage(scriptFqName).mapNotNull { it.script }.toList() @@ -98,8 +99,7 @@ internal abstract class AbstractDeclarationProvider( project.read { PsiTreeUtil.collectElementsOfType(it, KtClassOrObject::class.java).asSequence() } - } - .filter { it.getClassId() == classId } + }.filter { it.getClassId() == classId } .toList() override fun getAllTypeAliasesByClassId(classId: ClassId): Collection = @@ -108,8 +108,7 @@ internal abstract class AbstractDeclarationProvider( project.read { PsiTreeUtil.collectElementsOfType(it, KtTypeAlias::class.java).asSequence() } - } - .filter { it.getClassId() == classId } + }.filter { it.getClassId() == classId } .toList() override fun getClassLikeDeclarationByClassId(classId: ClassId): KtClassLikeDeclaration? = @@ -126,11 +125,11 @@ internal abstract class AbstractDeclarationProvider( ktFilesForPackage(callableId.packageName) .flatMap { project.read { - PsiTreeUtil.collectElementsOfType(it, KtNamedFunction::class.java) + PsiTreeUtil + .collectElementsOfType(it, KtNamedFunction::class.java) .asSequence() } - } - .filter { it.isTopLevel } + }.filter { it.isTopLevel } .filter { it.nameAsName == callableId.callableName } .toList() @@ -138,11 +137,11 @@ internal abstract class AbstractDeclarationProvider( ktFilesForPackage(packageFqName) .flatMap { project.read { - PsiTreeUtil.collectElementsOfType(it, KtClassLikeDeclaration::class.java) + PsiTreeUtil + .collectElementsOfType(it, KtClassLikeDeclaration::class.java) .asSequence() } - } - .filter { it.isTopLevelKtOrJavaMember() } + }.filter { it.isTopLevelKtOrJavaMember() } .mapNotNull { it.nameAsName } .toSet() @@ -150,11 +149,11 @@ internal abstract class AbstractDeclarationProvider( ktFilesForPackage(packageFqName) .flatMap { project.read { - PsiTreeUtil.collectElementsOfType(it, KtCallableDeclaration::class.java) + PsiTreeUtil + .collectElementsOfType(it, KtCallableDeclaration::class.java) .asSequence() } - } - .filter { it.isTopLevelKtOrJavaMember() } + }.filter { it.isTopLevelKtOrJavaMember() } .mapNotNull { it.nameAsName } .toSet() @@ -164,8 +163,7 @@ internal abstract class AbstractDeclarationProvider( project.read { PsiTreeUtil.collectElementsOfType(it, KtProperty::class.java).asSequence() } - } - .filter { it.isTopLevel } + }.filter { it.isTopLevel } .filter { it.nameAsName == callableId.callableName } .toList() } @@ -173,16 +171,16 @@ internal abstract class AbstractDeclarationProvider( internal class DeclarationProvider( val scope: GlobalSearchScope, project: Project, - private val index: KtSymbolIndex + private val index: KtSymbolIndex, ) : AbstractDeclarationProvider(project) { - override val hasSpecificCallablePackageNamesComputation = false override val hasSpecificClassifierPackageNamesComputation = false - override fun ktFilesForPackage(fqName: FqName): Sequence { - return index.filesForPackage(fqName.asString()) + @OptIn(ResolutionSideKtFileAccess::class) + override fun ktFilesForPackage(fqName: FqName): Sequence = + index + .filesForPackage(fqName.asString()) .mapNotNull { VirtualFileManager.getInstance().findFileByNioPath(Paths.get(it.filePath)) } .filter { it in scope } .mapNotNull { index.getKtFile(it) } - } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt index df24ee2918..bd69c5758c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DirectInheritorsProvider.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.services import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex +import com.itsaky.androidide.lsp.kotlin.compiler.index.ResolutionSideKtFileAccess import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence import com.itsaky.androidide.lsp.kotlin.compiler.modules.isSourceModule @@ -32,7 +33,9 @@ import org.jetbrains.kotlin.psi.psiUtil.contains import org.jetbrains.kotlin.psi.psiUtil.getImportedSimpleNameByImportAlias import org.jetbrains.kotlin.psi.psiUtil.getSuperNames -internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsProvider { +internal class DirectInheritorsProvider : + KtLspService, + KotlinDirectInheritorsProvider { private lateinit var index: KtSymbolIndex private lateinit var modules: List private lateinit var project: Project @@ -44,7 +47,7 @@ internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsPro project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.project = project this.index = index @@ -55,7 +58,7 @@ internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsPro override fun getDirectKotlinInheritors( ktClass: KtClass, scope: GlobalSearchScope, - includeLocalInheritors: Boolean + includeLocalInheritors: Boolean, ): Iterable { computeIndex() @@ -75,41 +78,48 @@ internal class DirectInheritorsProvider: KtLspService, KotlinDirectInheritorsPro } // Let's say this operation is not frequently called, if we discover it's not the case we should cache it + @OptIn(ResolutionSideKtFileAccess::class) private fun computeIndex() { classesBySupertypeName.clear() inheritableTypeAliasesByAliasedName.clear() modules .asFlatSequence() - .filter { it.isSourceModule }.flatMap { it.computeFiles(extended = true) } + .filter { it.isSourceModule } + .flatMap { it.computeFiles(extended = true) } .mapNotNull { index.getKtFile(it) } .forEach { ktFile -> - ktFile.accept(object : KtTreeVisitorVoid() { - override fun visitClassOrObject(classOrObject: KtClassOrObject) { - classOrObject.getSuperNames().forEach { superName -> - classesBySupertypeName - .computeIfAbsent(Name.identifier(superName)) { mutableSetOf() } - .add(classOrObject) + ktFile.accept( + object : KtTreeVisitorVoid() { + override fun visitClassOrObject(classOrObject: KtClassOrObject) { + classOrObject.getSuperNames().forEach { superName -> + classesBySupertypeName + .computeIfAbsent(Name.identifier(superName)) { mutableSetOf() } + .add(classOrObject) + } + super.visitClassOrObject(classOrObject) } - super.visitClassOrObject(classOrObject) - } - override fun visitTypeAlias(typeAlias: KtTypeAlias) { - val typeElement = typeAlias.getTypeReference()?.typeElement ?: return + override fun visitTypeAlias(typeAlias: KtTypeAlias) { + val typeElement = typeAlias.getTypeReference()?.typeElement ?: return - findInheritableSimpleNames(typeElement).forEach { expandedName -> - inheritableTypeAliasesByAliasedName - .computeIfAbsent(Name.identifier(expandedName)) { mutableSetOf() } - .add(typeAlias) - } + findInheritableSimpleNames(typeElement).forEach { expandedName -> + inheritableTypeAliasesByAliasedName + .computeIfAbsent(Name.identifier(expandedName)) { mutableSetOf() } + .add(typeAlias) + } - super.visitTypeAlias(typeAlias) - } - }) + super.visitTypeAlias(typeAlias) + } + }, + ) } } - private fun calculateAliases(aliasedName: Name, aliases: MutableSet) { + private fun calculateAliases( + aliasedName: Name, + aliases: MutableSet, + ) { inheritableTypeAliasesByAliasedName[aliasedName].orEmpty().forEach { alias -> val aliasName = alias.nameAsSafeName val isNewAliasName = aliases.add(aliasName) @@ -166,7 +176,13 @@ private fun findInheritableSimpleNames(typeElement: KtTypeElement): List } } } - is KtNullableType -> typeElement.innerType?.let(::findInheritableSimpleNames) ?: emptyList() - else -> emptyList() + + is KtNullableType -> { + typeElement.innerType?.let(::findInheritableSimpleNames) ?: emptyList() + } + + else -> { + emptyList() + } } -} \ No newline at end of file +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt index 7b56b13679..0b9e253042 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt @@ -215,7 +215,7 @@ internal class CurrentKtFileCacheTest : KtLspTest() { assertTrue(samePinnedInstance!!) } - @OptIn(UnpinnedKtFileAccess::class) + @OptIn(UnpinnedKtFileAccess::class, ResolutionSideKtFileAccess::class) @Test fun `getKtFile returns the current cached instance for an active document instead of reloading from disk`() { createSourceFile("I.kt", "fun i() {}") diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt index bb113c2217..25a766672e 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/LiveKtFilePinTest.kt @@ -65,7 +65,7 @@ internal class LiveKtFilePinTest : KtLspTest() { ) } - @OptIn(UnpinnedKtFileAccess::class) + @OptIn(UnpinnedKtFileAccess::class, ResolutionSideKtFileAccess::class) @Test fun `a version bump inside a pin does not install a second instance`() { val path = openDocument() @@ -87,6 +87,7 @@ internal class LiveKtFilePinTest : KtLspTest() { assertThat(doorsAgree).isTrue() } + @OptIn(ResolutionSideKtFileAccess::class) @Test fun `the resolution door keeps the pinned instance after the document is closed`() { val path = openDocument() @@ -137,6 +138,7 @@ internal class LiveKtFilePinTest : KtLspTest() { assertThat(instances.second).isEqualTo(instances.first) } + @OptIn(ResolutionSideKtFileAccess::class) @Test fun `an inner scope release does not unpin the path for the outer scope`() { val path = openDocument() diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt index cec5b58fb5..1aa91a7be1 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/StaleKtFileInstanceDiagnosticsTest.kt @@ -75,6 +75,7 @@ internal class StaleKtFileInstanceDiagnosticsTest : KtLspTest() { runBlocking { env.ktSymbolIndex.refreshCurrentKtFile(path) } } + @OptIn(ResolutionSideKtFileAccess::class) @Test fun `a version bump inside a pin cannot install a second instance`() { val path = openDocument() diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt index 319917428d..e327a32188 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -1,3 +1,5 @@ +@file:OptIn(UnpinnedAnalysis::class) + package com.itsaky.androidide.lsp.kotlin.compiler.modules import com.google.common.truth.Truth.assertThat diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTest.kt index 673504cbd1..f341656042 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTest.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.fixtures import com.itsaky.androidide.lsp.kotlin.compiler.index.toMetadata import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.UnpinnedAnalysis import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.progress.ICancelChecker @@ -70,6 +71,7 @@ abstract class KtLspTest { * Runs [action] in a dangling-aware analysis session for [ktFile], the way an interactive request * (completion, code action) does. Tests have no upstream cancellation source, hence [ICancelChecker.NOOP]. */ + @OptIn(UnpinnedAnalysis::class) internal fun analyzeMaybeDanglingForTest( ktFile: KtFile, action: KaSession.() -> R,