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..449386aebd 100755 --- a/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java +++ b/app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.java @@ -44,23 +44,22 @@ 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.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import kotlin.Unit; import org.slf4j.Logger; @@ -107,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) { @@ -271,56 +273,74 @@ 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)); 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()) { + 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() + || 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); + if (result != null) { + merged.putAll(result); + } else { + LOG.error("Failed to read search result files", throwable); + } + publishLocations(merged); + }); } private Boolean applyActionEdits(@Nullable final IDEEditor editor, final CodeActionItem action) { @@ -476,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); + } } 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) + } +} 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..21d8086f0d --- /dev/null +++ b/docs/features/kotlin-find-usages.md @@ -0,0 +1,272 @@ +# Kotlin find usages (K2 LSP) + +- **Ticket:** ADFA-4824 (subtask of ADFA-3317; split out of the closed ADFA-3321 "Navigation") +- **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. + +`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. + +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: + +| File | Prefilter text | PSI | +|---|---|---| +| open in the editor | the live buffer | `ktSymbolIndex.getCurrentKtFile(path).await()`, awaited **outside** `project.read` | +| everything else | disk | `ktSymbolIndex.getKtFile(path)` | + +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. + +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, 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 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. + +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. 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 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. + +**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. + +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. 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 + +- **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). +- **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. +- **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 + 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] + } + } + 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, compare [R6] + } + } -> locations [R7] + <- ReferenceResult(locations) [R8] +``` + +New components: + +- **`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, 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. +- **`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. + +## Verification + +`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest` and `:app:testV7DebugUnitTest`, split to match the helpers: + +- **`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`** (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. +- **`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. + +## 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). 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/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/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/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/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) + } + } + } } -} 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..c3273a6e5c --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt @@ -0,0 +1,536 @@ +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 +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 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.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") + +/** + * 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, params.cancelChecker) + 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 + + return SearchPlan( + simpleName = simpleName, + matchSet = matchSet(symbol).map { it.createPointer() }, + 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. + * + * 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. + 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()) + + // 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)) + } + + // 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. + * [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, + 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 -> { + listOf(scope.path) + } + + is UsageSearchScope.Modules -> { + scope.modules + .asSequence() + .filter { it.isSourceModule } + .flatMap { it.computeFiles(extended = true) } + .mapNotNull { runCatching { it.toNioPath() }.getOrNull() } + .distinct() + .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]. + * + * 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 { + // 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. + 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 simple-name references in [ktFile] written as [simpleName]. + * + * 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( + references: List, + plan: SearchPlan, + ktFile: KtFile, + 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 references.mapNotNull { reference -> + cancelChecker.abortIfCancelled() + if (resolvesInto(reference, targets)) locationOf(reference, ktFile, path) else null + } +} + +/** 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 9380913215..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 @@ -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 @@ -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() @@ -197,47 +200,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/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/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, 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/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) + } +} 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() + } +} 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()) } }