From 8b2b8d9d529cb434a689980128b0c0dafd910707 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 13:10:00 +0000 Subject: [PATCH 01/11] ADFA-4824: Document find-usages requirements and the COMMAND priority ADR Requirements, glossary and design for find usages in the K2 Kotlin LSP, ahead of the implementation. Follows the shape ADFA-4823 established for go-to-definition. Two decisions here reach outside lsp/kotlin and are recorded as such: - ADR 0011 adds AnalysisPriority.COMMAND between DIAGNOSTICS and INTERACTIVE. INTERACTIVE means "a newer request makes me stale, discard my work", which is false for a command the user invoked and is watching. Organize-imports and implement-members can be silently discarded by a completion request today, with no retry. - showLocations reads each result file in full once per hit, on the main thread. Find usages makes that a real cost rather than a latent one. Also corrects go-to-definition's claim that ADFA-4824 would reuse referenceAtCaret verbatim. It cannot: that helper deliberately resolves nothing when the caret is on a declaration's own name, which is exactly where find usages is invoked from. --- ...10-navigation-resolves-via-analysis-api.md | 2 + docs/adr/0011-command-analysis-priority.md | 65 +++++ docs/adr/README.md | 1 + docs/features/kotlin-find-usages.md | 251 ++++++++++++++++++ docs/features/kotlin-goto-definition.md | 4 +- 5 files changed, 321 insertions(+), 2 deletions(-) create mode 100644 docs/adr/0011-command-analysis-priority.md create mode 100644 docs/features/kotlin-find-usages.md diff --git a/docs/adr/0010-navigation-resolves-via-analysis-api.md b/docs/adr/0010-navigation-resolves-via-analysis-api.md index c72d3fad1d..78c8bcbafd 100644 --- a/docs/adr/0010-navigation-resolves-via-analysis-api.md +++ b/docs/adr/0010-navigation-resolves-via-analysis-api.md @@ -43,4 +43,6 @@ It cannot. The index stores names, kinds, visibility, and containing-class metad ## Related - [docs/features/kotlin-goto-definition.md](../features/kotlin-goto-definition.md) - the first feature built on this decision +- [docs/features/kotlin-find-usages.md](../features/kotlin-find-usages.md) - the second, which additionally has no reference-search infrastructure to fall back on: the bundled Analysis API ships no `ReferencesSearch`, no `PsiSearchHelper` and no word index +- [ADR 0011](0011-command-analysis-priority.md) - the analysis priority those features run at - [ADR 0001](0001-prefer-room-for-persistence.md) - persistence choices for the indexes this ADR declines to use diff --git a/docs/adr/0011-command-analysis-priority.md b/docs/adr/0011-command-analysis-priority.md new file mode 100644 index 0000000000..6e18909823 --- /dev/null +++ b/docs/adr/0011-command-analysis-priority.md @@ -0,0 +1,65 @@ +# 0011. User-invoked commands get their own analysis priority + +- **Status:** Proposed +- **Date:** 2026-08-03 +- **Deciders:** Code On The Go team + +## Context + +Analysis in the K2 Kotlin LSP is serialised behind one priority lock (`AnalysisScheduler`). Until now it had three tiers: + +| Priority | `supersedesSamePriority` | Preempted work | +|---|---|---| +| `INDEXING` | false | re-queued | +| `DIAGNOSTICS` | false | re-queued | +| `INTERACTIVE` | **true** | **discarded** | + +`INTERACTIVE`'s defining property is *"a newer request of the same priority makes me stale, so discard my work"*. That is exactly right for completion and signature help: they fire on keystrokes, and an in-flight result for text the user has already moved past is worthless. + +It is wrong for a command the user invoked from the code-actions menu. The user tapped a menu item and is watching a progress flashbar; the request is not stale, and discarding it silently produces a wrong answer rather than no answer. Yet three commands sat on `INTERACTIVE`: + +- `GoToDefinitionAction` - discovered the problem and worked around it with a one-shot retry (ADFA-4823). +- `OrganizeImportsAction` - no retry. A completion request discards it and it silently does nothing. +- `ImplementMembersAction` - same. + +Find usages (ADFA-4824) makes this acute. It is user-invoked, runs one analysis session per candidate file, and can take seconds across a workspace. On `INTERACTIVE` a single keystroke anywhere would discard an in-flight file's work, and two concurrent searches would discard each other. + +## Decision + +**Add a fourth priority, `COMMAND`, for user-invoked commands, ordered between `DIAGNOSTICS` and `INTERACTIVE`, with `supersedesSamePriority = false`.** + +``` +INDEXING < DIAGNOSTICS < COMMAND < INTERACTIVE +``` + +- Every user-invoked command runs at `COMMAND`: find usages, go-to-definition, organize imports, implement members. +- `supersedesSamePriority = false`, so **two commands never discard each other**; the second waits for the lock. +- Keystroke-driven features (completion, signature help) stay on `INTERACTIVE` and therefore still win against a command. +- A command preempted by `INTERACTIVE` retries. Long-running commands take their session **per unit of work** - for find usages, per candidate file - so a preemption costs one file, not the whole request. + +## Consequences + +**Positive** + +- The silent-failure bug in organize-imports and implement-members is fixed, not just in the one action that happened to notice it. +- Commands stop competing destructively with each other, which is what makes a multi-file search viable at all. +- Typing responsiveness is untouched. On a phone, completion is part of how text gets entered; starving it is the one regression a user would feel immediately. +- The priority now says what it means. `INTERACTIVE` is "stale on newer input"; `COMMAND` is "explicitly requested, must finish or be cancelled". + +**Negative / costs** + +- Commands still need a retry policy, because `INTERACTIVE` outranks them. The retry is one line at each call site and already proven in `findDefinitionAt`, but it is a rule every future command has to remember. +- Four tiers instead of three is more scheduler surface to reason about. +- Background diagnostics now lose to any command, so a long search delays diagnostics for its duration. Acceptable: diagnostics are re-queued, never discarded. + +## Alternatives considered + +- **`COMMAND` above `INTERACTIVE`** - rejected, though tempting. Nothing could preempt a command, so retries would disappear everywhere and the two buggy actions would be fixed for free. But a multi-second search would then starve the completion popup for its whole duration, and releasing the lock between files would not help - the command wins it straight back. Fixing *that* means teaching the scheduler to yield to waiting requesters between chunks, which is new machinery for a case only find usages hits. +- **Keep commands on `INTERACTIVE` and add a retry to each** - rejected: it leaves `supersedesSamePriority = true` applying to requests that are never stale, so two commands still discard each other, and every command pays for a property none of them want. +- **Flip `INTERACTIVE.supersedesSamePriority` to false** - rejected: completion genuinely needs discard-on-newer. Rapid typing would otherwise queue a chain of results for text the user has already left. + +## Related + +- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) - Kotlin navigation resolves via the Analysis API, not the symbol index +- [docs/features/kotlin-find-usages.md](../features/kotlin-find-usages.md) - the feature that forced the distinction +- `lsp/kotlin/.../compiler/modules/AnalysisScheduler.kt` - the scheduler and priority enum diff --git a/docs/adr/README.md b/docs/adr/README.md index 9bb6db0c4a..7139d240d5 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -24,3 +24,4 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0008](0008-retain-androidide-namespace.md) | Retain the `com.itsaky.androidide` namespace after rebrand | Proposed | | [0009](0009-jetpack-compose-for-new-ui.md) | Build new UI in Jetpack Compose, not XML Views | Proposed | | [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 | diff --git a/docs/features/kotlin-find-usages.md b/docs/features/kotlin-find-usages.md new file mode 100644 index 0000000000..4db34241c8 --- /dev/null +++ b/docs/features/kotlin-find-usages.md @@ -0,0 +1,251 @@ +# Kotlin find usages (K2 LSP) + +- **Ticket:** ADFA-4824 (subtask of ADFA-3317; split out of the closed ADFA-3321 "Navigation") +- **Status:** Design agreed, implementation in progress +- **Module:** `lsp/kotlin` + +From a Kotlin declaration - or from a reference to one - list every place in the workspace that uses it, across three scopes: same file, another file in the same module, another module in the workspace. + +`KotlinLanguageServer.findReferences` already exists as a stub that answers empty; this feature fills it in. Everything downstream of it (`ReferenceResult`, `IDEEditor.onFindReferencesResult`, the search-results panel) already existed for the Java server. + +The sibling feature [go-to-definition](kotlin-goto-definition.md) answers the *opposite* question and shares this feature's caret handling, symbol-to-location conversion, and test fixture. Read its Language section first; the terms below extend it rather than replace it. + +## Language + +**Usage**: +A reference that resolves into the match set. This is the unit the feature reports. +_Avoid_: reference (that is the PSI element, per go-to-definition's glossary), occurrence, hit, match. + +**Target**: +The declaration whose usages are being searched for. Derived from the caret either directly (the caret is on the declaration's own name) or by resolving the reference under the caret. +_Avoid_: symbol, subject, source, declaration (reserve that for the PSI element a reference resolves to). + +**Match set**: +The target plus every declaration a call to the target may legitimately have been written against: its **workspace-source** supers, and - when the target is a classifier - its constructors. A reference is a usage if and only if it resolves into this set. +_Avoid_: hierarchy, family, candidates (go-to-definition uses "candidate" for a resolved declaration). + +**Search scope**: +The set of modules a usage could possibly live in, derived from the target's visibility. Distinct from go-to-definition's **resolution scope** (same-file / inter-file / inter-module), which describes coverage rather than a bound. These two are easy to conflate and are deliberately named apart. +_Avoid_: scope (unqualified), visibility scope, module scope. + +**Candidate file**: +A file that survived the text prefilter and is therefore worth parsing and resolving. Most candidate files contain no usage at all - the prefilter is a cheap over-approximation. +_Avoid_: match, result, hit. + +**Workspace boundary**: +The line between declarations with source PSI in a source module and everything else (the stdlib, the framework, library jars). The match set stops at it, and so does the reportable result set. +_Avoid_: project boundary, library edge. + +## Scope + +### In scope + +Any reference, in any of the three resolution scopes, that resolves into the match set - where both the reference and the target's declaration are workspace sources. + +The **target** may be a Java-source declaration. A caret on a Kotlin reference to a workspace `.java` class or method resolves to it (go-to-definition's AC5 already covers that direction), and its Kotlin usages are found like any other target's. + +**Convention references are valid entry points.** A caret on `a + b`, on `by`, on `[`, on a `for` loop's `in`, or on a destructuring entry resolves through to `plus` / `getValue` / `get` / `iterator` / `componentN`, and the feature then searches for *named* usages of that function. This costs nothing beyond what go-to-definition already does. + +### Out of scope + +- **Implicit call sites as results.** A usage search on `operator fun plus` finds explicit `a.plus(b)` calls, not `a + b`. Discovering implicit sites would mean resolving every operator, index, call, delegate and loop expression in every file in scope, because the text of `a + b` contains no name to prefilter on. Java's find-references reports no implicit usages either. +- **`.java` files as search targets.** Kotlin declarations *are* visible to Java PSI as light classes here (`symbol-light-classes.xml` registers `KotlinAsJavaSupport`, and `JavaElementFinder` is registered), but nothing in the repo exercises Java PSI *resolution*, and the Java server has its own find-references. Kotlin call sites of a Java declaration work; Java call sites of a Kotlin declaration are not searched. +- **Usages reachable only through a subclass.** See R3 and Non-goals. +- **Binary symbols.** As with go-to-definition: no decompiler, and `showLocations` can only open a real file. A search from a reference to `listOf` finds nothing. +- **Test sources.** Not a choice made here - `AndroidModule.getSourceDirectories()` returns `mainSourceSet` only, so `src/test/**` and `src/androidTest/**` are not content roots for *any* Kotlin LSP feature. + +## Requirements + +**R1 - Trigger.** A "Find references" item appears in the Kotlin code-actions menu, mirroring Java's. `FindReferencesAction` extends `BaseKotlinCodeAction`, id `ide.editor.lsp.kt.findReferences`, reuses `R.string.action_find_references`, and delegates to `ILspEditor.findReferences()`. Registered in `KotlinCodeActionsMenu` immediately after `GoToDefinitionAction`, matching Java's ordering. + +It carries its own tooltip tag, `EDITOR_CODE_ACTIONS_KT_FIND_REFS = "editor.codeactions.kotlin.findrefs"`, not Java's `EDITOR_CODE_ACTIONS_FIND_REFS` - the same split go-to-definition made, so Kotlin and Java can carry different tooltip text. The tooltips database is not in this repo, so the tag shows no text until a row exists for it; that row is a hand-off item, not code. + +The item is **always visible** for `.kt`/`.kts` and never conditioned on what the caret is sitting on: deciding "is there a target here" needs PSI and the project lock, and `prepare()` runs on the UI thread. A caret on whitespace therefore flashes "No references found". A `.kts` file shows the item and it does nothing, because a script has no `CompilationEnvironment` - identical to go-to-definition. + +**R2 - Target at caret.** The caret maps to a target declaration by trying, in order: + +1. **The caret is on a declaration's own name** - the leaf is the `nameIdentifier` of a `KtNamedDeclaration`. That declaration is the target. +2. **The caret is on a reference** - delegate to go-to-definition's `referenceAtCaret`, then resolve it to its declaration, which becomes the target. + +Order matters, and it makes the two features answer differently from one identical caret. For `val (x, y) = p` with the caret on `x`, go-to-definition navigates to `component1`; find usages targets the local `x`. That is deliberate: `x` is both a declaration and a convention reference, and each feature wants the reading that is useful to it. + +`referenceAtCaret` cannot be reused for step 1. It is built so that a caret on a declaration's own name resolves nothing - go-to-definition's no-self-jump rule - which is precisely the caret position find usages is normally invoked from. Step 1 is therefore a new, separate check; the token accept-list and the `offset - 1` retry are shared. + +**R3 - Match set.** Assembled once, in the caret's analysis session: + +- The target symbol, normalised through `fakeOverrideOriginal`. A call `derived.foo()` where `Derived` does not redeclare `foo` resolves to a substituted fake override, not to `Base.foo`, so both sides of every comparison are normalised. +- Its supers, via `allOverriddenSymbols`, **stopping at the workspace boundary**. So a call dispatched through a workspace `Base.foo` counts as a usage of `Derived.foo`. Library supers are excluded: including them would make a usage search on an overridden `toString` match every `.toString()` call in the workspace, and a library super can never yield a reportable result anyway. +- When the target is a classifier, its **constructors**. Otherwise `Foo()` - which resolves to the constructor, not the class (go-to-definition's R4) - would not count as a usage of `class Foo`, and the feature would miss every instantiation. The reverse expansion is not applied: a target that *is* a specific constructor stays that constructor, because asking for usages of one overload is a deliberate act. + +The walk goes **up** only. Usages reachable solely through a subclass (`Base.foo` searched, `derived.foo()` written) are not found - that needs a workspace inheritor search, and `DirectInheritorsProvider.computeIndex()` rebuilds its entire index on every call. + +**Import directives count as usages.** `import a.b.Foo` resolves to `Foo`, so it is one by construction. The panel has no categories to separate them into, and the noise is bounded at one hit per importing file. + +**R4 - Search scope.** Derived from the target's visibility, which is an exact bound rather than a heuristic: + +| Target | Scope | +|---|---| +| local val/var, parameter, local fun, local class, loop variable | containing file | +| `private` top-level declaration | containing file (Kotlin private top-level is file-private) | +| `private` class/object member | containing file | +| `internal` | the target's module | +| `protected`, `public`, default | the target's module + its transitive dependents (`KotlinModuleDependentsProvider.getTransitiveDependents`) | + +The ticket's three resolution scopes fall out of this one code path rather than being three implementations. Cheap cases stay cheap: a search on a local variable never leaves the open file. + +`internal` needs no widening for test sources. There is no test module to widen to - `collectKtModules` builds one `KtSourceModule` per Gradle module from `mainSourceSet` only, and `directFriendDependencies` is empty everywhere. + +**R5 - Candidate discovery.** Two tiers, because find usages is run *while* editing and unsaved text must not be invisible: + +| File | Prefilter text | PSI | +|---|---|---| +| open in the editor | `FileManager.getDocumentContents(path)` - the live buffer | `ktSymbolIndex.getCurrentKtFile(path).await()`, awaited **outside** `project.read` | +| everything else | disk, via `StringSearch.containsWord` | `ktSymbolIndex.getKtFile(vf)` | + +The prefilter is word-boundary exact on the target's simple name. Its errors are one-directional: a file that mentions the name but contains no usage is parsed and discarded (wasted work, correct result), while a file that does not mention the name cannot contain a named usage. + +Open documents are tab-count many, so the live tier is free. Without it, a usage the user just typed would be missed entirely - the prefilter would never select the file, so it would never be parsed. + +**R6 - Identity.** A reference is a usage if its resolved symbol is in the match set. Deciding that across files needs care, because `KaSymbol` is session-scoped and the same declaration exists as two PSI instances - the on-disk `KtFile` cached in the index, and the dangling `KtFile` built from the editor buffer for an open file. + +Matching therefore uses `KaSymbolPointer`: `createPointer()` for each match-set member in the caret's session, then `restoreSymbol(session)` **once per candidate session**, then `==` against each resolved candidate symbol inside that session. This is the platform's cross-session identity mechanism, with structural implementations per symbol kind, and it is the direct analogue of the Java server re-deriving its target `Element` inside each compile task. + +Locals skip all of it: R4 confines them to one file and therefore one session, where instance equality is valid and cheapest. + +A pointer that fails to restore drops that session's candidates, with a log. That under-reports rather than reporting something false, which is the safe direction, and it is tested. + +Neither a PSI identity check nor a (file, offset) key works here. Both break exactly when the target's own file has unsaved edits: the live PSI and the on-disk PSI disagree about offsets, so every cross-file usage would be silently missed - and editing-then-searching is the common case. + +**R7 - Results.** Each usage becomes a `Location` whose range covers the reference's **name identifier** (`foo` in `a.b.foo()`, `Foo` in `Foo()`), matching go-to-definition's R6. Deduplicated by file plus range, ordered by file path then start offset. + +`includeDeclaration` is **ignored**, and the target's own declaration is never emitted. Java's provider ignores it too. Honouring it would also create a trap: a declaration with no usages would return exactly one location in the current file, which `onFindReferencesResult` turns into a silent `setSelection` on the declaration the caret is already on - indistinguishable from a broken no-op. Returning empty flashes "No references found", which is true. + +There is **no result cap**. See R10 for why one is not needed. + +**R8 - Result handling.** The server returns `ReferenceResult(locations)`; `IDEEditor.onFindReferencesResult` applies unchanged: + +- empty -> flash `msg_no_references` +- one location in the current file -> `setSelection` +- otherwise -> `languageClient.showLocations`, the grouped search-results panel + +**R9 - Scheduling.** The request runs at the new `AnalysisPriority.COMMAND` ([ADR 0011](../adr/0011-command-analysis-priority.md)), behind the editor's existing cancellable progress flashbar (`msg_finding_references`). + +Granularity is per candidate file, and it is load-bearing: + +- **One analysis session per candidate file.** A preemption by completion costs one file's work, which is retried once - `findDefinitionAt`'s pattern. One session for the whole search would let a single keystroke discard a whole-workspace scan. +- **`project.read` per candidate file, never once for the search.** A whole-workspace search holding the read lock start to finish would block every `project.write`, which is what index refresh needs. +- **The live-document await stays outside `project.read`.** The refresh it waits on needs `project.write`; awaiting it under the read lock deadlocks. Go-to-definition's R10 records the same constraint. +- `params.cancelChecker` is honoured between files **and** between references within a file. + +The prefilter pass runs first, before any analysis, holding no locks. No progress count is shown - `launchCancellableAsyncWithProgress` takes a fixed `@StringRes`, and threading a live count through it would change a shared editor API for a cosmetic gain. No timeout and no file budget: the search finishes or the user cancels. + +**R10 - Panel cost.** `IDELanguageClientImpl.showLocations` currently reads each result file **in full, once per hit, on the main thread** (`FileIOUtils.readFile2String` inside the per-location loop). That is a main-thread I/O violation and O(hits) file reads; Java's find-references has it today and simply rarely produces enough hits to hurt. + +Rewritten to: group locations by file, then one sequential `BufferedReader` pass per file pulling only the lines its ranges touch, building the `SearchResult`s and retaining nothing before moving on. A file with an open editor uses that editor's live `Content` - no read, no extra memory, and correct for unsaved edits. The whole map is built off the main thread; only `handleSearchResults` touches the UI. + +Reads drop from O(hits) to O(files), peak memory is one line rather than one file (deliberately *not* a per-file content cache - holding every result file's text at once is the wrong trade on a phone), and the main thread does no I/O. This removes the need for a result cap, which would otherwise silently truncate. One behaviour change: a stale location whose line no longer exists is dropped rather than yielding whatever `Content` returned. + +**R11 - Not ready.** No `CompilationEnvironment` for the file (a script, a file outside the content roots), or no analysis session yet, answers empty and logs. There is no "still indexing" signal; that gap is cross-cutting across every LSP feature and is not solved here. + +**R12 - Failure isolation.** A resolution failure on one candidate file drops that file and continues - one unparseable file must not lose the whole result. A failure in the target-resolution phase returns empty. Cancellation, including `AnalysisPreemptedException`, propagates rather than being reported as "no references". Nothing propagates an exception to the editor or leaves the progress flashbar up. + +## Non-goals + +- **Rename / safe-delete**, or anything that edits the usages found. +- **Usages via subclasses** (the down-walk). Blocked on `DirectInheritorsProvider.computeIndex()` being cached; filed separately. +- **Searching `.java` files** for usages of a Kotlin declaration. Filed separately. +- **Usages in test source sets.** Filed separately, as an LSP-wide content-root gap. +- **Implicit call sites as results** (see Scope). +- **Library-source usages**, via decompilation or `-sources.jar`. +- **Categorising results** (imports vs calls vs type references) - the panel has no grouping beyond file. +- **A partiality signal.** `ReferenceResult` is shared with the Java and XML servers and has no field for it, and `showLocations` has no header slot; the same caveat already applies silently to test sources. +- **A gesture trigger.** Editor-wide UX change that would apply to Java too. + +## Acceptance criteria + +1. "Find references" appears in a Kotlin file's code-actions menu and is absent in a non-Kotlin file. +2. Same-file: a local function's call sites are listed. +3. Inter-file: usages of a class in a sibling file of the same module are listed. +4. Inter-module: usages in a dependent module are listed. +5. Invoked from a **reference** rather than a declaration, the result is the same set. +6. A `private` top-level declaration reports no usages from another file, even when that file contains a same-named unrelated declaration. +7. An `internal` declaration reports usages within its module only. +8. A local variable's usages are confined to its file. +9. `Foo()` is reported as a usage of `class Foo`. +10. An `import` of the target is reported as a usage. +11. A call dispatched via a workspace `Base.foo` is reported as a usage of `Derived.foo`. +12. A usage search on an override of `toString` does **not** report unrelated `.toString()` calls. +13. A usage typed into an open, unsaved file is reported. +14. A target with no usages flashes "No references found". +15. The target's own declaration never appears in the results. +16. Cancelling the progress flashbar mid-search leaves the editor responsive and unchanged. +17. Typing during a search does not discard it. +18. A search from a reference to a stdlib or framework symbol flashes "No references found". +19. A caret on whitespace, in a comment, or on a non-navigable keyword produces no search. +20. Invoking before the project finishes loading flashes "No references found" and does not crash or hang. +21. A result set spanning many files opens the panel without a main-thread stall. + +## Design + +Resolution goes through the Analysis API and PSI only; the symbol indexes are never consulted - see [ADR 0010](../adr/0010-navigation-resolves-via-analysis-api.md). That decision is load-bearing here for a second reason: there is no reference-search infrastructure to fall back on. `analysis-api-standalone-embeddable-for-ide` ships no `ReferencesSearch`, no `PsiSearchHelper` and no word index, and `KtFileMetadata` records declarations only. The search is built here. + +``` +FindReferencesAction.execAction lsp/kotlin/actions + -> ILspEditor.findReferences() editor (unchanged: progress flashbar + cancel checker) + -> KotlinLanguageServer.findReferences(params) + guards: settings.referencesEnabled(), DocumentUtils.isKotlinFile + compilationEnvironmentFor(params.file) ?: empty [R11] + -> context(env) { findUsagesAt(params) } navigation/FindUsages.kt + ktFile = env.ktSymbolIndex.getCurrentKtFile(file).await() ?: empty [R5, R11] + env.project.read { + target = targetAtCaret(ktFile, offset) navigation/TargetAtCaret.kt [R2] + analyzeMaybeDangling(ktFile, COMMAND, cancelChecker) { + matchSet(target) -> List [R3, R6] + } + } + scopeOf(target) -> modules [R4] + prefilter(modules, target.name) -> candidate files [R5] + per candidate file: [R9] + await live PSI if open (outside project.read) + env.project.read { + analyzeMaybeDangling(file, COMMAND, cancelChecker) { + restore pointers once, walk name references, compare [R6] + } + } -> locations [R7] + <- ReferenceResult(locations) [R8] +``` + +New components: + +- **`navigation/TargetAtCaret.kt`** - `targetAtCaret(file: KtFile, offset: Int): KtElement?`. Pure PSI, no analysis session, so R2's caret rules are testable without one. Shares `ReferenceAtCaret.kt`'s token accept-list and `offset - 1` retry, which become `internal` rather than private. +- **`navigation/FindUsages.kt`** - the match set, the visibility-derived scope, the prefilter, the per-file resolve loop, and symbol-to-`Location` conversion, reusing go-to-definition's range helper. + +Touched existing components: + +- **`KotlinLanguageServer.findReferences`** - the stub's guards stay; it now delegates inside the file's `CompilationEnvironment`, matching how `findDefinition` and `signatureHelp` dispatch. +- **`navigation/ReferenceAtCaret.kt`** - visibility loosened for reuse. Behaviour unchanged, and its existing tests are kept as the proof of that. +- **`AnalysisPriority` / `AnalysisScheduler`** - the new `COMMAND` tier ([ADR 0011](../adr/0011-command-analysis-priority.md)). +- **`GoToDefinitionAction`, `OrganizeImportsAction`, `ImplementMembersAction`** - migrated to `COMMAND`; the latter two gain the retry they never had. +- **`IDELanguageClientImpl.showLocations`** - R10's grouped streaming rewrite. The grouping and line extraction are extracted into a pure helper so they can be unit-tested; the activity call stays a thin shell. +- **`TooltipTag`** - one new constant (R1). + +Unchanged: `ReferenceParams`/`ReferenceResult`, `ILanguageServer`, `IDEEditor`, and every string resource. + +## Verification + +Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest`), split to match the helpers: + +- **`TargetAtCaretTest`** - PSI only, no session. Caret on a declaration's own name; caret on a reference; whitespace / comment / non-navigable keyword; one past an identifier; a destructuring entry targeting the local rather than `componentN`. +- **`ReferenceAtCaretTest`** - kept as-is, as the regression proof that loosening visibility changed no behaviour. +- **`FindUsagesTest`** - the `lib` + `app(dependsOn = lib)` fixture from ADFA-4823: the three resolution scopes; each row of R4's visibility ladder, including a same-named decoy in another file; R3's super-walk, fake-override normalisation, workspace-boundary cutoff and constructor expansion; a Java-source target; dedup, ordering and ranges; the declaration's absence; a pre-cancelled `cancelChecker` returning empty without resolving; and a usage in an open unsaved file (via the `enableParserEventSystem = true` fixture). +- **`KotlinCodeActionTooltipTagTest`** - the new tag row. +- **The `showLocations` helper** - one read per file, hits grouped by file, a stale line past EOF dropped. + +Not unit-testable, so covered by on-device QA via the "Steps to QA" field on ADFA-4824: the menu item and its tooltip tag, the panel with a large result set, cancelling mid-search, and typing during a search without losing it. + +## Related + +- [docs/features/kotlin-goto-definition.md](kotlin-goto-definition.md) - the sibling feature whose helpers and fixture this reuses +- [ADR 0010](../adr/0010-navigation-resolves-via-analysis-api.md) - navigation resolves via the Analysis API, not the symbol index +- [ADR 0011](../adr/0011-command-analysis-priority.md) - user-invoked commands get their own analysis priority +- [ARCHITECTURE.md](../../ARCHITECTURE.md) diff --git a/docs/features/kotlin-goto-definition.md b/docs/features/kotlin-goto-definition.md index 30cf72a7ba..8bd6d0c872 100644 --- a/docs/features/kotlin-goto-definition.md +++ b/docs/features/kotlin-goto-definition.md @@ -111,7 +111,7 @@ Both rules are enforced by construction rather than by filtering afterwards: an ## Non-goals -- **Find usages** - ADFA-4824, the sibling subtask. It will share the reference-at-caret resolution helper. +- **Find usages** - ADFA-4824, the sibling subtask; see [kotlin-find-usages.md](kotlin-find-usages.md). - **Go-to-implementation.** A call through an interface or abstract member resolves to the declaring member only. Walking down to overriding implementations needs an inheritance search over the workspace. - **Go-to-super.** - **Library-source navigation**, via decompilation, generated stubs, or `-sources.jar` extraction. @@ -161,7 +161,7 @@ The dispatch mirrors `signatureHelp` line for line, which is what buys R3 and R1 Touched components: - **`KotlinLanguageServer.findDefinition`** - guards stay (`definitionsEnabled()`, `isKotlinFile`), then delegates inside the file's `CompilationEnvironment`, matching how `signatureHelp` and `analyze` already dispatch. A `.kts` has no environment, so the lookup returns null there and the request answers empty. -- **`navigation/ReferenceAtCaret.kt`** - `referenceAtCaret(file: KtFile, offset: Int): KtElement?`. Pure PSI, no analysis session: the caret-token accept-list, the `offset - 1` retry, and the two-level climb (R2). ADFA-4824 imports this verbatim; it needs the reference element, not the declarations. +- **`navigation/ReferenceAtCaret.kt`** - `referenceAtCaret(file: KtFile, offset: Int): KtElement?`. Pure PSI, no analysis session: the caret-token accept-list, the `offset - 1` retry, and the two-level climb (R2). ADFA-4824 reuses its accept-list and retry, but not the function: this deliberately resolves nothing when the caret is on a declaration's own name, which is exactly where find usages is invoked from. See [kotlin-find-usages.md](kotlin-find-usages.md) R2. - **`navigation/GoToDefinition.kt`** - `findDefinitionAt(params)` under `context(env: CompilationEnvironment)`. The two symbol paths (R4), then symbol -> source PSI -> name-identifier range -> `Location`, with dedup, ordering, cancellation and failure isolation (R5, R6, R10, R11). - **`GoToDefinitionAction` in `lsp/kotlin/actions`** extending `BaseKotlinCodeAction`, id `ide.editor.lsp.kt.gotoDefinition` (the prefix every other Kotlin action uses), `requiresUIThread = true` like Java's, registered in `KotlinCodeActionsMenu` after the comment actions - the same slot Java uses. - **`TooltipTag.EDITOR_CODE_ACTIONS_KT_GOTO_DEF`** - one new constant (R1). From d9f15cf7bce149219d6b85c04180c56e005a0f15 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 13:34:32 +0000 Subject: [PATCH 02/11] ADFA-4824: Give user-invoked commands their own analysis priority INTERACTIVE means "a newer request of the same priority makes me stale, so discard my work". That is right for completion and signature help, which fire on keystrokes. It is wrong for a command the user invoked from the code-actions menu and is watching a progress flashbar for: the request is not stale, so discarding it produces a wrong answer rather than no answer. Three commands ran at INTERACTIVE anyway. Go-to-definition noticed and worked around it with a one-shot retry. Organize-imports and implement-members did not: a completion request discards them, the AnalysisPreemptedException lands in their outer runCatching, and the action silently does nothing. Adds COMMAND between DIAGNOSTICS and INTERACTIVE with supersedesSamePriority = false, so two commands never discard each other, and migrates all three actions to it. Ordered below INTERACTIVE deliberately: a long command must not starve the completion popup, which on a phone is part of how text gets entered. See ADR 0011 for the rejected alternative of ordering it above. The cost of that ordering is that commands stay preemptable, so each one retries. Extracts retryingOnPreemption to hold the two invariants that retry depends on: a fresh ScheduledCancelChecker per attempt (preempt() latches, so a reused checker aborts the retry at its first checkpoint), and re-fetching the KtFile inside the attempt (the preemptor also refreshed the live PSI, unregistering the file the previous attempt held). The two migrated actions now take the delegate ICancelChecker rather than a pre-wrapped ScheduledCancelChecker, since the wrapping is per attempt. Prep for find usages, which is the case that makes this acute: it is user-invoked, takes one session per candidate file, and can run for seconds, so on INTERACTIVE a single keystroke would discard it. --- .../kotlin/actions/ImplementMembersAction.kt | 41 +++-- .../kotlin/actions/OrganizeImportsAction.kt | 29 ++-- .../compiler/modules/AnalysisScheduler.kt | 59 ++++++- .../lsp/kotlin/navigation/GoToDefinition.kt | 70 ++++---- .../modules/AnalysisSerializationTest.kt | 157 ++++++++++++++++++ .../utils/ImplementMembersEndToEndTest.kt | 3 +- .../utils/OrganizeImportsEndToEndTest.kt | 9 +- 7 files changed, 290 insertions(+), 78 deletions(-) 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 69a5673035..94c29c7ec1 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt @@ -8,8 +8,8 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority -import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.membersToImplement import com.itsaky.androidide.lsp.kotlin.utils.renderOverrideStub @@ -20,6 +20,7 @@ import com.itsaky.androidide.lsp.models.Command import com.itsaky.androidide.lsp.models.DocumentChange import com.itsaky.androidide.lsp.models.TextEdit import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.resources.R import com.itsaky.androidide.tasks.createJobCancelChecker import org.jetbrains.kotlin.analysis.api.symbols.KaClassKind @@ -52,7 +53,7 @@ class ImplementMembersAction : BaseKotlinCodeAction() { val offset = data.requireEditor().cursor.left val env = server.compilationEnvironmentFor(nioPath) ?: return emptyList() // Ties the analysis to this action's coroutine: cancelling the action aborts the queued analysis. - return computeImplementMembersEdit(env, nioPath, offset, ScheduledCancelChecker(createJobCancelChecker())) + return computeImplementMembersEdit(env, nioPath, offset, createJobCancelChecker()) } /** @@ -70,23 +71,29 @@ class ImplementMembersAction : BaseKotlinCodeAction() { env: AbstractCompilationEnvironment, nioPath: Path, offset: Int, - cancelChecker: ScheduledCancelChecker, + cancelChecker: ICancelChecker, ): List = runCatching { - val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return emptyList() - env.project.read { - val classOrObject = findEnclosingClassOrObject(ktFile, offset) ?: return@read emptyList() - analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { - val classSymbol = classOrObject.symbol as? KaClassSymbol ?: return@analyzeMaybeDangling emptyList() - if (!isImplementable(classSymbol)) return@analyzeMaybeDangling emptyList() - - val classIndent = classIndentOf(ktFile, classOrObject) - val unit = detectIndentUnit(ktFile.text) - val memberIndent = memberIndentOf(ktFile, classOrObject, classIndent, unit) - val stubs = membersToImplement(classSymbol).mapNotNull { renderOverrideStub(it, memberIndent, unit) } - if (stubs.isEmpty()) return@analyzeMaybeDangling emptyList() - - buildInsertionEdit(ktFile, classOrObject, stubs, classIndent) + // A user-invoked command: AnalysisPriority.COMMAND, retried once if keystroke-driven work + // preempts it (ADR 0011). Without the retry a preemption fell into the getOrElse below and the + // action silently inserted nothing. The file is re-fetched per attempt because the preemptor + // also refreshed the live PSI. + retryingOnPreemption(cancelChecker, "Implement members for $nioPath") { checker -> + val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return@retryingOnPreemption emptyList() + env.project.read { + val classOrObject = findEnclosingClassOrObject(ktFile, offset) ?: return@read emptyList() + analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, checker) { + val classSymbol = classOrObject.symbol as? KaClassSymbol ?: return@analyzeMaybeDangling emptyList() + if (!isImplementable(classSymbol)) return@analyzeMaybeDangling emptyList() + + val classIndent = classIndentOf(ktFile, classOrObject) + val unit = detectIndentUnit(ktFile.text) + val memberIndent = memberIndentOf(ktFile, classOrObject, classIndent, unit) + val stubs = membersToImplement(classSymbol).mapNotNull { renderOverrideStub(it, memberIndent, unit) } + if (stubs.isEmpty()) return@analyzeMaybeDangling emptyList() + + buildInsertionEdit(ktFile, classOrObject, stubs, classIndent) + } } } }.getOrElse { e -> diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt index 1294b3259d..bbb6afc113 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.kt @@ -7,8 +7,8 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority -import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.collectImportUsage import com.itsaky.androidide.lsp.kotlin.utils.organizedImportBlock @@ -19,6 +19,7 @@ import com.itsaky.androidide.lsp.models.Command import com.itsaky.androidide.lsp.models.DocumentChange import com.itsaky.androidide.lsp.models.TextEdit import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.resources.R import com.itsaky.androidide.tasks.createJobCancelChecker import org.slf4j.LoggerFactory @@ -41,7 +42,7 @@ class OrganizeImportsAction : BaseKotlinCodeAction() { val nioPath = data.requireFile().toPath() val env = server.compilationEnvironmentFor(nioPath) ?: return emptyList() // Ties the analysis to this action's coroutine: cancelling the action aborts the queued analysis. - return computeOrganizeEdit(env, nioPath, ScheduledCancelChecker(createJobCancelChecker())) + return computeOrganizeEdit(env, nioPath, createJobCancelChecker()) } /** @@ -57,17 +58,23 @@ class OrganizeImportsAction : BaseKotlinCodeAction() { internal fun computeOrganizeEdit( env: AbstractCompilationEnvironment, nioPath: Path, - cancelChecker: ScheduledCancelChecker, + cancelChecker: ICancelChecker, ): List = runCatching { - val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return emptyList() - if (ktFile.importDirectives.isEmpty()) return emptyList() - env.project.read { - val usage = analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { collectImportUsage(ktFile) } - val newText = organizedImportBlock(ktFile, usage) ?: return@read emptyList() - val range = ktFile.importList?.textRange?.toRange(ktFile) ?: return@read emptyList() - if (range == Range.NONE) return@read emptyList() - listOf(TextEdit(range, newText)) + // A user-invoked command: AnalysisPriority.COMMAND, retried once if keystroke-driven work + // preempts it (ADR 0011). Without the retry a preemption fell into the getOrElse below and + // organize-imports silently did nothing. The file is re-fetched per attempt because the + // preemptor also refreshed the live PSI. + retryingOnPreemption(cancelChecker, "Organize imports for $nioPath") { checker -> + val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return@retryingOnPreemption emptyList() + if (ktFile.importDirectives.isEmpty()) return@retryingOnPreemption emptyList() + env.project.read { + val usage = analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, checker) { collectImportUsage(ktFile) } + val newText = organizedImportBlock(ktFile, usage) ?: return@read emptyList() + val range = ktFile.importList?.textRange?.toRange(ktFile) ?: return@read emptyList() + if (range == Range.NONE) return@read emptyList() + listOf(TextEdit(range, newText)) + } } }.getOrElse { e -> logger.warn("Failed to organize imports", e) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt index a4f3afef95..61dcf1b408 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt @@ -1,6 +1,8 @@ package com.itsaky.androidide.lsp.kotlin.compiler.modules import com.itsaky.androidide.progress.ICancelChecker +import org.slf4j.Logger +import org.slf4j.LoggerFactory import java.util.concurrent.CancellationException import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.TimeUnit @@ -12,20 +14,37 @@ import kotlin.concurrent.withLock * lower-priority analysis that is currently running, and is served before any lower-priority request * that is merely waiting. * - * Order: [INDEXING] < [DIAGNOSTICS] < [INTERACTIVE] — interactive requests (completion, signature - * help) beat background diagnostics, which beats bulk indexing. + * Order: [INDEXING] < [DIAGNOSTICS] < [COMMAND] < [INTERACTIVE] — keystroke-driven requests + * (completion, signature help) beat user-invoked commands, which beat background diagnostics, which + * beat bulk indexing. * * [supersedesSamePriority] additionally lets a *newer* request preempt an in-flight one of the * **same** priority. On for [INTERACTIVE] only: rapid typing makes the in-flight request stale, so * the newer one cancels it and the superseded work is *discarded* (nothing reschedules it). Off for - * [DIAGNOSTICS]/[INDEXING], whose preempted work is re-queued — there same-priority preemption would - * livelock, two contenders endlessly re-queuing and re-preempting each other. + * the rest, whose preempted work is re-queued — there same-priority preemption would livelock, two + * contenders endlessly re-queuing and re-preempting each other. */ internal enum class AnalysisPriority( val supersedesSamePriority: Boolean, ) { INDEXING(supersedesSamePriority = false), DIAGNOSTICS(supersedesSamePriority = false), + + /** + * A command the user invoked from the code-actions menu: find usages, go-to-definition, organize + * imports, implement members. Distinct from [INTERACTIVE] because such a request is never *stale* — + * the user tapped a menu item and is watching a progress flashbar, so discarding the work produces + * a wrong answer rather than no answer. Hence [supersedesSamePriority] is off: two commands must + * not discard each other. + * + * Ordered below [INTERACTIVE] so a long command never starves the completion popup, which on a + * phone is part of how text gets entered. The cost is that a command *can* be preempted, so its + * call site must retry — and a long-running one should take the lock per unit of work (find usages + * takes it per candidate file) so a preemption costs one unit rather than the whole request. + * + * See ADR 0011 (docs/adr/0011-command-analysis-priority.md). + */ + COMMAND(supersedesSamePriority = false), INTERACTIVE(supersedesSamePriority = true), } @@ -96,6 +115,38 @@ internal class ScheduledCancelChecker( } } +/** + * Runs [attempt] and, if it was preempted, runs it exactly once more. + * + * The retry policy every [AnalysisPriority.COMMAND] call site needs. A command is preempted by + * keystroke-driven work ([AnalysisPriority.INTERACTIVE]), which - unlike a genuine cancellation - + * leaves the user's own request alive, so reporting the empty/failed result would be a lie: "no + * references" for a symbol that has plenty, or a silently skipped organize-imports. + * + * Two details this centralises: + * - **A fresh [ScheduledCancelChecker] per attempt.** [ScheduledCancelChecker.preempt] latches, so + * reusing the checker would make the retry abort at its first checkpoint. + * - **The whole pipeline is retried, not just the `analyze` block.** Whatever preempted the first + * attempt also refreshed the live PSI, unregistering the `KtFile` that attempt held; re-analyzing + * that stale file fails. So [attempt] must re-fetch the file too. + * + * A second preemption propagates - this is one retry, not a loop. + */ +internal inline fun retryingOnPreemption( + delegate: ICancelChecker, + label: String, + attempt: (ScheduledCancelChecker) -> R, +): R = + try { + attempt(ScheduledCancelChecker(delegate)) + } catch (e: AnalysisPreemptedException) { + schedulerLogger.debug("{} preempted; retrying once", label) + attempt(ScheduledCancelChecker(delegate)) + } + +@PublishedApi +internal val schedulerLogger: Logger = LoggerFactory.getLogger("AnalysisScheduler") + /** * A process-global, priority-aware, preemptive lock that serializes all Kotlin Analysis API access. * 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 9380913215..4500b4506f 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.kt @@ -3,10 +3,10 @@ package com.itsaky.androidide.lsp.kotlin.navigation import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority -import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation +import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.rangeOf import com.itsaky.androidide.lsp.kotlin.utils.toRange @@ -197,47 +197,35 @@ internal suspend fun findDefinitionAt(params: DefinitionParams): DefinitionResul return try { val offset = params.position.requireIndex() - // Navigation is user-initiated: 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. - // - // INTERACTIVE.supersedesSamePriority is true, so a concurrent completion/signature-help - // request can preempt this lookup even though the user's own request is still alive - unlike - // a genuine cancellation, that coroutine survives, so surfacing an empty result would be a lie - // ("Definition not found" for a reference that resolves fine). One retry, with a fresh - // checker, covers it without turning this into a retry loop. - suspend fun attempt(): List { - // Awaited per attempt, not once: whatever preempted the first attempt also refreshed the - // live PSI, unregistering the KtFile that attempt held, and analyzing it again would fail. - // - // Safe to await a (possibly blocking) refresh here: this runs outside any project.read/write - // block, so it can't deadlock against the refresh's project.write. Refreshed to the open - // document's current version, so the caret offset and the PSI it indexes into come from the - // same text - a stale snapshot points at the wrong element. (params.position is fixed by the - // request, so a retry after the user typed can still be one edit behind; that resolves to - // the wrong element or to nothing, never to a crash.) - val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await() - if (ktFile == null) { - logger.warn("File {} cannot be loaded for definition lookup", params.file) - return emptyList() - } - - val cancelChecker = ScheduledCancelChecker(params.cancelChecker) - cancelChecker.abortIfCancelled() - return env.project.read { - val element = referenceAtCaret(ktFile, offset) ?: return@read emptyList() - analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { - definitionLocations(element, cancelChecker) - } - } - } - + // 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 = - try { - attempt() - } catch (e: AnalysisPreemptedException) { - logger.debug("Definition lookup for {} preempted; retrying once", params.file) - attempt() + retryingOnPreemption(params.cancelChecker, "Definition lookup for ${params.file}") { cancelChecker -> + // Awaited per attempt, not once: whatever preempted the first attempt also refreshed the + // live PSI, unregistering the KtFile that attempt held, and analyzing it again would fail. + // + // Safe to await a (possibly blocking) refresh here: this runs outside any project.read/write + // block, so it can't deadlock against the refresh's project.write. Refreshed to the open + // document's current version, so the caret offset and the PSI it indexes into come from the + // same text - a stale snapshot points at the wrong element. (params.position is fixed by the + // request, so a retry after the user typed can still be one edit behind; that resolves to + // the wrong element or to nothing, never to a crash.) + val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await() + if (ktFile == null) { + logger.warn("File {} cannot be loaded for definition lookup", params.file) + emptyList() + } else { + cancelChecker.abortIfCancelled() + env.project.read { + val element = referenceAtCaret(ktFile, offset) ?: return@read emptyList() + analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { + definitionLocations(element, cancelChecker) + } + } + } } logger.debug("Definition result for {}: {} location(s)", params.file, locations.size) DefinitionResult(locations) 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 72d55ce4d2..94e4f33a5d 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 @@ -418,6 +418,163 @@ class AnalysisSerializationTest : KtLspTest() { assertThat(newerRan.get()).isTrue() } + /** + * ADR 0011's central property. Two user-invoked commands must not discard each other - before + * [AnalysisPriority.COMMAND] existed they both ran at [AnalysisPriority.INTERACTIVE], where the + * newer one superseded the older and the older silently produced nothing. + */ + @Test(timeout = 10_000) + fun `a command does not supersede an in-flight command`() { + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val secondEntered = AtomicBoolean(false) + + val first = + Thread { + withAnalysisLock(AnalysisPriority.COMMAND, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + first.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + val second = + Thread { + withAnalysisLock(AnalysisPriority.COMMAND, ScheduledCancelChecker(ICancelChecker.NOOP)) { + secondEntered.set(true) + } + } + second.start() + + // Give the second command time to (incorrectly) barge in. + Thread.sleep(300) + val enteredWhileHeld = secondEntered.get() + + release.countDown() + first.join(5_000) + second.join(5_000) + + assertThat(enteredWhileHeld).isFalse() + assertThat(secondEntered.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `a command preempts an in-flight diagnostics`() { + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val commandRan = AtomicBoolean(false) + + val diagnostics = + Thread { + try { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, holderChecker) { + holding.countDown() + repeat(2_000) { + holderChecker.abortIfCancelled() + Thread.sleep(5) + } + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + diagnostics.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + val command = + Thread { + withAnalysisLock(AnalysisPriority.COMMAND, ScheduledCancelChecker(ICancelChecker.NOOP)) { + commandRan.set(true) + } + } + command.start() + command.join(5_000) + diagnostics.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(commandRan.get()).isTrue() + } + + /** + * The cost ADR 0011 accepts in exchange for typing responsiveness: a command *is* preemptable, so + * every command call site retries (see [retryingOnPreemption]). + */ + @Test(timeout = 10_000) + fun `keystroke-driven work preempts an in-flight command`() { + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val completionRan = AtomicBoolean(false) + + val command = + Thread { + try { + withAnalysisLock(AnalysisPriority.COMMAND, holderChecker) { + holding.countDown() + repeat(2_000) { + holderChecker.abortIfCancelled() + Thread.sleep(5) + } + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + command.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + val completion = + Thread { + withAnalysisLock(AnalysisPriority.INTERACTIVE, ScheduledCancelChecker(ICancelChecker.NOOP)) { + completionRan.set(true) + } + } + completion.start() + completion.join(5_000) + command.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(completionRan.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `retryingOnPreemption runs a preempted attempt exactly once more with a fresh checker`() { + val attempts = AtomicInteger(0) + + val result = + retryingOnPreemption(ICancelChecker.NOOP, "test") { checker -> + // A latched checker would abort the retry immediately, so each attempt must get its own. + assertThat(checker.isCancelled()).isFalse() + if (attempts.incrementAndGet() == 1) { + checker.preempt() + checker.abortIfCancelled() + } + "done" + } + + assertThat(attempts.get()).isEqualTo(2) + assertThat(result).isEqualTo("done") + } + + @Test(timeout = 10_000) + fun `retryingOnPreemption propagates a second preemption rather than looping`() { + val attempts = AtomicInteger(0) + + val thrown = + runCatching { + retryingOnPreemption(ICancelChecker.NOOP, "test") { checker -> + attempts.incrementAndGet() + checker.preempt() + checker.abortIfCancelled() + } + }.exceptionOrNull() + + assertThat(attempts.get()).isEqualTo(2) + assertThat(thrown).isInstanceOf(AnalysisPreemptedException::class.java) + } + @Test(timeout = 10_000) fun `same priority diagnostics does not preempt an in-flight diagnostics`() { val holding = CountDownLatch(1) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/ImplementMembersEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/ImplementMembersEndToEndTest.kt index f3d017163f..fe0ebae8f0 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/ImplementMembersEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/ImplementMembersEndToEndTest.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.utils import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest import com.itsaky.androidide.lsp.models.TextEdit +import com.itsaky.androidide.progress.ICancelChecker import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Test @@ -15,7 +16,7 @@ class ImplementMembersEndToEndTest : KtLspTest() { ): List { createSourceFile("Main.kt", content) val mainPath = env.sourceRoots.first().resolve("Main.kt") - return ImplementMembersAction().computeImplementMembersEdit(env, mainPath, caret, noopCancelChecker()) + return ImplementMembersAction().computeImplementMembersEdit(env, mainPath, caret, ICancelChecker.NOOP) } /** Applies a single edit's newText over its [TextEdit.range] index span, returning the resulting text. */ diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/OrganizeImportsEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/OrganizeImportsEndToEndTest.kt index 535a4ae7b9..5fd9ea2ac2 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/OrganizeImportsEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/OrganizeImportsEndToEndTest.kt @@ -4,6 +4,7 @@ import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest import com.itsaky.androidide.models.Position import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Test @@ -31,7 +32,7 @@ class OrganizeImportsEndToEndTest : KtLspTest() { val mainPath = env.sourceRoots.first().resolve("Main.kt") // Drive the action's real plumbing: fetch-before-read ordering + full guard chain. - val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, noopCancelChecker()) + val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, ICancelChecker.NOOP) assertEquals(1, edits.size) assertEquals("import lib.Used", edits.single().newText) @@ -63,7 +64,7 @@ class OrganizeImportsEndToEndTest : KtLspTest() { """.trimIndent(), ) val mainPath = env.sourceRoots.first().resolve("Main.kt") - val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, noopCancelChecker()) + val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, ICancelChecker.NOOP) // Already organized -> no edit. A dropped import would produce a rewrite that removes it. assertTrue("constructor-only import must survive", edits.isEmpty()) } @@ -86,7 +87,7 @@ class OrganizeImportsEndToEndTest : KtLspTest() { """.trimIndent(), ) val mainPath = env.sourceRoots.first().resolve("Main.kt") - val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, noopCancelChecker()) + val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, ICancelChecker.NOOP) assertTrue("annotation-only import must survive", edits.isEmpty()) } @@ -109,7 +110,7 @@ class OrganizeImportsEndToEndTest : KtLspTest() { """.trimIndent(), ) val mainPath = env.sourceRoots.first().resolve("Main.kt") - val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, noopCancelChecker()) + val edits = OrganizeImportsAction().computeOrganizeEdit(env, mainPath, ICancelChecker.NOOP) assertTrue("typealias-only import used as constructor must survive", edits.isEmpty()) } } From 2f330f3ef209742c713e3bd43aae01b4c85e5ba1 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 13:38:42 +0000 Subject: [PATCH 03/11] ADFA-4824: Map a caret to the declaration whose usages to search for Find usages is invoked from either end: on a declaration's own name, or on any reference to it. Go-to-definition's referenceAtCaret cannot serve the first case, and not by accident - it is built so a caret on a declaration's own name resolves nothing, which is its no-self-jump rule. That is exactly the caret find usages starts from. targetAtCaret is declaration-first, falling back to referenceAtCaret. It returns a CaretTarget rather than a bare KtElement so the resolution step does not have to re-derive which case it is looking at. Two details worth naming: - The declaration check requires the caret's leaf to *be* the declaration's name identifier, not merely to sit inside a declaration. Every caret has an enclosing declaration - a call site's nearest one is the function containing it - so proximity alone would target that container for every reference in the file. - It checks both the leaf at the offset and the one before it. referenceAtCaret retries only when the primary leaf names nothing, which is not enough here: a caret just past `fun target` lands on '(', navigable in its own right for the invoke convention, so checking only that leaf made a caret one character past a declaration's name find nothing. Caught by the test for it. Declaration-first is observable on a destructuring entry, which is both a declaration and a convention reference: `x` in `val (x, y) = p` targets the local x here, while go-to-definition navigates from that same caret to component1. Deliberate, and asserted in both test classes. navigableLeafAt becomes internal so the accept-list is shared rather than duplicated. ReferenceAtCaret's behaviour is unchanged, and its tests are kept as the proof of that. --- .../lsp/kotlin/navigation/ReferenceAtCaret.kt | 7 +- .../lsp/kotlin/navigation/TargetAtCaret.kt | 86 +++++++++ .../kotlin/navigation/TargetAtCaretTest.kt | 167 ++++++++++++++++++ 3 files changed, 259 insertions(+), 1 deletion(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaret.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaretTest.kt diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.kt index b954a8d72f..755352688a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.kt @@ -75,7 +75,12 @@ internal fun referenceAtCaret( return null } -private fun navigableLeafAt( +/** + * The leaf token at [offset] if a caret there could name something, else null. Shared with + * [targetAtCaret], which applies the same accept-list before asking whether the leaf is a + * declaration's own name. + */ +internal fun navigableLeafAt( file: KtFile, offset: Int, ): PsiElement? { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaret.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaret.kt new file mode 100644 index 0000000000..3d7e2e2179 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaret.kt @@ -0,0 +1,86 @@ +package com.itsaky.androidide.lsp.kotlin.navigation + +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtElement +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtNamedDeclaration +import org.slf4j.LoggerFactory + +private val logger = LoggerFactory.getLogger("TargetAtCaret") + +/** + * What a caret names, for a feature that starts *from* a declaration rather than navigating to one. + * + * Find usages can be invoked from either end - on the declaration itself, or on any reference to it - + * and the two need different resolution, so the distinction is made once here rather than re-derived + * by a type test later. + */ +internal sealed interface CaretTarget { + /** The caret sits on [declaration]'s own name identifier. Its symbol is the search target. */ + data class Declaration( + val declaration: KtNamedDeclaration, + ) : CaretTarget + + /** The caret sits on a reference. Resolving [element] yields the search target. */ + data class Reference( + val element: KtElement, + ) : CaretTarget +} + +/** + * What the caret at [offset] in [file] names, or null when it names nothing. + * + * Declaration-first: a caret on a declaration's own name targets *that declaration*, and only a caret + * that names nothing declarable is interpreted as a reference. The order is observable for a + * destructuring entry, which is both at once - `x` in `val (x, y) = p` targets the local `x` here, + * while go-to-definition navigates from the same caret to `component1`. + * + * Callers must hold the project read lock. Pure PSI: no analysis session is needed or used. + */ +internal fun targetAtCaret( + file: KtFile, + offset: Int, +): CaretTarget? { + declarationAtCaret(file, offset)?.let { return CaretTarget.Declaration(it) } + + // Not a declaration's name, so fall back to go-to-definition's reference lookup, which repeats the + // leaf lookup above. One extra findElementAt is worth leaving that helper's contract untouched: + // it must keep returning null for a declaration's own name, which is the caret we just handled. + return referenceAtCaret(file, offset)?.let(CaretTarget::Reference)?.also { + logger.debug("Caret at {} in {} names a reference", offset, file.name) + } +} + +/** + * The declaration whose own name the caret at [offset] sits on, or null. + * + * Both candidate leaves are tried, not just the first navigable one. `referenceAtCaret` can stop at + * the first, because it retries only when the primary leaf names nothing at all; here the primary + * leaf can be navigable in its own right and still not be a name - a caret just past `fun target` + * lands on `(`, which is navigable for the invoke convention. Checking only that leaf would make a + * caret one character past a declaration's name find nothing. + */ +private fun declarationAtCaret( + file: KtFile, + offset: Int, +): KtNamedDeclaration? = + ( + declarationNamedBy(navigableLeafAt(file, offset)) + ?: declarationNamedBy(navigableLeafAt(file, (offset - 1).coerceAtLeast(0))) + )?.also { + logger.debug("Caret at {} in {} names declaration '{}'", offset, file.name, it.name) + } + +/** + * The declaration [leaf] is the name identifier of, or null. + * + * The identity check is what makes this precise: every caret has some enclosing declaration - a call + * site's nearest one is the function containing it - so proximity alone would target the container + * for every reference in the file. + */ +private fun declarationNamedBy(leaf: PsiElement?): KtNamedDeclaration? { + leaf ?: return null + val declaration = PsiTreeUtil.getParentOfType(leaf, KtNamedDeclaration::class.java) ?: return null + return declaration.takeIf { it.nameIdentifier === leaf } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaretTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaretTest.kt new file mode 100644 index 0000000000..94673eb9d5 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaretTest.kt @@ -0,0 +1,167 @@ +package com.itsaky.androidide.lsp.kotlin.navigation + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.KtDestructuringDeclarationEntry +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtOperationReferenceExpression +import org.jetbrains.kotlin.psi.KtParameter +import org.jetbrains.kotlin.psi.KtProperty +import org.junit.Test + +/** + * R2's caret rules for find usages. Pure PSI, no analysis session. + * + * The interesting cases are the ones where this must answer *differently* from + * [ReferenceAtCaretTest]: a caret on a declaration's own name is nothing to navigate to, but it is + * the normal place to search for usages from. + */ +class TargetAtCaretTest : KtLspTest() { + /** The target for a caret at `text.indexOf(marker) + delta` in a file containing [text]. */ + private fun targetAt( + name: String, + text: String, + marker: String, + delta: Int = 0, + ): CaretTarget? { + val file = createSourceFile(name, text) + val offset = + text.indexOf(marker).also { check(it >= 0) { "marker '$marker' not in source" } } + delta + return env.project.read { targetAtCaret(file, offset) } + } + + private fun assertDeclaration( + target: CaretTarget?, + name: String, + ): CaretTarget.Declaration { + assertThat(target).isInstanceOf(CaretTarget.Declaration::class.java) + val declaration = (target as CaretTarget.Declaration) + assertThat(declaration.declaration.name).isEqualTo(name) + return declaration + } + + @Test + fun `caret on a function's own name targets that function`() { + val target = targetAt("A.kt", "fun target() {}", "target", delta = 1) + assertDeclaration(target, "target") + assertThat((target as CaretTarget.Declaration).declaration).isInstanceOf(KtNamedFunction::class.java) + } + + @Test + fun `caret on a class's own name targets that class`() { + val target = targetAt("B.kt", "class Widget", "Widget", delta = 2) + assertDeclaration(target, "Widget") + assertThat((target as CaretTarget.Declaration).declaration).isInstanceOf(KtClass::class.java) + } + + @Test + fun `caret on a property's own name targets that property`() { + val target = targetAt("C.kt", "fun caller() {\n\tval count = 1\n}", "count", delta = 1) + assertDeclaration(target, "count") + assertThat((target as CaretTarget.Declaration).declaration).isInstanceOf(KtProperty::class.java) + } + + @Test + fun `caret on a parameter's own name targets that parameter`() { + val target = targetAt("D.kt", "fun caller(value: Int) = value", "value", delta = 1) + assertDeclaration(target, "value") + assertThat((target as CaretTarget.Declaration).declaration).isInstanceOf(KtParameter::class.java) + } + + /** + * The contrast that makes this file necessary: `referenceAtCaret` returns null here, because a + * declaration's own name is not something go-to-definition can navigate to. + */ + @Test + fun `a caret that go-to-definition rejects still yields a target`() { + val text = "fun target() {}" + val file = createSourceFile("E.kt", text) + val offset = text.indexOf("target") + 1 + + env.project.read { + assertThat(referenceAtCaret(file, offset)).isNull() + assertThat(targetAtCaret(file, offset)).isInstanceOf(CaretTarget.Declaration::class.java) + } + } + + @Test + fun `caret on a call targets the reference, not the enclosing declaration`() { + // The nearest enclosing KtNamedDeclaration is `caller`, so this only works because the + // declaration check requires the caret's leaf to *be* that declaration's name identifier. + val target = targetAt("F.kt", "fun target() {}\nfun caller() { target() }", "{ target()", delta = 3) + assertThat(target).isInstanceOf(CaretTarget.Reference::class.java) + } + + @Test + fun `caret one past a declaration's name targets that declaration`() { + // The character after `target` is '(', which is navigable in its own right (the invoke + // convention), so this asserts the declaration check runs on the primary leaf before any + // reference interpretation of it. + val target = targetAt("G.kt", "fun target() {}", "target", delta = 6) + assertDeclaration(target, "target") + } + + @Test + fun `caret on a local declaration inside a lambda targets that declaration`() { + // ReferenceAtCaretTest asserts this same caret navigates nowhere. Searching for usages of a + // local function is legitimate, so it must not inherit that null. + val target = + targetAt( + "H.kt", + "fun run(block: () -> Unit) {}\nfun caller() { run { fun inner() {} } }", + "inner", + delta = 1, + ) + assertDeclaration(target, "inner") + } + + /** + * Q15c / R2: a destructuring entry is simultaneously a declaration and a convention reference to + * `componentN`. Go-to-definition reads it as the reference; find usages reads it as the + * declaration, so a search from here finds usages of `x` rather than of `component1`. + */ + @Test + fun `caret on a destructuring entry targets the entry as a declaration`() { + val target = + targetAt( + "I.kt", + "data class P(val x: Int, val y: Int)\nfun caller(p: P) { val (x, y) = p }", + "(x, y)", + delta = 1, + ) + assertDeclaration(target, "x") + assertThat((target as CaretTarget.Declaration).declaration) + .isInstanceOf(KtDestructuringDeclarationEntry::class.java) + } + + @Test + fun `caret on an operator targets the operation reference`() { + val target = + targetAt( + "J.kt", + "class P { operator fun plus(other: P): P = this }\nfun caller(a: P, b: P) { a + b }", + "a + b", + delta = 2, + ) + assertThat(target).isInstanceOf(CaretTarget.Reference::class.java) + assertThat((target as CaretTarget.Reference).element) + .isInstanceOf(KtOperationReferenceExpression::class.java) + } + + @Test + fun `caret on whitespace yields no target`() { + assertThat(targetAt("K.kt", "fun caller() { }", " ", delta = 1)).isNull() + } + + @Test + fun `caret in a comment yields no target`() { + assertThat(targetAt("L.kt", "// target here\nfun target() {}", "target here", delta = 1)).isNull() + } + + @Test + fun `caret on a non-navigable keyword yields no target`() { + assertThat(targetAt("M.kt", "fun target() {}", "fun", delta = 1)).isNull() + } +} From cd32bf06459fd3cd64c297dd3ea86baffc76bad6 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 13:59:02 +0000 Subject: [PATCH 04/11] ADFA-4824: Find usages of a Kotlin declaration across the workspace Fills in KotlinLanguageServer.findReferences, which until now answered empty. There is no reference-search infrastructure to build on: the bundled analysis-api-standalone jar ships no ReferencesSearch, no PsiSearchHelper and no word index, and KtFileMetadata records declarations only. So the search is target -> match set -> scope -> candidate files -> resolve. Match set (R3). The target, plus its workspace-source supers, plus a classifier's constructors. Supers because a call dispatched through Base.foo may reach Derived.foo. Constructors because Foo() resolves to a constructor, not to the class, so without them a search on `class Foo` misses every instantiation. The up-walk stops at the workspace boundary: with Any.toString in the match set, a search on an overridden toString would report every .toString() call in the workspace. Both sides of every comparison are normalised through fakeOverrideOriginal, since a call through a subtype that does not redeclare the member resolves to a substituted fake override. Scope (R4) comes from the target's visibility, which is an exact bound rather than a heuristic. local/private stays in the file, internal in the module, anything more visible reaches the module and its transitive dependents. The ticket's three resolution scopes fall out of this rather than being three implementations, and a search on a local variable never leaves the open file. Candidates (R5) are narrowed by StringSearch.containsWord, which already reads an open file's live editor buffer rather than its saved bytes - so a usage typed but not yet saved is still found. That matters more here than for go-to-definition: find usages is run *while* editing. The name filter is also what implements "convention references are not results": `a + b` contains no plus token, so it is never a candidate. Identity (R6) uses KaSymbolPointer, restored once per candidate session, then compared with ==. KaSymbol cannot cross a session boundary, and KaSymbol equality within one session compares the underlying FIR symbol, so both sides must come from the same session. Neither PSI identity nor a (file, offset) key would work: the live and on-disk instances of the target's own file disagree about offsets as soon as there are unsaved edits, which would silently drop every cross-file usage in the common case. A pointer that will not restore drops that file rather than falling back to a looser comparison - under-report, never report something false. Scheduling (R9) is per candidate file: one analysis session and one project.read each, so a preemption costs one file and index refresh is never blocked for the length of a search. The live-PSI await stays outside project.read, since the refresh it waits on needs project.write. Tests cover the three resolution scopes, each row of the visibility ladder, the super-walk and its workspace cutoff, constructor expansion, imports, a Java-source target, a symbol-vs-name decoy, ordering, cancellation, and a usage that exists only in an unsaved buffer. Java files are not searched for usages, and neither are test source sets - AndroidModule.getSourceDirectories() returns mainSourceSet only, so test sources are not content roots for any LSP feature. Both documented in docs/features/kotlin-find-usages.md. --- .../lsp/kotlin/KotlinLanguageServer.kt | 7 +- .../lsp/kotlin/navigation/FindUsages.kt | 441 ++++++++++++++++++ .../lsp/kotlin/navigation/GoToDefinition.kt | 5 +- .../navigation/FindUsagesLiveDocumentTest.kt | 93 ++++ .../lsp/kotlin/navigation/FindUsagesTest.kt | 292 ++++++++++++ 5 files changed, 836 insertions(+), 2 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesLiveDocumentTest.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt index ae9f0d903c..958928e292 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt @@ -38,6 +38,7 @@ import com.itsaky.androidide.lsp.kotlin.compiler.index.KT_SOURCE_FILE_META_INDEX import com.itsaky.androidide.lsp.kotlin.completion.codeComplete import com.itsaky.androidide.lsp.kotlin.diagnostic.collectDiagnosticsFor import com.itsaky.androidide.lsp.kotlin.navigation.findDefinitionAt +import com.itsaky.androidide.lsp.kotlin.navigation.findUsagesAt import com.itsaky.androidide.lsp.kotlin.signaturehelp.doSignatureHelp import com.itsaky.androidide.lsp.models.CompletionParams import com.itsaky.androidide.lsp.models.CompletionResult @@ -233,7 +234,11 @@ class KotlinLanguageServer : ILanguageServer { return ReferenceResult.empty() } - return ReferenceResult.empty() + logger.debug("findReferences(position={}, file={})", params.position, params.file) + return compiler + ?.compilationEnvironmentFor(params.file) + ?.let { context(it) { findUsagesAt(params) } } + ?: ReferenceResult.empty() } override suspend fun findDefinition(params: DefinitionParams): DefinitionResult { 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 new file mode 100644 index 0000000000..5b7227570b --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt @@ -0,0 +1,441 @@ +package com.itsaky.androidide.lsp.kotlin.navigation + +import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence +import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation +import com.itsaky.androidide.lsp.kotlin.compiler.modules.isSourceModule +import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption +import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.compiler.services.ProjectStructureProvider +import com.itsaky.androidide.lsp.kotlin.utils.rangeOf +import com.itsaky.androidide.lsp.models.ReferenceParams +import com.itsaky.androidide.lsp.models.ReferenceResult +import com.itsaky.androidide.models.Location +import com.itsaky.androidide.models.Range +import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.projects.FileManager +import com.itsaky.androidide.projects.util.StringSearch +import kotlinx.coroutines.future.await +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.platform.projectStructure.KotlinModuleDependentsProvider +import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaClassSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaConstructorSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaDeclarationSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaSymbolLocation +import org.jetbrains.kotlin.analysis.api.symbols.KaSymbolVisibility +import org.jetbrains.kotlin.analysis.api.symbols.markers.KaNamedSymbol +import org.jetbrains.kotlin.analysis.api.symbols.pointers.KaSymbolPointer +import org.jetbrains.kotlin.analysis.api.symbols.sourcePsiSafe +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.idea.references.mainReference +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.slf4j.LoggerFactory +import java.nio.file.Path + +private val logger = LoggerFactory.getLogger("FindUsages") + +/** + * Where a usage could possibly be written, derived from the target's visibility (R4). + * + * Kotlin's visibility rules are an exact bound, not a heuristic: a `private` declaration cannot be + * referenced from another file, and a `public` one cannot be referenced from a module that does not + * depend on its own. Narrowing here is what keeps the common cases cheap - a search on a local + * variable never leaves the open file - and it is also what makes the ticket's three resolution + * scopes fall out of one code path. + */ +internal sealed interface UsageSearchScope { + data class SingleFile( + val path: Path, + ) : UsageSearchScope + + data class Modules( + val modules: List, + ) : UsageSearchScope +} + +/** + * Everything the per-file search loop needs, computed once in the caret's analysis session. + * + * [matchSet] holds pointers rather than symbols because a [KaSymbol] cannot cross a session boundary, + * and each candidate file may be analyzed in a different one (R6). + */ +internal class SearchPlan( + val simpleName: String, + val matchSet: List>, + val scope: UsageSearchScope, +) + +/** + * Computes the usage result for [params]. + * + * Structured so that no lock spans the whole search (R9): the target is resolved under one short + * `project.read`, candidate files are selected with no lock at all, 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 `project.write`) and would lose all its work to a single + * keystroke. + */ +context(env: AbstractCompilationEnvironment) +internal suspend fun findUsagesAt(params: ReferenceParams): ReferenceResult { + logger.debug("findUsagesAt requested for file={} position={}", params.file, params.position) + + if (params.cancelChecker.isCancelled()) { + logger.debug("References request for {} was cancelled before processing", params.file) + return ReferenceResult.empty() + } + + return try { + val plan = planAt(params) ?: return ReferenceResult.empty() + val candidates = candidateFiles(plan) + logger.debug("Usage search for '{}': {} candidate file(s)", plan.simpleName, candidates.size) + + val locations = + candidates + .flatMap { candidate -> + params.cancelChecker.abortIfCancelled() + usagesIn(candidate, plan, params.cancelChecker) + }.distinctBy { it.file to it.range } + .sortedWith(compareBy({ it.file.toString() }, { it.range.start.index })) + + logger.debug("Usage result for {}: {} location(s)", params.file, locations.size) + ReferenceResult(locations) + } catch (e: Throwable) { + if (e.isAnalysisCancellation()) { + logger.debug("Usage search for {} cancelled", params.file) + return ReferenceResult.empty() + } + logger.warn("Usage search failed for {}", params.file, e) + ReferenceResult.empty() + } +} + +/** + * The search plan for [params]' caret, or null when it names nothing searchable. + * + * Its own short-lived read lock and analysis session, released before any candidate file is touched. + */ +context(env: AbstractCompilationEnvironment) +internal suspend fun planAt(params: ReferenceParams): SearchPlan? { + val offset = params.position.requireIndex() + + return retryingOnPreemption(params.cancelChecker, "Usage search target for ${params.file}") { cancelChecker -> + // Awaited per attempt and outside project.read, exactly as in findDefinitionAt: the refresh this + // waits on needs project.write, and a preemption invalidates the KtFile it returned. + val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await() + if (ktFile == null) { + logger.warn("File {} cannot be loaded for usage search", params.file) + null + } else { + cancelChecker.abortIfCancelled() + env.project.read { + val target = targetAtCaret(ktFile, offset) ?: return@read null + analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { + planFor(target) + } + } + } + } +} + +/** The search plan for [target], or null when it names nothing searchable. */ +context(env: AbstractCompilationEnvironment) +private fun KaSession.planFor(target: CaretTarget): SearchPlan? { + val symbol = targetSymbol(target) ?: return null + val declaration = symbol.sourcePsiSafe() + if (declaration == null) { + // Not a workspace source: the stdlib, the framework, a library jar. Its usages are unreachable + // for the same reason go-to-definition cannot navigate to it. + logger.debug("Usage search target is not a workspace source; nothing to search") + return null + } + + val simpleName = prefilterName(symbol) ?: return null + val declarationPath = declaration.containingFile?.virtualFile?.let { runCatching { it.toNioPath() }.getOrNull() } + + return SearchPlan( + simpleName = simpleName, + matchSet = matchSet(symbol).map { it.createPointer() }, + scope = scopeOf(symbol, declaration, declarationPath), + ) +} + +/** + * The declaration [target] names. + * + * A [CaretTarget.Declaration] already *is* the declaration, so it answers through its own symbol; a + * [CaretTarget.Reference] answers through the same two resolution paths go-to-definition uses. + */ +private fun KaSession.targetSymbol(target: CaretTarget): KaDeclarationSymbol? = + when (target) { + is CaretTarget.Declaration -> { + runCatching { target.declaration.symbol }.getOrNull() + } + + is CaretTarget.Reference -> { + symbolsAt(target.element) + .also { + if (it.size > 1) { + // An ambiguous reference (overloads, broken code). Searching for the first candidate + // beats refusing to search; the alternative is a chooser UI the panel cannot host. + logger.debug("Reference at caret resolved to {} symbols; searching the first", it.size) + } + }.firstOrNull() as? KaDeclarationSymbol + } + }?.let { symbol -> + // A call through a subtype that does not redeclare the member resolves to a substituted fake + // override rather than to the declaration the user wrote. Normalise both sides of every + // comparison, starting here. + (symbol as? KaCallableSymbol)?.fakeOverrideOriginal ?: symbol + } + +/** + * The declarations a reference may resolve to and still count as a usage of [symbol] (R3). + * + * Two edges are added to the target itself: + * - **Workspace-source supers.** A call dispatched through `Base.foo` may reach `Derived.foo`, so it + * counts as a usage of it. The walk stops at the workspace boundary: `Any.toString` in the match set + * would make a usage search on an overridden `toString` report every `.toString()` call in the + * workspace, and a library super can never contribute a reportable result anyway. + * - **A classifier's constructors.** `Foo()` resolves to a constructor, not to the class, so without + * this a search on `class Foo` would miss every instantiation. Not applied in reverse: a target that + * *is* one constructor stays that constructor, because asking for usages of one overload is a + * deliberate act. + */ +private fun KaSession.matchSet(symbol: KaDeclarationSymbol): List = + buildList { + add(symbol) + + if (symbol is KaCallableSymbol) { + addAll( + symbol.allOverriddenSymbols + .map { it.fakeOverrideOriginal } + .filter { it.sourcePsiSafe() != null }, + ) + } + + if (symbol is KaClassSymbol) { + addAll(symbol.declaredMemberScope.constructors) + } + } + +/** + * The simple name to prefilter candidate files on, or null when there is none to search by. + * + * A constructor is written as its class's name, never as its own, so prefiltering on the symbol's own + * name would match nothing. + */ +private fun KaSession.prefilterName(symbol: KaDeclarationSymbol): String? { + val named = + if (symbol is KaConstructorSymbol) { + symbol.containingDeclaration as? KaNamedSymbol + } else { + symbol as? KaNamedSymbol + } + + return named?.name?.asString()?.takeUnless { it.isEmpty() } +} + +/** [symbol]'s search scope, per R4's visibility ladder. */ +context(env: AbstractCompilationEnvironment) +private fun KaSession.scopeOf( + symbol: KaDeclarationSymbol, + declaration: PsiElement, + declarationPath: Path?, +): UsageSearchScope { + val fileOnly = declarationPath?.let(UsageSearchScope::SingleFile) + + // A local is confined to its declaring block, and a private declaration to its file: Kotlin's + // private top-level is file-private, and a private member cannot escape the class body it is + // written in. Both are the cheap, exact cases. + if (fileOnly != null && + (symbol.location == KaSymbolLocation.LOCAL || symbol.visibility == KaSymbolVisibility.PRIVATE) + ) { + return fileOnly + } + + val module = moduleOf(declaration) ?: return fileOnly ?: UsageSearchScope.Modules(sourceModules()) + if (symbol.visibility == KaSymbolVisibility.INTERNAL) { + // internal is module-wide, and there is no associated test module to widen to: this project + // model builds one module per Gradle module from the main source set only. + return UsageSearchScope.Modules(listOf(module)) + } + + // Anything more visible can be referenced from any module that depends on this one. Dependents, + // not all modules: a module that cannot see the declaration cannot reference it. + val dependents = + KotlinModuleDependentsProvider + .getInstance(env.project) + .getTransitiveDependents(module) + .filterIsInstance() + + return UsageSearchScope.Modules( + buildList { + add(module) + addAll(dependents) + }, + ) +} + +context(env: AbstractCompilationEnvironment) +private fun moduleOf(declaration: PsiElement): KtModule? = + runCatching { + ProjectStructureProvider.getInstance(env.project).getModule(declaration, useSiteModule = null) as? KtModule + }.getOrNull() + +context(env: AbstractCompilationEnvironment) +private fun sourceModules(): List = + env.modules + .asFlatSequence() + .filter { it.isSourceModule } + .toList() + +/** + * The files worth parsing and resolving for [plan]. + * + * The prefilter is a one-directional over-approximation: a file that mentions the name but contains no + * usage is parsed and discarded, while a file that does not mention it cannot contain a named usage. + * [StringSearch.containsWord] reads an open file's live editor buffer rather than its saved bytes, so a + * usage typed but not yet saved is still found - which matters here, because find usages is run *while* + * editing. + */ +context(env: AbstractCompilationEnvironment) +internal fun candidateFiles(plan: SearchPlan): List = + when (val scope = plan.scope) { + // The declaration's own file always contains its name, so there is nothing to filter. + is UsageSearchScope.SingleFile -> { + listOf(scope.path) + } + + is UsageSearchScope.Modules -> { + scope.modules + .asSequence() + .filter { it.isSourceModule } + .flatMap { it.computeFiles(extended = true) } + .mapNotNull { runCatching { it.toNioPath() }.getOrNull() } + .distinct() + .filter { StringSearch.containsWord(it, plan.simpleName) } + .toList() + } + } + +/** + * Every usage of [plan]'s target in the file at [path]. + * + * One analysis session per file, so a preemption costs this file rather than the whole search, and the + * live-PSI await stays outside `project.read` (R9). + */ +context(env: AbstractCompilationEnvironment) +private suspend fun usagesIn( + path: Path, + plan: SearchPlan, + delegate: ICancelChecker, +): List = + try { + retryingOnPreemption(delegate, "Usage search in $path") { cancelChecker -> + val ktFile = ktFileFor(path) + if (ktFile == null) { + logger.debug("Skipping candidate {}: no PSI", path) + emptyList() + } else { + env.project.read { + analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { + matchingReferences(ktFile, plan, path, cancelChecker) + } + } + } + } + } catch (e: Throwable) { + if (e.isAnalysisCancellation()) throw e + // One unresolvable file must not lose the whole result. + logger.debug("Usage search skipped candidate {}", path, e) + emptyList() + } + +/** + * PSI for a candidate file: refreshed to the live editor buffer when the file is open, the indexed + * on-disk instance otherwise. + * + * The open case must be awaited here, outside `project.read`, because the refresh it waits on needs + * `project.write`. `getKtFile` cannot do it - it runs under `project.read` inside Analysis API + * services, so it only ever peeks the live cache. + */ +context(env: AbstractCompilationEnvironment) +private suspend fun ktFileFor(path: Path): KtFile? = + if (FileManager.isActive(path)) { + env.ktSymbolIndex.getCurrentKtFile(path).await() + } else { + env.ktSymbolIndex.getKtFile(path) + } + +/** + * The references in [ktFile] that resolve into [plan]'s match set. + * + * The name filter runs first and on PSI alone, so only references that could possibly match are ever + * resolved. It is also what implements "convention references are not discovered": `a + b` contains no + * `plus` token, so it is never a candidate. + * + * Match-set pointers are restored **once** for this session; [KaSymbol] equality within a single + * session compares the underlying FIR symbol, so it is the right comparison once both sides come from + * the same session (R6). + */ +private fun KaSession.matchingReferences( + ktFile: KtFile, + plan: SearchPlan, + path: Path, + cancelChecker: ICancelChecker, +): List { + val targets = plan.matchSet.mapNotNull { it.restoreSymbol() } + if (targets.isEmpty()) { + // Under-reporting beats reporting something false, so a pointer that will not restore drops this + // file rather than falling back to a looser comparison. + logger.debug("No match-set symbol restored in {}; skipping", path) + return emptyList() + } + + return PsiTreeUtil + .collectElementsOfType(ktFile, KtSimpleNameExpression::class.java) + .asSequence() + .filter { it.getReferencedName() == plan.simpleName } + .mapNotNull { reference -> + cancelChecker.abortIfCancelled() + if (resolvesInto(reference, targets)) locationOf(reference, ktFile, path) else null + }.toList() +} + +/** Whether [reference] resolves to one of [targets]. */ +private fun KaSession.resolvesInto( + reference: KtSimpleNameExpression, + targets: List, +): Boolean = + runCatching { + reference.mainReference + .resolveToSymbols() + .asSequence() + .map { (it as? KaCallableSymbol)?.fakeOverrideOriginal ?: it } + .any { resolved -> targets.any { it == resolved } } + }.getOrElse { + if (it.isAnalysisCancellation()) throw it + logger.debug("Could not resolve '{}'", reference.text, it) + false + } + +/** [reference]'s name range as an editor [Location], or null when the file has no document. */ +private fun locationOf( + reference: KtSimpleNameExpression, + ktFile: KtFile, + path: Path, +): Location? { + val range = rangeOf(reference.getReferencedNameElement(), ktFile) + if (range == Range.NONE) { + logger.debug("No document for {}; dropping usage", path) + return null + } + return Location(path, range) +} 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 4500b4506f..da360c5536 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 @@ -78,8 +78,11 @@ private fun KaSession.resolvedLocations( * * Resolution over broken code throws, and a throw must read as "not found" rather than crash the * request, so both paths are guarded. + * + * Shared with find usages, which resolves the reference under the caret the same way before searching + * for what it names. */ -private fun KaSession.symbolsAt(element: KtElement): List = +internal fun KaSession.symbolsAt(element: KtElement): List = runCatching { element.mainReference ?.resolveToSymbols() diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesLiveDocumentTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesLiveDocumentTest.kt new file mode 100644 index 0000000000..6e3c18539c --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesLiveDocumentTest.kt @@ -0,0 +1,93 @@ +package com.itsaky.androidide.lsp.kotlin.navigation + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.models.ReferenceParams +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.projects.FileManager +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Test +import java.nio.file.Path + +/** + * R5's live-buffer tier: a usage that exists only in an unsaved editor buffer must still be found. + * + * Separate from [FindUsagesTest] because it needs `enableParserEventSystem`, so that the `KtFile` built + * from the buffer is physical the way production's is (see `KtLspTestEnvironment`). + * + * This is the case find usages is most often run in - you search *while* editing - and the one a + * disk-only prefilter silently gets wrong: the file would never be selected as a candidate, so it would + * never be parsed and the usage would simply not appear. + */ +class FindUsagesLiveDocumentTest : KtLspTest() { + override val enableParserEventSystem = true + + private val openedPaths = mutableListOf() + + @After + fun closeDocs() { + openedPaths.forEach { FileManager.onDocumentClose(DocumentCloseEvent(it)) } + openedPaths.clear() + } + + private fun openDocument( + path: Path, + content: String, + ) { + FileManager.onDocumentOpen(DocumentOpenEvent(path, content, 1)) + openedPaths.add(path) + } + + @Test + fun `a usage typed into an unsaved buffer is found`() { + val declarationText = "fun target() {}" + val declaration = createSourceFile("Declaration.kt", declarationText) + val declarationPath = Path.of(declaration.virtualFile.path) + + // On disk this file contains no usage at all, so a prefilter reading saved bytes would skip it. + val usage = createSourceFile("Usage.kt", "fun caller() { }") + val usagePath = Path.of(usage.virtualFile.path) + val editedText = "fun caller() { target() }" + openDocument(usagePath, editedText) + + val params = + ReferenceParams( + declarationPath, + Position(0, 0, declarationText.indexOf("target")), + true, + ICancelChecker.NOOP, + ) + val locations = runBlocking { context(env) { findUsagesAt(params) } }.locations + + assertThat(locations).hasSize(1) + assertThat(locations[0].file).isEqualTo(usagePath) + assertThat(locations[0].range.start.index).isEqualTo(editedText.indexOf("target()")) + } + + @Test + fun `a usage deleted in an unsaved buffer is not reported`() { + val declarationText = "fun target() {}" + val declaration = createSourceFile("GoneDeclaration.kt", declarationText) + val declarationPath = Path.of(declaration.virtualFile.path) + + // The saved bytes still mention the name, so this file is still a candidate; it is resolution, + // not the prefilter, that must reject it. + val usage = createSourceFile("GoneUsage.kt", "fun caller() { target() }") + val usagePath = Path.of(usage.virtualFile.path) + openDocument(usagePath, "fun caller() { }") + + val params = + ReferenceParams( + declarationPath, + Position(0, 0, declarationText.indexOf("target")), + true, + ICancelChecker.NOOP, + ) + + assertThat(runBlocking { context(env) { findUsagesAt(params) } }.locations).isEmpty() + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt new file mode 100644 index 0000000000..37a11cce4d --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt @@ -0,0 +1,292 @@ +package com.itsaky.androidide.lsp.kotlin.navigation + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.kotlin.fixtures.TestSourceModuleSpec +import com.itsaky.androidide.lsp.models.ReferenceParams +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.progress.ICancelChecker +import kotlinx.coroutines.runBlocking +import org.junit.Test +import java.nio.file.Path + +/** + * The search itself: match set, visibility-derived scope, candidate selection and matching. + * + * Driven through `findUsagesAt`/`planAt` rather than the individual helpers, so each case exercises + * the real request path. + */ +class FindUsagesTest : KtLspTest() { + override val moduleSpecs = + listOf( + TestSourceModuleSpec("lib"), + TestSourceModuleSpec("app", dependsOn = listOf("lib")), + ) + + private class Source( + val path: Path, + val text: String, + ) + + private fun source( + module: String, + name: String, + text: String, + ): Source = Source(Path.of(createSourceFile(module, name, text).virtualFile.path), text) + + private fun paramsAt( + source: Source, + marker: String, + delta: Int = 0, + cancelChecker: ICancelChecker = ICancelChecker.NOOP, + ): ReferenceParams { + val offset = + source.text.indexOf(marker).also { check(it >= 0) { "marker '$marker' not in source" } } + delta + return ReferenceParams(source.path, Position(0, 0, offset), true, cancelChecker) + } + + /** Usages for a caret at `marker + delta` in [source], as `fileName:startOffset` pairs. */ + private fun usagesAt( + source: Source, + marker: String, + delta: Int = 0, + cancelChecker: ICancelChecker = ICancelChecker.NOOP, + ): List = + runBlocking { + context(env) { findUsagesAt(paramsAt(source, marker, delta, cancelChecker)) } + .locations + .map { "${it.file.fileName}:${it.range.start.index}" } + } + + private fun scopeAt( + source: Source, + marker: String, + delta: Int = 0, + ): UsageSearchScope? = + runBlocking { + context(env) { planAt(paramsAt(source, marker, delta))?.scope } + } + + private fun expected( + source: Source, + vararg markers: String, + ): List = + markers.map { marker -> + val index = source.text.indexOf(marker).also { check(it >= 0) { "marker '$marker' not in source" } } + "${source.path.fileName}:$index" + } + + @Test + fun `a same-file call is a usage`() { + val file = source("app", "SameFile.kt", "fun target() {}\nfun caller() { target() }") + + assertThat(usagesAt(file, "fun target", delta = 5)).isEqualTo(expected(file, "target() }")) + } + + @Test + fun `every call in the file is reported, ordered by offset`() { + val text = "fun target() {}\nfun a() { target() }\nfun b() { target() }" + val file = source("app", "Many.kt", text) + + val usages = usagesAt(file, "fun target", delta = 5) + + assertThat(usages).hasSize(2) + assertThat(usages).isEqualTo( + listOf( + "Many.kt:${text.indexOf("target() }")}", + "Many.kt:${text.lastIndexOf("target() }")}", + ), + ) + } + + @Test + fun `the declaration itself is never reported`() { + // includeDeclaration is ignored (R7): a target with no usages must come back empty so the editor + // flashes "no references" rather than silently selecting the declaration the caret is already on. + val file = source("app", "Unused.kt", "fun unused() {}") + + assertThat(usagesAt(file, "fun unused", delta = 5)).isEmpty() + } + + @Test + fun `an inter-file call in the same module is a usage`() { + val declaration = source("app", "Decl.kt", "fun shared() {}") + val usage = source("app", "Use.kt", "fun caller() { shared() }") + + assertThat(usagesAt(declaration, "fun shared", delta = 5)).isEqualTo(expected(usage, "shared()")) + } + + @Test + fun `an inter-module call is a usage`() { + val declaration = source("lib", "LibApi.kt", "fun libFun() {}") + val usage = source("app", "AppUse.kt", "fun caller() { libFun() }") + + assertThat(usagesAt(declaration, "fun libFun", delta = 5)).isEqualTo(expected(usage, "libFun()")) + } + + @Test + fun `searching from a reference finds the same usages as from the declaration`() { + val declaration = source("app", "FromRefDecl.kt", "fun shared() {}") + val usage = source("app", "FromRefUse.kt", "fun caller() { shared() }") + + val fromDeclaration = usagesAt(declaration, "fun shared", delta = 5) + val fromReference = usagesAt(usage, "shared()", delta = 1) + + assertThat(fromReference).isEqualTo(fromDeclaration) + assertThat(fromReference).isNotEmpty() + } + + @Test + fun `a constructor call is a usage of the class`() { + val declaration = source("app", "Widget.kt", "class Widget") + val usage = source("app", "WidgetUse.kt", "fun caller() { Widget() }") + + assertThat(usagesAt(declaration, "class Widget", delta = 7)).isEqualTo(expected(usage, "Widget()")) + } + + @Test + fun `an import is a usage`() { + val declaration = source("lib", "Imported.kt", "package lib\n\nclass Imported") + val usage = source("app", "ImportUse.kt", "package app\n\nimport lib.Imported\n\nfun caller(p: Imported) {}") + + assertThat(usagesAt(declaration, "class Imported", delta = 7)) + .isEqualTo(expected(usage, "Imported\n", "Imported) {}")) + } + + @Test + fun `a same-named declaration elsewhere is not a usage`() { + // Matching is by symbol, not by name: the decoy shares the name and nothing else. Separate + // packages are load-bearing - two top-level `fun ambiguous()` in one package is a redeclaration, + // and the decoy's call then legitimately binds to whichever the resolver picks first. + val declaration = source("app", "Real.kt", "package real\n\nfun ambiguous() {}") + source("app", "Decoy.kt", "package decoy\n\nfun ambiguous() {}\nfun decoyCaller() { ambiguous() }") + + assertThat(usagesAt(declaration, "fun ambiguous", delta = 5)).isEmpty() + } + + @Test + fun `a call dispatched through a workspace supertype is a usage of the override`() { + val declaration = + source( + "app", + "Hierarchy.kt", + """ + interface Base { + fun render() + } + + class Impl : Base { + override fun render() {} + } + """.trimIndent(), + ) + val usage = source("app", "HierarchyUse.kt", "fun caller(b: Base) { b.render() }") + + // The call statically resolves to Base.render, but may dispatch to Impl.render at runtime. + assertThat(usagesAt(declaration, "override fun render", delta = 14)) + .isEqualTo(expected(usage, "render() }")) + } + + @Test + fun `an override of a library member does not match unrelated calls to it`() { + // The up-walk stops at the workspace boundary: with Any.toString in the match set this would + // report every .toString() call in the workspace. + val declaration = + source( + "app", + "Renderer.kt", + "class Renderer {\n\toverride fun toString(): String = \"r\"\n}", + ) + source("app", "OtherToString.kt", "fun caller(value: Int) = value.toString()") + + assertThat(usagesAt(declaration, "override fun toString", delta = 14)).isEmpty() + } + + @Test + fun `a local declaration is scoped to its own file`() { + val file = source("app", "LocalScope.kt", "fun caller() {\n\tval count = 1\n\tprintln(count)\n}") + + assertThat(scopeAt(file, "val count", delta = 4)) + .isEqualTo(UsageSearchScope.SingleFile(file.path)) + assertThat(usagesAt(file, "val count", delta = 4)).isEqualTo(expected(file, "count)")) + } + + @Test + fun `a private top-level declaration is scoped to its own file`() { + val file = source("app", "PrivateScope.kt", "private fun hidden() {}\nfun caller() { hidden() }") + + assertThat(scopeAt(file, "fun hidden", delta = 5)) + .isEqualTo(UsageSearchScope.SingleFile(file.path)) + } + + @Test + fun `an internal declaration is scoped to its own module`() { + val file = source("lib", "InternalScope.kt", "internal fun shared() {}") + + val scope = scopeAt(file, "fun shared", delta = 5) + + assertThat(scope).isInstanceOf(UsageSearchScope.Modules::class.java) + assertThat((scope as UsageSearchScope.Modules).modules.map { it.id }).hasSize(1) + } + + @Test + fun `a public declaration is scoped to its module and dependents`() { + val file = source("lib", "PublicScope.kt", "fun exported() {}") + + val scope = scopeAt(file, "fun exported", delta = 5) + + assertThat(scope).isInstanceOf(UsageSearchScope.Modules::class.java) + // lib plus app, which depends on it. + assertThat((scope as UsageSearchScope.Modules).modules.map { it.id }).hasSize(2) + } + + /** + * Direction 1 of the cross-language split: a Java-source *target* is in scope, because resolving a + * Kotlin reference to it already works. Searching `.java` files for usages is not - and falls out of + * this design rather than needing a special case, since `getKtFile` rejects a non-Kotlin path, so a + * `.java` candidate file is simply skipped. + */ + @Test + fun `a workspace Java declaration is a valid target`() { + env.createFile("lib", "lib/JavaGreeter.java", "package lib;\npublic class JavaGreeter {}") + val usage = + source( + "app", + "app/JavaUse.kt", + "package app\n\nimport lib.JavaGreeter\n\nfun make(): JavaGreeter? = null", + ) + + // The caret is on the Kotlin reference; the target it resolves to is the Java class. + assertThat(usagesAt(usage, ": JavaGreeter", delta = 2)) + .isEqualTo(expected(usage, "JavaGreeter\n", "JavaGreeter? = null")) + } + + @Test + fun `a reference to a stdlib symbol yields no usages`() { + val file = source("app", "Stdlib.kt", "fun caller() { listOf(1) }") + + assertThat(usagesAt(file, "listOf", delta = 1)).isEmpty() + } + + @Test + fun `a caret that names nothing yields no usages`() { + val file = source("app", "Nothing.kt", "fun caller() { }") + + assertThat(usagesAt(file, "{ }", delta = 2)).isEmpty() + } + + @Test + fun `a cancelled request yields no usages rather than throwing`() { + val file = source("app", "Cancelled.kt", "fun target() {}\nfun caller() { target() }") + + assertThat(usagesAt(file, "fun target", delta = 5, cancelChecker = ICancelChecker.CANCELLED)).isEmpty() + } + + @Test + fun `a property read and write are both usages`() { + val text = "var counter = 0\nfun caller() {\n\tcounter = 1\n\tprintln(counter)\n}" + val file = source("app", "Property.kt", text) + + assertThat(usagesAt(file, "var counter", delta = 5)).hasSize(2) + } +} From 42cb1026eefab846bcc171a9fd8ac39d2de43962 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 13:59:23 +0000 Subject: [PATCH 05/11] ADFA-4824: Add Find references to the Kotlin code actions menu Mirrors Java's action and its menu position, immediately after Go to definition. The work itself is the editor's existing cancellable request, so the action only starts it. Carries its own tooltip tag rather than reusing Java's, following the split established for Kotlin go-to-definition and fix-imports, so the two languages can describe different behaviour. The tooltips database lives outside this repo, so the tag shows no text until a row exists for it - that row is a hand-off item, not code. Deliberately always visible for .kt/.kts and never conditioned on what the caret sits on: answering that needs PSI and the project read lock, and prepare() runs on the UI thread. A caret on whitespace therefore shows the item and flashes "no references". A .kts shows it and it does nothing, since a script has no CompilationEnvironment - both identical to go-to-definition. --- .../androidide/idetooltips/TooltipTag.kt | 1 + .../lsp/kotlin/KotlinCodeActionsMenu.kt | 2 + .../kotlin/actions/FindReferencesAction.kt | 50 +++++++++++++++++++ .../kotlin/KotlinCodeActionTooltipTagTest.kt | 2 + 4 files changed, 55 insertions(+) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/FindReferencesAction.kt diff --git a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt index c4865af7ab..c2638092c2 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -91,6 +91,7 @@ object TooltipTag { const val EDITOR_CODE_ACTIONS_KT_NULL_SAFETY_FIX = "editor.codeactions.kotlin.nullsafetyfix" const val EDITOR_CODE_ACTIONS_KT_SURROUND_TRY_CATCH = "editor.codeactions.kotlin.trycatch" const val EDITOR_CODE_ACTIONS_KT_GOTO_DEF = "editor.codeactions.kotlin.gotodef" + const val EDITOR_CODE_ACTIONS_KT_FIND_REFS = "editor.codeactions.kotlin.findrefs" const val EXIT_TO_MAIN = "exit.to.main" diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt index 7530a6fe9b..1188a15022 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt @@ -6,6 +6,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.IActionsMenuProvider import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction @@ -32,6 +33,7 @@ object KotlinCodeActionsMenu : IActionsMenuProvider { TooltipTag.EDITOR_CODE_ACTIONS_KT_UNCOMMENT, ), GoToDefinitionAction(), + FindReferencesAction(), AddImportAction(), OrganizeImportsAction(), SurroundWithTryCatchAction(), diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/FindReferencesAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/FindReferencesAction.kt new file mode 100644 index 0000000000..ab40a307da --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/FindReferencesAction.kt @@ -0,0 +1,50 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.hasRequiredData +import com.itsaky.androidide.actions.markInvisible +import com.itsaky.androidide.editor.api.ILspEditor +import com.itsaky.androidide.idetooltips.TooltipTag +import com.itsaky.androidide.resources.R +import io.github.rosemoe.sora.widget.CodeEditor + +/** + * Lists every usage of the declaration at the caret, or of whatever the reference at the caret names. + * + * Mirrors the Java action: the real work is the editor's own cancellable request, so this only has to + * start it. + */ +class FindReferencesAction : BaseKotlinCodeAction() { + override var titleTextRes: Int = R.string.action_find_references + override val id: String = ID + override var label: String = "" + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_KT_FIND_REFS + + // execAction only starts the editor's own background request, so it must not be moved off the UI + // thread. Nothing here or in prepare() touches the project lock, the index, or an analysis session - + // but super.prepare() -> BaseKotlinCodeAction.prepare -> isKotlinFile() does stat the file + // (Files.exists + Files.isDirectory) on the UI thread. Pre-existing, shared by every Kotlin/Java + // code action, and out of scope here. + override var requiresUIThread: Boolean = true + + override fun prepare(data: ActionData) { + super.prepare(data) + + // Deliberately not conditioned on what the caret sits on: answering that needs PSI and the + // project read lock, and prepare() runs on the UI thread. A caret that names nothing therefore + // shows the item and flashes "no references", exactly as go-to-definition does. + if (!visible || !data.hasRequiredData(CodeEditor::class.java)) { + markInvisible() + return + } + } + + override suspend fun execAction(data: ActionData): Any { + val editor = data[CodeEditor::class.java] ?: return false + return (editor as? ILspEditor)?.findReferences() ?: false + } + + companion object { + const val ID = "ide.editor.lsp.kt.findReferences" + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt index 117ce7ef9e..352e81eaec 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt @@ -5,6 +5,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.KotlinCodeActionsMenu.KT_LANG import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction @@ -34,6 +35,7 @@ class KotlinCodeActionTooltipTagTest { CommentLineAction.idFor(KT_LANG) to TooltipTag.EDITOR_CODE_ACTIONS_KT_COMMENT, UncommentLineAction.idFor(KT_LANG) to TooltipTag.EDITOR_CODE_ACTIONS_KT_UNCOMMENT, GoToDefinitionAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_GOTO_DEF, + FindReferencesAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_FIND_REFS, AddImportAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_IMPORT_CLASS, OrganizeImportsAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_ORGANIZE_IMPORTS, NullSafetyAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_NULL_SAFETY_FIX, From 2f0193f80fc9c8d8694053afeb326bb35495e356 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 14:08:12 +0000 Subject: [PATCH 06/11] ADFA-4824: Stop the search panel reading each result file once per hit showLocations read every result file in full, once per hit, on the main thread: a file with twelve usages was read and materialised twelve times, plus an exists() stat per hit. Java find-references has had this all along and simply rarely produces enough hits to hurt. Find usages does. A row needs only two short strings per hit - the hit's line and the matched text - so the fix is to group by file and read only the lines the hits touch: - One sequential BufferedReader pass per file, stopping after the last wanted line, retaining nothing. Reads drop from O(hits) to O(files) and peak memory is one line rather than one file. Deliberately not a per-file content cache, which would fix the repeated reads but hold every result file's text at once - the wrong trade on a phone. - The disk pass runs off the main thread via TaskExecutor, which posts the callback back to the UI thread. - A file with an open editor is still resolved on the UI thread. Its Content is live UI state that a background thread must not touch, and pulling a few lines out of it is substring work with no I/O. This is also what keeps unsaved edits reflected in the panel. The grouping and line extraction are extracted into SearchResultGrouping so they can be unit-tested; the activity call stays a thin shell. Two behaviour changes, both improvements: a hit whose line no longer exists is dropped rather than yielding whatever Content returned, and a file whose every hit is stale is omitted rather than contributing an empty group. The per-hit exists() check is gone because an unreadable file now yields no lines and therefore no rows. --- .../androidide/lsp/IDELanguageClientImpl.java | 81 +++++----- .../androidide/lsp/SearchResultGrouping.kt | 143 ++++++++++++++++++ .../lsp/SearchResultGroupingTest.kt | 130 ++++++++++++++++ 3 files changed, 316 insertions(+), 38 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/lsp/SearchResultGrouping.kt create mode 100644 app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java b/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java index 64bc5de9fa..1799fe673a 100755 --- a/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java +++ b/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java @@ -44,21 +44,19 @@ import com.itsaky.androidide.models.SearchResult; import com.itsaky.androidide.tasks.TaskExecutor; import com.itsaky.androidide.ui.CodeEditorView; -import com.itsaky.androidide.utils.FileIOUtils; import com.itsaky.androidide.utils.FileUtils; import com.itsaky.androidide.utils.FlashbarActivityUtilsKt; import com.itsaky.androidide.utils.FlashbarUtilsKt; import com.itsaky.androidide.utils.LSPUtils; import io.github.rosemoe.sora.lang.diagnostic.DiagnosticsContainer; -import io.github.rosemoe.sora.text.Content; import java.io.File; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.TreeSet; import java.util.stream.Collectors; @@ -281,46 +279,53 @@ public void showLocations(List locations) { return; } - final Map> results = new HashMap<>(); - for (int i = 0; i < locations.size(); i++) { - try { - final Location loc = locations.get(i); - if (loc == null) { - continue; - } + // Group by file first. Reads then cost one pass per file instead of one full read per hit, which + // is what this used to do - and it did it on this thread. See SearchResultGrouping. + final Map> byFile = new LinkedHashMap<>(); + for (final Location loc : locations) { + if (loc == null) { + continue; + } + byFile.computeIfAbsent(loc.getFile().toFile(), f -> new ArrayList<>()).add(loc); + } - final File file = loc.getFile().toFile(); - if (!file.exists() || !file.isFile()) { - continue; + // A file with an open editor is resolved here, on the UI thread: its Content is live UI state + // that a background thread must not touch, and pulling a few lines out of it is substring work + // with no I/O. Everything else is read off this thread below. + final Map> fromEditors = new HashMap<>(); + final Map> onDisk = new LinkedHashMap<>(); + for (final Map.Entry> entry : byFile.entrySet()) { + final var frag = findEditorByFile(entry.getKey()); + if (frag != null && frag.getEditor() != null) { + final List rows = SearchResultGrouping.INSTANCE.resultsFor( + entry.getKey(), entry.getValue(), frag.getEditor().getText()); + if (!rows.isEmpty()) { + fromEditors.put(entry.getKey(), rows); } - var frag = findEditorByFile(file); - Content content; - if (frag != null && frag.getEditor() != null) { - content = frag.getEditor().getText(); - } else { - content = new Content(FileIOUtils.readFile2String(file)); - } - final List matches = results.containsKey(file) ? results.get(file) : new ArrayList<>(); - Objects.requireNonNull(matches) - .add( - new SearchResult( - loc.getRange(), - file, - content.getLineString(loc.getRange().getStart().getLine()), - content - .subContent( - loc.getRange().getStart().getLine(), - loc.getRange().getStart().getColumn(), - loc.getRange().getEnd().getLine(), - loc.getRange().getEnd().getColumn()) - .toString())); - results.put(file, matches); - } catch (Throwable th) { - LOG.error("Failed to show file location", th); + } else { + onDisk.put(entry.getKey(), entry.getValue()); } } - activity.handleSearchResults(results); + if (onDisk.isEmpty()) { + activity.handleSearchResults(fromEditors); + return; + } + + TaskExecutor.executeAsyncProvideError( + () -> SearchResultGrouping.INSTANCE.readFromDisk(onDisk), + (result, throwable) -> { + if (!canUseActivity()) { + return; + } + final Map> merged = new HashMap<>(fromEditors); + if (result != null) { + merged.putAll(result); + } else { + LOG.error("Failed to read search result files", throwable); + } + activity.handleSearchResults(merged); + }); } private Boolean applyActionEdits(@Nullable final IDEEditor editor, final CodeActionItem action) { diff --git a/app/src/main/java/com/itsaky/androidide/lsp/SearchResultGrouping.kt b/app/src/main/java/com/itsaky/androidide/lsp/SearchResultGrouping.kt new file mode 100644 index 0000000000..b8613ae925 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/lsp/SearchResultGrouping.kt @@ -0,0 +1,143 @@ +package com.itsaky.androidide.lsp + +import com.itsaky.androidide.models.Location +import com.itsaky.androidide.models.SearchResult +import io.github.rosemoe.sora.text.Content +import org.slf4j.LoggerFactory +import java.io.BufferedReader +import java.io.File + +/** + * Builds the search-results panel's rows for a set of [Location]s. + * + * Exists because the panel used to read every result file **in full, once per hit, on the main + * thread**: a file with twelve usages was read and materialised twelve times. Find usages made that a + * real cost rather than a latent one. + * + * A row needs only two short strings per hit - the hit's line, and the matched text - so nothing here + * retains a file's contents. Reads are one sequential pass per file, and peak memory is one line rather + * than one file. A per-file content cache would fix the repeated reads but hold every result file's text + * at once, which is the wrong trade on a phone. + */ +internal object SearchResultGrouping { + private val logger = LoggerFactory.getLogger(SearchResultGrouping::class.java) + + /** + * Rows for [locations] in [file], built from already-available [lines] (0-based line number to text). + * + * A location whose lines are not all present is dropped: a stale location can point past the end of + * a file that has since been edited, and a row referring to a line that no longer exists is worse + * than no row. + */ + fun resultsFor( + file: File, + locations: List, + lines: Map, + ): List = + locations.mapNotNull { location -> + val range = location.range + val startLine = lines[range.start.line] ?: return@mapNotNull null + val match = matchedText(range.start.line, range.start.column, range.end.line, range.end.column, lines) + if (match == null) { + logger.debug("Dropping stale search result in {}", file.name) + return@mapNotNull null + } + SearchResult(range, file, startLine, match) + } + + /** Rows for [locations] in [file], read from the live editor buffer [content]. */ + fun resultsFor( + file: File, + locations: List, + content: Content, + ): List { + val lines = + linesNeededBy(locations) + .filter { it >= 0 && it < content.lineCount } + .associateWith { content.getLineString(it) } + + return resultsFor(file, locations, lines) + } + + /** Rows for every file in [byFile], reading each file exactly once. */ + fun readFromDisk(byFile: Map>): Map> = + byFile + .mapValues { (file, locations) -> resultsFor(file, locations, readLines(file, linesNeededBy(locations))) } + .filterValues { it.isNotEmpty() } + + /** Every 0-based line number whose text [locations] need. */ + fun linesNeededBy(locations: List): Set = + locations + .flatMapTo(mutableSetOf()) { location -> + location.range.start.line..location.range.end.line + } + + /** + * The text of just the [wanted] lines of [file], in one sequential pass. + * + * Stops as soon as the last wanted line has been seen, and never holds more than the current line, + * so a hit near the top of a large file does not read the rest of it. Missing lines - a file shorter + * than the location claims, or an unreadable file - are simply absent from the result. + */ + fun readLines( + file: File, + wanted: Set, + ): Map { + if (wanted.isEmpty()) { + return emptyMap() + } + + val last = wanted.max() + val lines = HashMap(wanted.size) + return try { + file.bufferedReader().use { reader -> + reader.collectLines(wanted, last, lines) + } + lines + } catch (e: Exception) { + // A result file that has been deleted or is unreadable drops its rows, which is what the + // previous implementation did too by way of an exists() check per hit. + logger.debug("Could not read search result file {}", file, e) + lines + } + } + + private fun BufferedReader.collectLines( + wanted: Set, + last: Int, + into: MutableMap, + ) { + var number = 0 + while (number <= last) { + val line = readLine() ?: return + if (number in wanted) { + into[number] = line + } + number++ + } + } + + /** The text covered by the range, or null when any line it spans is missing. */ + private fun matchedText( + startLine: Int, + startColumn: Int, + endLine: Int, + endColumn: Int, + lines: Map, + ): String? { + val first = lines[startLine] ?: return null + if (startLine == endLine) { + val from = startColumn.coerceIn(0, first.length) + return first.substring(from, endColumn.coerceIn(from, first.length)) + } + + return buildString { + append(first.substring(startColumn.coerceIn(0, first.length))) + for (line in (startLine + 1) until endLine) { + append('\n').append(lines[line] ?: return null) + } + val lastLine = lines[endLine] ?: return null + append('\n').append(lastLine.substring(0, endColumn.coerceIn(0, lastLine.length))) + } + } +} diff --git a/app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt b/app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt new file mode 100644 index 0000000000..6c01a6f4e2 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt @@ -0,0 +1,130 @@ +package com.itsaky.androidide.lsp + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.models.Location +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.models.Range +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import java.io.File + +/** + * The panel used to read each result file in full, once per hit, on the main thread. These pin the + * replacement: one pass per file, only the lines a hit needs, and stale hits dropped rather than + * throwing. + */ +class SearchResultGroupingTest { + @get:Rule + val folder = TemporaryFolder() + + private fun location( + file: File, + startLine: Int, + startColumn: Int, + endLine: Int = startLine, + endColumn: Int = startColumn, + ) = Location( + file.toPath(), + Range(Position(startLine, startColumn, 0), Position(endLine, endColumn, 0)), + ) + + @Test + fun `a single-line hit carries its line and the matched text`() { + val file = File("Example.kt") + val lines = mapOf(1 to "fun caller() { target() }") + + val results = SearchResultGrouping.resultsFor(file, listOf(location(file, 1, 15, 1, 21)), lines) + + assertThat(results).hasSize(1) + assertThat(results[0].line).isEqualTo("fun caller() { target() }") + assertThat(results[0].match).isEqualTo("target") + assertThat(results[0].file).isEqualTo(file) + } + + @Test + fun `a multi-line hit joins the lines it spans`() { + val file = File("Multi.kt") + val lines = mapOf(0 to "first line", 1 to "middle", 2 to "last line") + + val results = SearchResultGrouping.resultsFor(file, listOf(location(file, 0, 6, 2, 4)), lines) + + assertThat(results).hasSize(1) + assertThat(results[0].match).isEqualTo("line\nmiddle\nlast") + // The row's line text is the line the hit starts on. + assertThat(results[0].line).isEqualTo("first line") + } + + @Test + fun `a hit on a line that no longer exists is dropped`() { + val file = File("Stale.kt") + + val results = SearchResultGrouping.resultsFor(file, listOf(location(file, 9, 0, 9, 3)), mapOf(0 to "only line")) + + assertThat(results).isEmpty() + } + + @Test + fun `a column past the end of its line is clamped rather than throwing`() { + val file = File("Clamped.kt") + + val results = SearchResultGrouping.resultsFor(file, listOf(location(file, 0, 2, 0, 99)), mapOf(0 to "short")) + + assertThat(results).hasSize(1) + assertThat(results[0].match).isEqualTo("ort") + } + + @Test + fun `only the lines a hit needs are collected`() { + val file = folder.newFile("Wanted.kt") + file.writeText("zero\none\ntwo\nthree\nfour\n") + + assertThat(SearchResultGrouping.readLines(file, setOf(1, 3))) + .isEqualTo(mapOf(1 to "one", 3 to "three")) + } + + @Test + fun `lines past the end of the file are absent rather than failing`() { + val file = folder.newFile("Short.kt") + file.writeText("only\n") + + assertThat(SearchResultGrouping.readLines(file, setOf(0, 7))).isEqualTo(mapOf(0 to "only")) + } + + @Test + fun `an unreadable file yields no lines rather than throwing`() { + val missing = File(folder.root, "Absent.kt") + + assertThat(SearchResultGrouping.readLines(missing, setOf(0))).isEmpty() + } + + @Test + fun `every hit in a file is built from one read`() { + val file = folder.newFile("Several.kt") + file.writeText("fun a() { target() }\nfun b() { target() }\n") + + val results = + SearchResultGrouping.readFromDisk( + mapOf(file to listOf(location(file, 0, 10, 0, 16), location(file, 1, 10, 1, 16))), + ) + + assertThat(results.keys).containsExactly(file) + assertThat(results.getValue(file).map { it.match }).containsExactly("target", "target") + } + + @Test + fun `a file whose every hit is stale is omitted entirely`() { + val file = folder.newFile("AllStale.kt") + file.writeText("one line\n") + + assertThat(SearchResultGrouping.readFromDisk(mapOf(file to listOf(location(file, 40, 0, 40, 2))))).isEmpty() + } + + @Test + fun `linesNeededBy covers every line a hit spans`() { + val file = File("Spans.kt") + + assertThat(SearchResultGrouping.linesNeededBy(listOf(location(file, 2, 0, 4, 1), location(file, 9, 0)))) + .containsExactly(2, 3, 4, 9) + } +} From 5df6c84f4c1cad6e63f363bdc36445128a4b804f Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 3 Aug 2026 14:10:35 +0000 Subject: [PATCH 07/11] ADFA-4824: Sync the find-usages doc with what was built Corrections found while implementing: - The prefilter needs no live-buffer branch of its own. StringSearch.containsWord already reads FileManager.getActiveDocument when the file is open. Records its two pre-existing limits too: it reads only the first 1 MB of a file, and it scans through one shared unsynchronised static buffer, so a concurrent Java find-references can corrupt the scan. - showLocations does not build the whole map off the main thread. A file with an open editor is resolved on the UI thread deliberately, because its Content is live UI state a background thread must not touch. - Only KtSimpleNameExpressions are examined, so a KDoc [link] to the target is not reported. Added to R5 and to the non-goals rather than left implicit. - An ambiguous reference at the caret searches its first resolved candidate. - targetAtCaret checks both candidate leaves, not one. - planAt/SearchPlan/candidateFiles are internal so the visibility ladder can be asserted; it is not observable from a result set, because symbol matching means a same-named decoy can never be a false positive whatever the scope. Also updates the verification section to the tests that now exist and their counts. --- docs/features/kotlin-find-usages.md | 65 ++++++++++++++++++----------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/docs/features/kotlin-find-usages.md b/docs/features/kotlin-find-usages.md index 4db34241c8..1839605d64 100644 --- a/docs/features/kotlin-find-usages.md +++ b/docs/features/kotlin-find-usages.md @@ -1,7 +1,7 @@ # Kotlin find usages (K2 LSP) - **Ticket:** ADFA-4824 (subtask of ADFA-3317; split out of the closed ADFA-3321 "Navigation") -- **Status:** Design agreed, implementation in progress +- **Status:** Implemented in `lsp/kotlin/navigation/`, pending on-device QA - **Module:** `lsp/kotlin` From a Kotlin declaration - or from a reference to one - list every place in the workspace that uses it, across three scopes: same file, another file in the same module, another module in the workspace. @@ -99,13 +99,17 @@ The ticket's three resolution scopes fall out of this one code path rather than | File | Prefilter text | PSI | |---|---|---| -| open in the editor | `FileManager.getDocumentContents(path)` - the live buffer | `ktSymbolIndex.getCurrentKtFile(path).await()`, awaited **outside** `project.read` | -| everything else | disk, via `StringSearch.containsWord` | `ktSymbolIndex.getKtFile(vf)` | +| open in the editor | the live buffer | `ktSymbolIndex.getCurrentKtFile(path).await()`, awaited **outside** `project.read` | +| everything else | disk | `ktSymbolIndex.getKtFile(path)` | -The prefilter is word-boundary exact on the target's simple name. Its errors are one-directional: a file that mentions the name but contains no usage is parsed and discarded (wasted work, correct result), while a file that does not mention the name cannot contain a named usage. +The prefilter is `StringSearch.containsWord`, word-boundary exact on the target's simple name. It needs no live-buffer branch of its own: it already reads `FileManager.getActiveDocument(file)` when the file is open, and only falls back to disk otherwise. Its errors are one-directional: a file that mentions the name but contains no usage is parsed and discarded (wasted work, correct result), while a file that does not mention the name cannot contain a named usage. Open documents are tab-count many, so the live tier is free. Without it, a usage the user just typed would be missed entirely - the prefilter would never select the file, so it would never be parsed. +Two pre-existing `StringSearch` limits carry over. It reads only the **first 1 MB** of a file, so a usage past that point is missed; and it scans through one shared static `ByteBuffer` with no synchronisation, so a Kotlin usage search running concurrently with a Java find-references can corrupt the other's scan. Neither is introduced here and neither is fixed here, but the second is worth a ticket. + +Only `KtSimpleNameExpression`s are examined. That is what makes the name filter cheap - it runs on PSI alone, before any resolution - and it is also what implements "convention references are not results": `a + b` contains no `plus` token, so it is never a candidate. The cost is that a **KDoc `[link]`** to the target is not reported, even though go-to-definition navigates from one; a documented gap rather than a decision worth its own machinery in v1. + **R6 - Identity.** A reference is a usage if its resolved symbol is in the match set. Deciding that across files needs care, because `KaSymbol` is session-scoped and the same declaration exists as two PSI instances - the on-disk `KtFile` cached in the index, and the dangling `KtFile` built from the editor buffer for an open file. Matching therefore uses `KaSymbolPointer`: `createPointer()` for each match-set member in the caret's session, then `restoreSymbol(session)` **once per candidate session**, then `==` against each resolved candidate symbol inside that session. This is the platform's cross-session identity mechanism, with structural implementations per symbol kind, and it is the direct analogue of the Java server re-deriving its target `Element` inside each compile task. @@ -139,11 +143,15 @@ Granularity is per candidate file, and it is load-bearing: The prefilter pass runs first, before any analysis, holding no locks. No progress count is shown - `launchCancellableAsyncWithProgress` takes a fixed `@StringRes`, and threading a live count through it would change a shared editor API for a cosmetic gain. No timeout and no file budget: the search finishes or the user cancels. -**R10 - Panel cost.** `IDELanguageClientImpl.showLocations` currently reads each result file **in full, once per hit, on the main thread** (`FileIOUtils.readFile2String` inside the per-location loop). That is a main-thread I/O violation and O(hits) file reads; Java's find-references has it today and simply rarely produces enough hits to hurt. +**R10 - Panel cost.** `IDELanguageClientImpl.showLocations` used to read each result file **in full, once per hit, on the main thread** (`FileIOUtils.readFile2String` inside the per-location loop, plus an `exists()` stat per hit). That is a main-thread I/O violation and O(hits) file reads; Java's find-references had it too and simply rarely produced enough hits to hurt. + +Rewritten to: group locations by file, then one sequential `BufferedReader` pass per file pulling only the lines its ranges touch, retaining nothing before moving on. The disk pass runs **off** the main thread through `TaskExecutor`, which posts its callback back to the UI thread. + +A file with an **open editor** is still resolved **on** the UI thread. Its `Content` is live UI state that a background thread must not touch, and pulling a few lines out of it is substring work with no I/O. That is also what keeps unsaved edits reflected in the panel. -Rewritten to: group locations by file, then one sequential `BufferedReader` pass per file pulling only the lines its ranges touch, building the `SearchResult`s and retaining nothing before moving on. A file with an open editor uses that editor's live `Content` - no read, no extra memory, and correct for unsaved edits. The whole map is built off the main thread; only `handleSearchResults` touches the UI. +Reads drop from O(hits) to O(files), peak memory is one line rather than one file (deliberately *not* a per-file content cache - holding every result file's text at once is the wrong trade on a phone), and the main thread does no I/O. This removes the need for a result cap, which would otherwise silently truncate. -Reads drop from O(hits) to O(files), peak memory is one line rather than one file (deliberately *not* a per-file content cache - holding every result file's text at once is the wrong trade on a phone), and the main thread does no I/O. This removes the need for a result cap, which would otherwise silently truncate. One behaviour change: a stale location whose line no longer exists is dropped rather than yielding whatever `Content` returned. +Two behaviour changes, both improvements: a hit whose line no longer exists is dropped rather than yielding whatever `Content` returned, and a file whose every hit is stale is omitted rather than contributing an empty group. The grouping and line extraction live in `SearchResultGrouping` so they can be unit-tested; the activity call is a thin shell. **R11 - Not ready.** No `CompilationEnvironment` for the file (a script, a file outside the content roots), or no analysis session yet, answers empty and logs. There is no "still indexing" signal; that gap is cross-cutting across every LSP feature and is not solved here. @@ -156,6 +164,7 @@ Reads drop from O(hits) to O(files), peak memory is one line rather than one fil - **Searching `.java` files** for usages of a Kotlin declaration. Filed separately. - **Usages in test source sets.** Filed separately, as an LSP-wide content-root gap. - **Implicit call sites as results** (see Scope). +- **KDoc `[link]`s as results.** Only `KtSimpleNameExpression`s are examined (R5). Go-to-definition navigates *from* a KDoc link, so this is an asymmetry, but a bounded one. - **Library-source usages**, via decompilation or `-sources.jar`. - **Categorising results** (imports vs calls vs type references) - the panel has no grouping beyond file. - **A partiality signal.** `ReferenceResult` is shared with the Java and XML servers and has no field for it, and `showLocations` has no header slot; the same caveat already applies silently to test sources. @@ -196,16 +205,16 @@ FindReferencesAction.execAction lsp/kotlin/actions guards: settings.referencesEnabled(), DocumentUtils.isKotlinFile compilationEnvironmentFor(params.file) ?: empty [R11] -> context(env) { findUsagesAt(params) } navigation/FindUsages.kt - ktFile = env.ktSymbolIndex.getCurrentKtFile(file).await() ?: empty [R5, R11] - env.project.read { - target = targetAtCaret(ktFile, offset) navigation/TargetAtCaret.kt [R2] - analyzeMaybeDangling(ktFile, COMMAND, cancelChecker) { - matchSet(target) -> List [R3, R6] + planAt(params): (retried once if preempted) + ktFile = env.ktSymbolIndex.getCurrentKtFile(file).await() ?: empty [R5, R11] + env.project.read { + target = targetAtCaret(ktFile, offset) navigation/TargetAtCaret.kt [R2] + analyzeMaybeDangling(ktFile, COMMAND, cancelChecker) { + planFor(target) -> simpleName, matchSet pointers, scope [R3, R4, R6] + } } - } - scopeOf(target) -> modules [R4] - prefilter(modules, target.name) -> candidate files [R5] - per candidate file: [R9] + candidateFiles(plan) [R5] + per candidate file: (retried once if preempted) [R9] await live PSI if open (outside project.read) env.project.read { analyzeMaybeDangling(file, COMMAND, cancelChecker) { @@ -217,29 +226,35 @@ FindReferencesAction.execAction lsp/kotlin/actions New components: -- **`navigation/TargetAtCaret.kt`** - `targetAtCaret(file: KtFile, offset: Int): KtElement?`. Pure PSI, no analysis session, so R2's caret rules are testable without one. Shares `ReferenceAtCaret.kt`'s token accept-list and `offset - 1` retry, which become `internal` rather than private. -- **`navigation/FindUsages.kt`** - the match set, the visibility-derived scope, the prefilter, the per-file resolve loop, and symbol-to-`Location` conversion, reusing go-to-definition's range helper. +- **`navigation/TargetAtCaret.kt`** - `targetAtCaret(file: KtFile, offset: Int): CaretTarget?`, returning either a `Declaration` or a `Reference` so the resolution step does not re-derive which case it is looking at. Pure PSI, no analysis session, so R2's caret rules are testable without one. Shares `ReferenceAtCaret.kt`'s token accept-list, which becomes `internal`. It checks the leaf at the offset **and** the one before it, because `referenceAtCaret`'s single retry is not enough here: a caret just past `fun target` lands on `(`, which is navigable in its own right, so checking only that leaf made a caret one character past a declaration's name find nothing. +- **`navigation/FindUsages.kt`** - `planAt` (target, match set, scope) and the per-file resolve loop, reusing go-to-definition's `symbolsAt` and range helper. `planAt`, `SearchPlan` and `candidateFiles` are `internal` rather than private so the visibility ladder is directly assertable: it is *not* observable from a result set, since symbol matching means a same-named decoy can never be a false positive whatever the scope. +- **`SearchResultGrouping`** (in `app/`) - R10's grouping and line extraction. + +An **ambiguous** reference at the caret (overloads, broken code) searches for its first resolved candidate and logs. The alternative is a chooser the panel cannot host, and refusing to search would be worse. Touched existing components: - **`KotlinLanguageServer.findReferences`** - the stub's guards stay; it now delegates inside the file's `CompilationEnvironment`, matching how `findDefinition` and `signatureHelp` dispatch. - **`navigation/ReferenceAtCaret.kt`** - visibility loosened for reuse. Behaviour unchanged, and its existing tests are kept as the proof of that. -- **`AnalysisPriority` / `AnalysisScheduler`** - the new `COMMAND` tier ([ADR 0011](../adr/0011-command-analysis-priority.md)). -- **`GoToDefinitionAction`, `OrganizeImportsAction`, `ImplementMembersAction`** - migrated to `COMMAND`; the latter two gain the retry they never had. -- **`IDELanguageClientImpl.showLocations`** - R10's grouped streaming rewrite. The grouping and line extraction are extracted into a pure helper so they can be unit-tested; the activity call stays a thin shell. +- **`AnalysisPriority` / `AnalysisScheduler`** - the new `COMMAND` tier, plus `retryingOnPreemption`, which holds the two invariants every command's retry depends on: a fresh `ScheduledCancelChecker` per attempt (`preempt()` latches), and re-fetching the `KtFile` inside the attempt ([ADR 0011](../adr/0011-command-analysis-priority.md)). +- **`GoToDefinitionAction`, `OrganizeImportsAction`, `ImplementMembersAction`** - migrated to `COMMAND`; the latter two gain the retry they never had, and take the delegate `ICancelChecker` rather than a pre-wrapped one since wrapping is now per attempt. +- **`GoToDefinition.symbolsAt`** - `internal`, so the reference-at-caret resolution is shared rather than duplicated. +- **`IDELanguageClientImpl.showLocations`** - R10's grouped streaming rewrite. - **`TooltipTag`** - one new constant (R1). Unchanged: `ReferenceParams`/`ReferenceResult`, `ILanguageServer`, `IDEEditor`, and every string resource. ## Verification -Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest`), split to match the helpers: +`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest` and `:app:testV7DebugUnitTest`, split to match the helpers: -- **`TargetAtCaretTest`** - PSI only, no session. Caret on a declaration's own name; caret on a reference; whitespace / comment / non-navigable keyword; one past an identifier; a destructuring entry targeting the local rather than `componentN`. +- **`TargetAtCaretTest`** (13) - PSI only, no session. Caret on a function's / class's / property's / parameter's own name; caret on a reference rather than the enclosing declaration; one past a declaration's name; a local declaration inside a lambda; a destructuring entry targeting the local rather than `componentN`; an operator; whitespace / comment / non-navigable keyword. One case asserts the contrast directly: the same caret that `referenceAtCaret` rejects still yields a target. - **`ReferenceAtCaretTest`** - kept as-is, as the regression proof that loosening visibility changed no behaviour. -- **`FindUsagesTest`** - the `lib` + `app(dependsOn = lib)` fixture from ADFA-4823: the three resolution scopes; each row of R4's visibility ladder, including a same-named decoy in another file; R3's super-walk, fake-override normalisation, workspace-boundary cutoff and constructor expansion; a Java-source target; dedup, ordering and ranges; the declaration's absence; a pre-cancelled `cancelChecker` returning empty without resolving; and a usage in an open unsaved file (via the `enableParserEventSystem = true` fixture). +- **`FindUsagesTest`** (20) - the `lib` + `app(dependsOn = lib)` fixture from ADFA-4823: the three resolution scopes; each row of R4's visibility ladder, asserted on the plan's scope rather than the result set; R3's super-walk, workspace-boundary cutoff and constructor expansion; imports; a Java-source target; a same-named decoy in another package; ordering; property reads and writes; a stdlib reference; a caret that names nothing; and a pre-cancelled request. +- **`FindUsagesLiveDocumentTest`** (2) - R5's live tier, which needs `enableParserEventSystem`: a usage that exists only in an unsaved buffer is found, and one deleted in the buffer but still on disk is not. +- **`AnalysisSerializationTest`** (+5) - `COMMAND`'s three ordering properties, plus `retryingOnPreemption`'s one-retry-with-a-fresh-checker contract and its refusal to loop. - **`KotlinCodeActionTooltipTagTest`** - the new tag row. -- **The `showLocations` helper** - one read per file, hits grouped by file, a stale line past EOF dropped. +- **`SearchResultGroupingTest`** (10, in `:app`) - single-line and multi-line hits, a hit on a line that no longer exists, a column past its line's end, only-the-wanted-lines collection, a short file, an unreadable file, and several hits in one file from one read. Not unit-testable, so covered by on-device QA via the "Steps to QA" field on ADFA-4824: the menu item and its tooltip tag, the panel with a large result set, cancelling mid-search, and typing during a search without losing it. From d308b947db3adc7c2a780c07cd2d2a81ce825a8c Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 4 Aug 2026 12:19:41 +0000 Subject: [PATCH 08/11] ADFA-4824: Fix the lossy module-dependents map The direct- and refinement-dependents maps were built per module and merged with `reduce { acc, value -> acc + value }`. `Map + Map` *replaces* a shared dependency's dependent set rather than merging it, so a module used by more than one other kept only the last of them. Find usages reads that map for R4's public-visibility scope, so a public declaration in a module with two consumers silently reported no usages in all but one of them - and `reduce` additionally threw on an empty module list. Accumulate into one map across all modules instead. --- .../services/ModuleDependentsProvider.kt | 59 +++++++++---------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ModuleDependentsProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ModuleDependentsProvider.kt index 5b057064c8..d334483f9c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ModuleDependentsProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ModuleDependentsProvider.kt @@ -9,59 +9,58 @@ import org.jetbrains.kotlin.cli.jvm.index.JavaRoot import org.jetbrains.kotlin.com.intellij.mock.MockProject import org.jetbrains.kotlin.com.intellij.util.containers.ContainerUtil.createConcurrentSoftMap -internal class ModuleDependentsProvider : KtLspService, KotlinModuleDependentsProviderBase() { - +internal class ModuleDependentsProvider : + KotlinModuleDependentsProviderBase(), + KtLspService { private lateinit var modules: List override fun setupWith( project: MockProject, index: KtSymbolIndex, modules: List, - libraryRoots: List + libraryRoots: List, ) { this.modules = modules } private val directDependentsByKtModule by lazy { - modules.asSequence() - .map { module -> - buildDependentsMap(module, module.allDirectDependencies()) - } - .reduce { acc, value -> acc + value } + buildDependentsMap(modules) { it.allDirectDependencies() } } private val transitiveDependentsByKtModule = createConcurrentSoftMap>() private val refinementDependentsByKtModule by lazy { - modules - .asSequence() - .map { buildDependentsMap(it, it.transitiveDependsOnDependencies.asSequence()) } - .reduce { acc, map -> acc + map } + buildDependentsMap(modules) { it.transitiveDependsOnDependencies.asSequence() } } - override fun getDirectDependents(module: KaModule): Set { - return directDependentsByKtModule[module].orEmpty() - } + override fun getDirectDependents(module: KaModule): Set = directDependentsByKtModule[module].orEmpty() - override fun getRefinementDependents(module: KaModule): Set { - return refinementDependentsByKtModule[module].orEmpty() - } + override fun getRefinementDependents(module: KaModule): Set = refinementDependentsByKtModule[module].orEmpty() - override fun getTransitiveDependents(module: KaModule): Set { - return transitiveDependentsByKtModule.computeIfAbsent(module) { key -> + override fun getTransitiveDependents(module: KaModule): Set = + transitiveDependentsByKtModule.computeIfAbsent(module) { key -> computeTransitiveDependents( - key + key, ) } - } } +/** + * Inverts every module's dependency edges into one dependency -> dependents map. + * + * Accumulated across all of [modules] rather than built per module and merged: `Map + Map` *replaces* a + * shared dependency's dependent set, so a module used by more than one other kept only the last of them + * and find usages then missed every call site in the rest. + */ private fun buildDependentsMap( - module: KaModule, - dependencies: Sequence, -): Map> = buildMap { - dependencies.forEach { dependency -> - if (dependency == module) return@forEach - val dependents = computeIfAbsent(dependency) { mutableSetOf() } - dependents.add(module) + modules: List, + dependenciesOf: (KtModule) -> Sequence, +): Map> = + buildMap> { + modules.forEach { module -> + dependenciesOf(module).forEach { dependency -> + if (dependency != module) { + getOrPut(dependency) { mutableSetOf() }.add(module) + } + } + } } -} From 12e19c25765a25fa894f634efb297ad9a0b60f67 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 4 Aug 2026 12:19:41 +0000 Subject: [PATCH 09/11] ADFA-4824: Stop the usage search losing or over-scanning results Five fixes to the search itself, all of them cases where it answered "no references" for a symbol with plenty, or did far more work than it needed to: - A candidate file preempted twice escaped `retryingOnPreemption` as a `CancellationException` and unwound the whole search, discarding every location already collected. Preemption is keystroke-driven work winning the lock, not the user cancelling, so it now costs that file like any other candidate failure. Genuine cancellation still propagates. - The declaration path came from the VFS alone, but the file the user is editing is a live `KtFile` whose `virtualFile` is a `LightVirtualFile`. A local or `private` target therefore had no path in the common case, fell through R4's single-file scope and searched the whole module graph for a variable that cannot leave one block. Derive it through `backingFilePath` first, as go-to-definition does, and fall back to module scope - never to the dependents graph - when there is still no path. - The text prefilter went through `StringSearch.containsWord`, which reads only a file's first megabyte (silently dropping usages below the mark), reads through one process-global `ByteBuffer` the Java server mutates from its own threads, and rethrows an unreadable file as a `RuntimeException`, which aborted the entire search. Replaced with `mentionsName`: whole file, line by line through `FileManager.getReader`, so an open file is still matched against its live buffer, and an unreadable one drops out with a log. - The prefilter had no cancel checker, so cancelling mid-scan let it read every remaining source file before the result was discarded. It now checks per file. - Every prefiltered candidate paid an analysis-lock acquisition, a FIR session and a match-set restore before the pure-PSI name filter could reject it. On a short, common name most candidates only mention it in a comment or a string literal. Run the name filter first and skip the session entirely when it finds nothing. --- .../lsp/kotlin/navigation/FindUsages.kt | 157 ++++++++++++++---- 1 file changed, 126 insertions(+), 31 deletions(-) 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 5b7227570b..c3273a6e5c 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 @@ -1,11 +1,13 @@ package com.itsaky.androidide.lsp.kotlin.navigation import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence +import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.modules.isAnalysisCancellation import com.itsaky.androidide.lsp.kotlin.compiler.modules.isSourceModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.retryingOnPreemption @@ -18,7 +20,6 @@ import com.itsaky.androidide.models.Location import com.itsaky.androidide.models.Range import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.projects.FileManager -import com.itsaky.androidide.projects.util.StringSearch import kotlinx.coroutines.future.await import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.platform.projectStructure.KotlinModuleDependentsProvider @@ -32,12 +33,14 @@ import org.jetbrains.kotlin.analysis.api.symbols.KaSymbolVisibility import org.jetbrains.kotlin.analysis.api.symbols.markers.KaNamedSymbol import org.jetbrains.kotlin.analysis.api.symbols.pointers.KaSymbolPointer import org.jetbrains.kotlin.analysis.api.symbols.sourcePsiSafe +import org.jetbrains.kotlin.analysis.low.level.api.fir.util.originalKtFile import org.jetbrains.kotlin.com.intellij.psi.PsiElement import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil import org.jetbrains.kotlin.idea.references.mainReference import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.KtSimpleNameExpression import org.slf4j.LoggerFactory +import java.io.IOException import java.nio.file.Path private val logger = LoggerFactory.getLogger("FindUsages") @@ -93,7 +96,7 @@ internal suspend fun findUsagesAt(params: ReferenceParams): ReferenceResult { return try { val plan = planAt(params) ?: return ReferenceResult.empty() - val candidates = candidateFiles(plan) + val candidates = candidateFiles(plan, params.cancelChecker) logger.debug("Usage search for '{}': {} candidate file(s)", plan.simpleName, candidates.size) val locations = @@ -157,15 +160,32 @@ private fun KaSession.planFor(target: CaretTarget): SearchPlan? { } val simpleName = prefilterName(symbol) ?: return null - val declarationPath = declaration.containingFile?.virtualFile?.let { runCatching { it.toNioPath() }.getOrNull() } return SearchPlan( simpleName = simpleName, matchSet = matchSet(symbol).map { it.createPointer() }, - scope = scopeOf(symbol, declaration, declarationPath), + scope = scopeOf(symbol, declaration, pathOf(declaration)), ) } +/** + * The on-disk path of [declaration]'s file, or null when it has none. + * + * [backingFilePath] is tried before the VFS, exactly as in go-to-definition: the file the user is + * editing is a live [KtFile] built from the editor buffer, whose `virtualFile` is a non-physical + * `LightVirtualFile`. Reading the VFS alone would leave the common case pathless, and a pathless local + * or `private` target loses its single-file scope (R4) and widens to the whole module graph. + */ +private fun pathOf(declaration: PsiElement): Path? { + val psiFile = declaration.containingFile ?: return null + val ktFile = psiFile as? KtFile + + return (ktFile?.backingFilePath ?: ktFile?.originalKtFile?.backingFilePath) + ?: psiFile.virtualFile + ?.takeIf { it.fileSystem.protocol == "file" } + ?.let { runCatching { it.toNioPath() }.getOrNull() } +} + /** * The declaration [target] names. * @@ -254,16 +274,19 @@ private fun KaSession.scopeOf( // A local is confined to its declaring block, and a private declaration to its file: Kotlin's // private top-level is file-private, and a private member cannot escape the class body it is // written in. Both are the cheap, exact cases. - if (fileOnly != null && - (symbol.location == KaSymbolLocation.LOCAL || symbol.visibility == KaSymbolVisibility.PRIVATE) - ) { + val fileConfined = + symbol.location == KaSymbolLocation.LOCAL || symbol.visibility == KaSymbolVisibility.PRIVATE + if (fileOnly != null && fileConfined) { return fileOnly } val module = moduleOf(declaration) ?: return fileOnly ?: UsageSearchScope.Modules(sourceModules()) - if (symbol.visibility == KaSymbolVisibility.INTERNAL) { - // internal is module-wide, and there is no associated test module to widen to: this project - // model builds one module per Gradle module from the main source set only. + + // internal is module-wide, and there is no associated test module to widen to: this project model + // builds one module per Gradle module from the main source set only. A file-confined target with no + // derivable path lands here too - it cannot be narrowed to one file, but it is still unreferenceable + // outside its own module, so it must not fall through to the dependents below. + if (fileConfined || symbol.visibility == KaSymbolVisibility.INTERNAL) { return UsageSearchScope.Modules(listOf(module)) } @@ -301,12 +324,14 @@ private fun sourceModules(): List = * * The prefilter is a one-directional over-approximation: a file that mentions the name but contains no * usage is parsed and discarded, while a file that does not mention it cannot contain a named usage. - * [StringSearch.containsWord] reads an open file's live editor buffer rather than its saved bytes, so a - * usage typed but not yet saved is still found - which matters here, because find usages is run *while* - * editing. + * [mentionsName] reads an open file's live editor buffer rather than its saved bytes, so a usage typed + * but not yet saved is still found - which matters here, because find usages is run *while* editing. */ context(env: AbstractCompilationEnvironment) -internal fun candidateFiles(plan: SearchPlan): List = +internal fun candidateFiles( + plan: SearchPlan, + cancelChecker: ICancelChecker, +): List = when (val scope = plan.scope) { // The declaration's own file always contains its name, so there is nothing to filter. is UsageSearchScope.SingleFile -> { @@ -320,11 +345,57 @@ internal fun candidateFiles(plan: SearchPlan): List = .flatMap { it.computeFiles(extended = true) } .mapNotNull { runCatching { it.toNioPath() }.getOrNull() } .distinct() - .filter { StringSearch.containsWord(it, plan.simpleName) } - .toList() + .filter { + // Checked per file: a whole-workspace scan is seconds of I/O, and cancelling must stop it + // rather than let it run to completion and then discard the result. + cancelChecker.abortIfCancelled() + mentionsName(it, plan.simpleName) + }.toList() + } + } + +/** + * Whether the file at [path] writes [name] as a whole word. + * + * Read line by line through [FileManager] rather than through `StringSearch.containsWord`: that helper + * scans only a file's first megabyte, so a usage below the mark is silently dropped, it does so through + * one process-global `ByteBuffer` the Java LSP mutates concurrently from its own threads, and it rethrows + * an unreadable file as a `RuntimeException` - which here would abort the whole search rather than skip + * one file. [FileManager] keeps the property that matters: an open file is matched against its live + * editor buffer. A name cannot span a line break, so matching per line is exact. + */ +private fun mentionsName( + path: Path, + name: String, +): Boolean = + try { + FileManager.getReader(path).use { reader -> + reader.lineSequence().any { it.containsWord(name) } } + } catch (e: IOException) { + // One unreadable file must not lose the whole result. + logger.debug("Usage search could not prefilter candidate {}", path, e) + false } +/** Whether this line contains [name] bounded by non-identifier characters on both sides. */ +private fun String.containsWord(name: String): Boolean { + var at = indexOf(name) + while (at >= 0) { + val before = at - 1 + val after = at + name.length + if ((before < 0 || !this[before].isIdentifierChar()) && + (after >= length || !this[after].isIdentifierChar()) + ) { + return true + } + at = indexOf(name, at + 1) + } + return false +} + +private fun Char.isIdentifierChar(): Boolean = isLetterOrDigit() || this == '_' || this == '$' + /** * Every usage of [plan]'s target in the file at [path]. * @@ -345,12 +416,28 @@ private suspend fun usagesIn( emptyList() } else { env.project.read { - analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { - matchingReferences(ktFile, plan, path, cancelChecker) + // 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) + if (named.isEmpty()) { + emptyList() + } else { + analyzeMaybeDangling(ktFile, AnalysisPriority.COMMAND, cancelChecker) { + matchingReferences(named, plan, ktFile, path, cancelChecker) + } } } } } + } 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). + logger.debug("Usage search gave up on candidate {}: preempted twice", path) + emptyList() } catch (e: Throwable) { if (e.isAnalysisCancellation()) throw e // One unresolvable file must not lose the whole result. @@ -375,19 +462,31 @@ private suspend fun ktFileFor(path: Path): KtFile? = } /** - * The references in [ktFile] that resolve into [plan]'s match set. + * The simple-name references in [ktFile] written as [simpleName]. * - * The name filter runs first and on PSI alone, so only references that could possibly match are ever - * resolved. It is also what implements "convention references are not discovered": `a + b` contains no - * `plus` token, so it is never a candidate. + * PSI alone, so it can rule a candidate file out before any analysis session is opened. It is also what + * implements "convention references are not discovered": `a + b` contains no `plus` token, so it is never + * a candidate. + */ +private fun namedReferences( + ktFile: KtFile, + simpleName: String, +): List = + PsiTreeUtil + .collectElementsOfType(ktFile, KtSimpleNameExpression::class.java) + .filter { it.getReferencedName() == simpleName } + +/** + * The [references] that resolve into [plan]'s match set. * * Match-set pointers are restored **once** for this session; [KaSymbol] equality within a single * session compares the underlying FIR symbol, so it is the right comparison once both sides come from * the same session (R6). */ private fun KaSession.matchingReferences( - ktFile: KtFile, + references: List, plan: SearchPlan, + ktFile: KtFile, path: Path, cancelChecker: ICancelChecker, ): List { @@ -399,14 +498,10 @@ private fun KaSession.matchingReferences( return emptyList() } - return PsiTreeUtil - .collectElementsOfType(ktFile, KtSimpleNameExpression::class.java) - .asSequence() - .filter { it.getReferencedName() == plan.simpleName } - .mapNotNull { reference -> - cancelChecker.abortIfCancelled() - if (resolvesInto(reference, targets)) locationOf(reference, ktFile, path) else null - }.toList() + return references.mapNotNull { reference -> + cancelChecker.abortIfCancelled() + if (resolvesInto(reference, targets)) locationOf(reference, ktFile, path) else null + } } /** Whether [reference] resolves to one of [targets]. */ From 9df2eec1ea6ea61929c362afbe44c8d43a5274b1 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 4 Aug 2026 12:19:41 +0000 Subject: [PATCH 10/11] ADFA-4824: Guard the search panel against a superseded publish Moving the result-file reads off the main thread made the publish asynchronous, but nothing checked that the request still owned the panel. Two overlapping searches published in completion order, not request order, so a slow find-references that started first landed last and overwrote the newer search the user was looking at - and rows they never asked for navigated somewhere unrelated when tapped. `showLocations` now claims the panel with a request counter and captures `EditorViewModel.currentSearchGeneration`; the callback publishes only if both still hold. The counter catches a superseding find-references, the generation catches a text search publishing in between. Panel visibility moves next to the rows in `publishLocations` for the same reason: it was committed eagerly while the publish could be skipped entirely (activity recreated mid-read), which left the panel open with the "no results" placeholder hidden over the previous query's rows. --- .../androidide/lsp/IDELanguageClientImpl.java | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java b/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java index 1799fe673a..449386aebd 100755 --- a/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java +++ b/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java @@ -59,6 +59,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import kotlin.Unit; import org.slf4j.Logger; @@ -105,6 +106,9 @@ public static void shutdown() { private final Map> diagnostics = new HashMap<>(); + /** Identifies the most recent {@link #showLocations(List)} request; older ones must not publish. */ + private final AtomicInteger showLocationsRequest = new AtomicInteger(); + protected EditorHandlerActivity activity; private IDELanguageClientImpl(EditorHandlerActivity provider) { @@ -269,10 +273,13 @@ public void showLocations(List locations) { return; } - boolean error = locations == null || locations.isEmpty(); - activity.handleSearchResultVisibility(error); + // Claims the panel for this request. The publish below is asynchronous, so without this a slow + // request that started first would land last and overwrite the newer search the user is looking at. + final int request = showLocationsRequest.incrementAndGet(); + boolean error = locations == null || locations.isEmpty(); if (error) { + activity.handleSearchResultVisibility(true); activity .setSearchResultAdapter( new SearchListAdapter(Collections.emptyMap(), this::noOp, this::noOp)); @@ -308,14 +315,22 @@ public void showLocations(List locations) { } if (onDisk.isEmpty()) { - activity.handleSearchResults(fromEditors); + publishLocations(fromEditors); return; } + // Some other search may publish (and bump the generation) while the read is in flight; capture it + // here so this request does not overwrite whatever replaced it. + final int generation = activity.getEditorViewModel().getCurrentSearchGeneration(); + TaskExecutor.executeAsyncProvideError( () -> SearchResultGrouping.INSTANCE.readFromDisk(onDisk), (result, throwable) -> { - if (!canUseActivity()) { + if (!canUseActivity() + || request != showLocationsRequest.get() + || generation != activity.getEditorViewModel().getCurrentSearchGeneration()) { + // Superseded, or the activity went away. Leave the panel to whoever owns it now: this + // request's results would be an answer to a question no longer on screen. return; } final Map> merged = new HashMap<>(fromEditors); @@ -324,7 +339,7 @@ public void showLocations(List locations) { } else { LOG.error("Failed to read search result files", throwable); } - activity.handleSearchResults(merged); + publishLocations(merged); }); } @@ -481,4 +496,14 @@ private List mapAsGroup(Map> map) { private Unit noOp(final Object obj) { return Unit.INSTANCE; } + + /** + * Shows {@code results} in the search panel. + * + * Visibility and rows are committed together: a publish that never happens - superseded, or the activity recreated mid-read - must not leave the panel open with the "no results" placeholder hidden over the previous query's rows. + */ + private void publishLocations(final Map> results) { + activity.handleSearchResultVisibility(results.isEmpty()); + activity.handleSearchResults(results); + } } From 8ea969d59493bf1976622df7ccd803478d63ec56 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 4 Aug 2026 12:19:42 +0000 Subject: [PATCH 11/11] ADFA-4824: Sync the find-usages doc with the review fixes R4 gains the path-derivation fallback, R5 the new prefilter and its cancellation granularity, R6 the PSI-before-session ordering, R9 the twice-preempted candidate, R10 the panel's staleness guard and R12 the two failures now isolated per file. The flow diagram and the touched-components list follow. Also drops two claims that were never true of the shipped code: that locals skip the pointer machinery, and that the `StringSearch` limits were carried over unfixed. --- docs/features/kotlin-find-usages.md | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/features/kotlin-find-usages.md b/docs/features/kotlin-find-usages.md index 1839605d64..21d8086f0d 100644 --- a/docs/features/kotlin-find-usages.md +++ b/docs/features/kotlin-find-usages.md @@ -93,6 +93,8 @@ The walk goes **up** only. Usages reachable solely through a subclass (`Base.foo The ticket's three resolution scopes fall out of this one code path rather than being three implementations. Cheap cases stay cheap: a search on a local variable never leaves the open file. +The first three rows need the declaration's path. The file the user is editing is a live `KtFile` built from the editor buffer, whose `virtualFile` is a non-physical `LightVirtualFile`, so the path comes from `backingFilePath` first and the VFS only as a fallback - go-to-definition's derivation. A target that still has no path cannot be confined to one file, but it is still unreferenceable outside its own module, so it falls back to the `internal` row rather than to the last one. + `internal` needs no widening for test sources. There is no test module to widen to - `collectKtModules` builds one `KtSourceModule` per Gradle module from `mainSourceSet` only, and `directFriendDependencies` is empty everywhere. **R5 - Candidate discovery.** Two tiers, because find usages is run *while* editing and unsaved text must not be invisible: @@ -102,19 +104,19 @@ The ticket's three resolution scopes fall out of this one code path rather than | open in the editor | the live buffer | `ktSymbolIndex.getCurrentKtFile(path).await()`, awaited **outside** `project.read` | | everything else | disk | `ktSymbolIndex.getKtFile(path)` | -The prefilter is `StringSearch.containsWord`, word-boundary exact on the target's simple name. It needs no live-buffer branch of its own: it already reads `FileManager.getActiveDocument(file)` when the file is open, and only falls back to disk otherwise. Its errors are one-directional: a file that mentions the name but contains no usage is parsed and discarded (wasted work, correct result), while a file that does not mention the name cannot contain a named usage. +The prefilter is `mentionsName`, word-boundary exact on the target's simple name, reading line by line through `FileManager.getReader(path)` - which returns the live document when the file is open and the file itself otherwise, so the two tiers above need no branch of their own. Its errors are one-directional: a file that mentions the name but contains no usage is parsed and discarded (wasted work, correct result), while a file that does not mention the name cannot contain a named usage. An unreadable file drops out of the scan with a log rather than failing the search. Open documents are tab-count many, so the live tier is free. Without it, a usage the user just typed would be missed entirely - the prefilter would never select the file, so it would never be parsed. -Two pre-existing `StringSearch` limits carry over. It reads only the **first 1 MB** of a file, so a usage past that point is missed; and it scans through one shared static `ByteBuffer` with no synchronisation, so a Kotlin usage search running concurrently with a Java find-references can corrupt the other's scan. Neither is introduced here and neither is fixed here, but the second is worth a ticket. +Deliberately **not** `StringSearch.containsWord`, the equivalent helper the Java server prefilters with. It reads only the first 1 MB of a file, so a usage below the mark would be silently dropped; it reads through one process-global `ByteBuffer` that the Java server mutates concurrently from its own threads; and it rethrows an unreadable file as a `RuntimeException`, which here would abort the whole search. A name cannot span a line break, so matching per line loses nothing. -Only `KtSimpleNameExpression`s are examined. That is what makes the name filter cheap - it runs on PSI alone, before any resolution - and it is also what implements "convention references are not results": `a + b` contains no `plus` token, so it is never a candidate. The cost is that a **KDoc `[link]`** to the target is not reported, even though go-to-definition navigates from one; a documented gap rather than a decision worth its own machinery in v1. +Only `KtSimpleNameExpression`s are examined. That is what makes the name filter cheap - it runs on PSI alone, so it runs *before the analysis session is opened*, and a text-prefilter hit whose only mention is a comment or a string literal never costs an analysis-lock acquisition, a FIR session or a match-set restore. It is also what implements "convention references are not results": `a + b` contains no `plus` token, so it is never a candidate. The cost is that a **KDoc `[link]`** to the target is not reported, even though go-to-definition navigates from one; a documented gap rather than a decision worth its own machinery in v1. **R6 - Identity.** A reference is a usage if its resolved symbol is in the match set. Deciding that across files needs care, because `KaSymbol` is session-scoped and the same declaration exists as two PSI instances - the on-disk `KtFile` cached in the index, and the dangling `KtFile` built from the editor buffer for an open file. Matching therefore uses `KaSymbolPointer`: `createPointer()` for each match-set member in the caret's session, then `restoreSymbol(session)` **once per candidate session**, then `==` against each resolved candidate symbol inside that session. This is the platform's cross-session identity mechanism, with structural implementations per symbol kind, and it is the direct analogue of the Java server re-deriving its target `Element` inside each compile task. -Locals skip all of it: R4 confines them to one file and therefore one session, where instance equality is valid and cheapest. +Locals pay almost none of it: R4 confines them to one file, so there is a single candidate session and the pointers restore once. A pointer that fails to restore drops that session's candidates, with a log. That under-reports rather than reporting something false, which is the safe direction, and it is tested. @@ -136,10 +138,10 @@ There is **no result cap**. See R10 for why one is not needed. Granularity is per candidate file, and it is load-bearing: -- **One analysis session per candidate file.** A preemption by completion costs one file's work, which is retried once - `findDefinitionAt`'s pattern. One session for the whole search would let a single keystroke discard a whole-workspace scan. +- **One analysis session per candidate file.** A preemption by completion costs one file's work, which is retried once - `findDefinitionAt`'s pattern. One session for the whole search would let a single keystroke discard a whole-workspace scan. A file preempted *twice* is dropped like any other failed candidate (R12), not rethrown: keystroke-driven work winning the lock must not turn a search with plenty of hits into "no references". - **`project.read` per candidate file, never once for the search.** A whole-workspace search holding the read lock start to finish would block every `project.write`, which is what index refresh needs. - **The live-document await stays outside `project.read`.** The refresh it waits on needs `project.write`; awaiting it under the read lock deadlocks. Go-to-definition's R10 records the same constraint. -- `params.cancelChecker` is honoured between files **and** between references within a file. +- `params.cancelChecker` is honoured per prefiltered file, between candidate files **and** between references within a file. The prefilter checks it per file rather than once for the pass: a whole-workspace scan is seconds of I/O, and cancelling has to stop it rather than let it finish and discard the result. The prefilter pass runs first, before any analysis, holding no locks. No progress count is shown - `launchCancellableAsyncWithProgress` takes a fixed `@StringRes`, and threading a live count through it would change a shared editor API for a cosmetic gain. No timeout and no file budget: the search finishes or the user cancels. @@ -151,11 +153,13 @@ A file with an **open editor** is still resolved **on** the UI thread. Its `Cont Reads drop from O(hits) to O(files), peak memory is one line rather than one file (deliberately *not* a per-file content cache - holding every result file's text at once is the wrong trade on a phone), and the main thread does no I/O. This removes the need for a result cap, which would otherwise silently truncate. +Because the publish is now asynchronous, it is also guarded: `showLocations` claims the panel with a request counter and captures `EditorViewModel.currentSearchGeneration`, and the callback publishes only if both still hold. Otherwise a slow request that started first would land last and overwrite the newer search the user is looking at. Panel visibility is committed *with* the rows for the same reason - a publish that never happens (superseded, or the activity recreated mid-read) must not leave the panel open with the "no results" placeholder hidden over the previous query's rows. + Two behaviour changes, both improvements: a hit whose line no longer exists is dropped rather than yielding whatever `Content` returned, and a file whose every hit is stale is omitted rather than contributing an empty group. The grouping and line extraction live in `SearchResultGrouping` so they can be unit-tested; the activity call is a thin shell. **R11 - Not ready.** No `CompilationEnvironment` for the file (a script, a file outside the content roots), or no analysis session yet, answers empty and logs. There is no "still indexing" signal; that gap is cross-cutting across every LSP feature and is not solved here. -**R12 - Failure isolation.** A resolution failure on one candidate file drops that file and continues - one unparseable file must not lose the whole result. A failure in the target-resolution phase returns empty. Cancellation, including `AnalysisPreemptedException`, propagates rather than being reported as "no references". Nothing propagates an exception to the editor or leaves the progress flashbar up. +**R12 - Failure isolation.** A resolution failure on one candidate file drops that file and continues - one unparseable file must not lose the whole result. So does an unreadable one in the prefilter pass, and one preempted past `retryingOnPreemption`'s single retry. A failure in the target-resolution phase returns empty. Genuine cancellation propagates rather than being reported as "no references" - but preemption is not that, so it is isolated per file like any other candidate failure. Nothing propagates an exception to the editor or leaves the progress flashbar up. ## Non-goals @@ -213,12 +217,13 @@ FindReferencesAction.execAction lsp/kotlin/actions planFor(target) -> simpleName, matchSet pointers, scope [R3, R4, R6] } } - candidateFiles(plan) [R5] + candidateFiles(plan, cancelChecker) [R5] per candidate file: (retried once if preempted) [R9] await live PSI if open (outside project.read) env.project.read { + walk name references (PSI only); no hit -> skip the file [R5] analyzeMaybeDangling(file, COMMAND, cancelChecker) { - restore pointers once, walk name references, compare [R6] + restore pointers once, compare [R6] } } -> locations [R7] <- ReferenceResult(locations) [R8] @@ -239,7 +244,8 @@ Touched existing components: - **`AnalysisPriority` / `AnalysisScheduler`** - the new `COMMAND` tier, plus `retryingOnPreemption`, which holds the two invariants every command's retry depends on: a fresh `ScheduledCancelChecker` per attempt (`preempt()` latches), and re-fetching the `KtFile` inside the attempt ([ADR 0011](../adr/0011-command-analysis-priority.md)). - **`GoToDefinitionAction`, `OrganizeImportsAction`, `ImplementMembersAction`** - migrated to `COMMAND`; the latter two gain the retry they never had, and take the delegate `ICancelChecker` rather than a pre-wrapped one since wrapping is now per attempt. - **`GoToDefinition.symbolsAt`** - `internal`, so the reference-at-caret resolution is shared rather than duplicated. -- **`IDELanguageClientImpl.showLocations`** - R10's grouped streaming rewrite. +- **`services/ModuleDependentsProvider`** - its direct- and refinement-dependents maps are now accumulated across all modules instead of built per module and merged with `Map + Map`, which *replaced* a shared dependency's dependent set. R4's last row reads that map, so a module used by more than one other silently lost every dependent but the last. +- **`IDELanguageClientImpl.showLocations`** - R10's grouped streaming rewrite, plus its staleness guard. - **`TooltipTag`** - one new constant (R1). Unchanged: `ReferenceParams`/`ReferenceResult`, `ILanguageServer`, `IDEEditor`, and every string resource.