feat(ADFA-4824): Find usages in the Kotlin K2 LSP - #1624
Conversation
… ADR Requirements, glossary and design for find usages in the K2 Kotlin LSP, ahead of the implementation. Follows the shape ADFA-4823 established for go-to-definition. Two decisions here reach outside lsp/kotlin and are recorded as such: - ADR 0011 adds AnalysisPriority.COMMAND between DIAGNOSTICS and INTERACTIVE. INTERACTIVE means "a newer request makes me stale, discard my work", which is false for a command the user invoked and is watching. Organize-imports and implement-members can be silently discarded by a completion request today, with no retry. - showLocations reads each result file in full once per hit, on the main thread. Find usages makes that a real cost rather than a latent one. Also corrects go-to-definition's claim that ADFA-4824 would reuse referenceAtCaret verbatim. It cannot: that helper deliberately resolves nothing when the caret is on a declaration's own name, which is exactly where find usages is invoked from.
INTERACTIVE means "a newer request of the same priority makes me stale, so discard my work". That is right for completion and signature help, which fire on keystrokes. It is wrong for a command the user invoked from the code-actions menu and is watching a progress flashbar for: the request is not stale, so discarding it produces a wrong answer rather than no answer. Three commands ran at INTERACTIVE anyway. Go-to-definition noticed and worked around it with a one-shot retry. Organize-imports and implement-members did not: a completion request discards them, the AnalysisPreemptedException lands in their outer runCatching, and the action silently does nothing. Adds COMMAND between DIAGNOSTICS and INTERACTIVE with supersedesSamePriority = false, so two commands never discard each other, and migrates all three actions to it. Ordered below INTERACTIVE deliberately: a long command must not starve the completion popup, which on a phone is part of how text gets entered. See ADR 0011 for the rejected alternative of ordering it above. The cost of that ordering is that commands stay preemptable, so each one retries. Extracts retryingOnPreemption to hold the two invariants that retry depends on: a fresh ScheduledCancelChecker per attempt (preempt() latches, so a reused checker aborts the retry at its first checkpoint), and re-fetching the KtFile inside the attempt (the preemptor also refreshed the live PSI, unregistering the file the previous attempt held). The two migrated actions now take the delegate ICancelChecker rather than a pre-wrapped ScheduledCancelChecker, since the wrapping is per attempt. Prep for find usages, which is the case that makes this acute: it is user-invoked, takes one session per candidate file, and can run for seconds, so on INTERACTIVE a single keystroke would discard it.
Find usages is invoked from either end: on a declaration's own name, or on
any reference to it. Go-to-definition's referenceAtCaret cannot serve the
first case, and not by accident - it is built so a caret on a declaration's
own name resolves nothing, which is its no-self-jump rule. That is exactly
the caret find usages starts from.
targetAtCaret is declaration-first, falling back to referenceAtCaret. It
returns a CaretTarget rather than a bare KtElement so the resolution step
does not have to re-derive which case it is looking at.
Two details worth naming:
- The declaration check requires the caret's leaf to *be* the declaration's
name identifier, not merely to sit inside a declaration. Every caret has
an enclosing declaration - a call site's nearest one is the function
containing it - so proximity alone would target that container for every
reference in the file.
- It checks both the leaf at the offset and the one before it. referenceAtCaret
retries only when the primary leaf names nothing, which is not enough here:
a caret just past `fun target` lands on '(', navigable in its own right for
the invoke convention, so checking only that leaf made a caret one character
past a declaration's name find nothing. Caught by the test for it.
Declaration-first is observable on a destructuring entry, which is both a
declaration and a convention reference: `x` in `val (x, y) = p` targets the
local x here, while go-to-definition navigates from that same caret to
component1. Deliberate, and asserted in both test classes.
navigableLeafAt becomes internal so the accept-list is shared rather than
duplicated. ReferenceAtCaret's behaviour is unchanged, and its tests are
kept as the proof of that.
Fills in KotlinLanguageServer.findReferences, which until now answered empty. There is no reference-search infrastructure to build on: the bundled analysis-api-standalone jar ships no ReferencesSearch, no PsiSearchHelper and no word index, and KtFileMetadata records declarations only. So the search is target -> match set -> scope -> candidate files -> resolve. Match set (R3). The target, plus its workspace-source supers, plus a classifier's constructors. Supers because a call dispatched through Base.foo may reach Derived.foo. Constructors because Foo() resolves to a constructor, not to the class, so without them a search on `class Foo` misses every instantiation. The up-walk stops at the workspace boundary: with Any.toString in the match set, a search on an overridden toString would report every .toString() call in the workspace. Both sides of every comparison are normalised through fakeOverrideOriginal, since a call through a subtype that does not redeclare the member resolves to a substituted fake override. Scope (R4) comes from the target's visibility, which is an exact bound rather than a heuristic. local/private stays in the file, internal in the module, anything more visible reaches the module and its transitive dependents. The ticket's three resolution scopes fall out of this rather than being three implementations, and a search on a local variable never leaves the open file. Candidates (R5) are narrowed by StringSearch.containsWord, which already reads an open file's live editor buffer rather than its saved bytes - so a usage typed but not yet saved is still found. That matters more here than for go-to-definition: find usages is run *while* editing. The name filter is also what implements "convention references are not results": `a + b` contains no plus token, so it is never a candidate. Identity (R6) uses KaSymbolPointer, restored once per candidate session, then compared with ==. KaSymbol cannot cross a session boundary, and KaSymbol equality within one session compares the underlying FIR symbol, so both sides must come from the same session. Neither PSI identity nor a (file, offset) key would work: the live and on-disk instances of the target's own file disagree about offsets as soon as there are unsaved edits, which would silently drop every cross-file usage in the common case. A pointer that will not restore drops that file rather than falling back to a looser comparison - under-report, never report something false. Scheduling (R9) is per candidate file: one analysis session and one project.read each, so a preemption costs one file and index refresh is never blocked for the length of a search. The live-PSI await stays outside project.read, since the refresh it waits on needs project.write. Tests cover the three resolution scopes, each row of the visibility ladder, the super-walk and its workspace cutoff, constructor expansion, imports, a Java-source target, a symbol-vs-name decoy, ordering, cancellation, and a usage that exists only in an unsaved buffer. Java files are not searched for usages, and neither are test source sets - AndroidModule.getSourceDirectories() returns mainSourceSet only, so test sources are not content roots for any LSP feature. Both documented in docs/features/kotlin-find-usages.md.
Mirrors Java's action and its menu position, immediately after Go to definition. The work itself is the editor's existing cancellable request, so the action only starts it. Carries its own tooltip tag rather than reusing Java's, following the split established for Kotlin go-to-definition and fix-imports, so the two languages can describe different behaviour. The tooltips database lives outside this repo, so the tag shows no text until a row exists for it - that row is a hand-off item, not code. Deliberately always visible for .kt/.kts and never conditioned on what the caret sits on: answering that needs PSI and the project read lock, and prepare() runs on the UI thread. A caret on whitespace therefore shows the item and flashes "no references". A .kts shows it and it does nothing, since a script has no CompilationEnvironment - both identical to go-to-definition.
showLocations read every result file in full, once per hit, on the main thread: a file with twelve usages was read and materialised twelve times, plus an exists() stat per hit. Java find-references has had this all along and simply rarely produces enough hits to hurt. Find usages does. A row needs only two short strings per hit - the hit's line and the matched text - so the fix is to group by file and read only the lines the hits touch: - One sequential BufferedReader pass per file, stopping after the last wanted line, retaining nothing. Reads drop from O(hits) to O(files) and peak memory is one line rather than one file. Deliberately not a per-file content cache, which would fix the repeated reads but hold every result file's text at once - the wrong trade on a phone. - The disk pass runs off the main thread via TaskExecutor, which posts the callback back to the UI thread. - A file with an open editor is still resolved on the UI thread. Its Content is live UI state that a background thread must not touch, and pulling a few lines out of it is substring work with no I/O. This is also what keeps unsaved edits reflected in the panel. The grouping and line extraction are extracted into SearchResultGrouping so they can be unit-tested; the activity call stays a thin shell. Two behaviour changes, both improvements: a hit whose line no longer exists is dropped rather than yielding whatever Content returned, and a file whose every hit is stale is omitted rather than contributing an empty group. The per-hit exists() check is gone because an unreadable file now yields no lines and therefore no rows.
Corrections found while implementing: - The prefilter needs no live-buffer branch of its own. StringSearch.containsWord already reads FileManager.getActiveDocument when the file is open. Records its two pre-existing limits too: it reads only the first 1 MB of a file, and it scans through one shared unsynchronised static buffer, so a concurrent Java find-references can corrupt the scan. - showLocations does not build the whole map off the main thread. A file with an open editor is resolved on the UI thread deliberately, because its Content is live UI state a background thread must not touch. - Only KtSimpleNameExpressions are examined, so a KDoc [link] to the target is not reported. Added to R5 and to the non-goals rather than left implicit. - An ambiguous reference at the caret searches its first resolved candidate. - targetAtCaret checks both candidate leaves, not one. - planAt/SearchPlan/candidateFiles are internal so the visibility ladder can be asserted; it is not observable from a result set, because symbol matching means a same-named decoy can never be a false positive whatever the scope. Also updates the verification section to the tests that now exist and their counts.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
📝 Walkthrough
WalkthroughAdds Kotlin K2 find-usages navigation with declaration-aware caret resolution, visibility-based scopes, live-buffer support, and grouped result rendering. Adds command-priority analysis retries and exposes find references through the Kotlin editor action menu. ChangesKotlin navigation and usage search
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Kotlin editor
participant Action as FindReferencesAction
participant Server as KotlinLanguageServer
participant Search as findUsagesAt
participant Files as Source files and live buffers
participant Client as IDELanguageClientImpl
Editor->>Action: execute find-references
Action->>Server: findReferences
Server->>Search: resolve usage locations
Search->>Files: inspect candidate content
Files-->>Search: matching locations
Search-->>Server: sorted locations
Server-->>Client: reference locations
Client->>Files: group and read result content
Client-->>Editor: display search results
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt (1)
76-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep cancellation and second preemption out of the generic getOrElse fallback.
runCatchingcatches everyThrowable; afterretryingOnPreemptionhandles oneAnalysisPreemptedException, a second preemption andCancellationExceptionfromcreateJobCancelChecker()still fall into.getOrElse { ... emptyList() }. Use typed handling that rethrowsCancellationExceptionand preserves the second preemption as an explicit result. Apply the same handling inImplementMembersAction.kt:76-102andOrganizeImportsAction.kt:63-82.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt` around lines 76 - 102, Replace the broad runCatching/getOrElse handling in ImplementMembersAction.kt lines 76-102 and OrganizeImportsAction.kt lines 63-82 with typed exception handling: rethrow CancellationException, preserve a second AnalysisPreemptedException as an explicit result, and use the generic fallback only for other failures. Keep retryingOnPreemption behavior unchanged.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (2)
app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt (1)
49-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the live-buffer overload.
The tests exercise the
Map<Int, String>overload andreadLines, but theresultsFor(file, locations, content: Content)path has separate out-of-range filtering. Add one JVM test that passes a smallContent("only")forStale.ktwith a stale hit (for examplefile, listOf(location(file, 1, 0, 1, 3)), Content("only")) and asserts the result is empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt` around lines 49 - 60, Add a JVM test for the Content overload of SearchResultGrouping.resultsFor, using Stale.kt, a stale hit on line 1, and Content("only"). Assert that the returned results are empty, covering out-of-range filtering for live-buffer content.lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse JUnit Jupiter in the Kotlin lsp tests.
These new test classes import JUnit 4 annotations. The test guideline requires JUnit Jupiter for
**/src/test/*.{kt,java}, so migrate the annotations and the@Afterlifecycle hook.KtLspTest,KtLspTestRule, and the Robolectric runner currently use JUnit 4-only APIs, so move those to JUnit/Jupiter-compatible fixtures or obtain an approved legacy exception before keeping these imports.
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt#L10: Replaceorg.junit.Testwithorg.junit.jupiter.api.Test.lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesLiveDocumentTest.kt#L12-L13: Replaceorg.junit.Afterwithorg.junit.jupiter.api.AfterEachand move theorg.junit.Testimport to Jupiter.- Also migrate
KtLspTest/KtLspTestRuleif new tests continue to depend on@Rule,@RunWith,TestRule,Statement, orTemporaryFolder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt` at line 10, Migrate the Kotlin LSP tests to JUnit Jupiter: in FindUsagesTest.kt replace the JUnit 4 Test import, and in FindUsagesLiveDocumentTest.kt replace Test and After with Jupiter Test and AfterEach. Update KtLspTest and KtLspTestRule to remove JUnit 4-only `@Rule`, `@RunWith`, TestRule, Statement, and TemporaryFolder dependencies, or obtain an approved legacy exception before retaining them.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0011-command-analysis-priority.md`:
- Around line 31-33: Update the priority-order code fence containing “INDEXING <
DIAGNOSTICS < COMMAND < INTERACTIVE” to use the text language label, preserving
its plain-text rendering and resolving the MD040 warning.
In `@docs/features/kotlin-find-usages.md`:
- Line 201: Specify the diagram’s fenced code block language as text by changing
the untyped Markdown fence associated with the pseudo-code diagram; leave the
diagram content unchanged.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt`:
- Around line 81-95: Update the retry flow around retryingOnPreemption and
findEnclosingClassOrObject so it does not reuse the captured offset after the
live PSI changes. Bind the request to the document revision and target marker,
then re-resolve the original target on retry or reject the result as stale
before postExec; add a regression test that inserts text before the caret during
the first attempt.
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt`:
- Line 426: Update the tests in AnalysisSerializationTest at the affected
methods to replace JUnit 4 `@Test`(timeout = ...) usage with JUnit Jupiter
`@Timeout`, including the required Jupiter import, while preserving each existing
timeout duration and test behavior.
- Around line 432-459: Update the first thread in the withAnalysisLock test to
poll holderChecker.abortIfCancelled() while waiting on release, allowing
same-priority preemption to be observed; then assert that the first command was
not preempted in addition to the existing entry assertions. Preserve the current
synchronization and release flow.
---
Outside diff comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt`:
- Around line 76-102: Replace the broad runCatching/getOrElse handling in
ImplementMembersAction.kt lines 76-102 and OrganizeImportsAction.kt lines 63-82
with typed exception handling: rethrow CancellationException, preserve a second
AnalysisPreemptedException as an explicit result, and use the generic fallback
only for other failures. Keep retryingOnPreemption behavior unchanged.
---
Nitpick comments:
In `@app/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.kt`:
- Around line 49-60: Add a JVM test for the Content overload of
SearchResultGrouping.resultsFor, using Stale.kt, a stale hit on line 1, and
Content("only"). Assert that the returned results are empty, covering
out-of-range filtering for live-buffer content.
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.kt`:
- Line 10: Migrate the Kotlin LSP tests to JUnit Jupiter: in FindUsagesTest.kt
replace the JUnit 4 Test import, and in FindUsagesLiveDocumentTest.kt replace
Test and After with Jupiter Test and AfterEach. Update KtLspTest and
KtLspTestRule to remove JUnit 4-only `@Rule`, `@RunWith`, TestRule, Statement, and
TemporaryFolder dependencies, or obtain an approved legacy exception before
retaining them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b6f968a-a074-4ba3-8584-c84c4c25b501
📒 Files selected for processing (26)
app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.javaapp/src/main/java/com/itsaky/androidide/lsp/SearchResultGrouping.ktapp/src/test/java/com/itsaky/androidide/lsp/SearchResultGroupingTest.ktdocs/adr/0010-navigation-resolves-via-analysis-api.mddocs/adr/0011-command-analysis-priority.mddocs/adr/README.mddocs/features/kotlin-find-usages.mddocs/features/kotlin-goto-definition.mdidetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/FindReferencesAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinition.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaret.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesLiveDocumentTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsagesTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/TargetAtCaretTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/ImplementMembersEndToEndTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/OrganizeImportsEndToEndTest.kt
| ``` | ||
| INDEXING < DIAGNOSTICS < COMMAND < INTERACTIVE | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Set a language on the priority-order code fence.
markdownlint reports MD040 for this unlabeled fence. Add text to retain plain-text rendering and clear the lint warning.
Proposed documentation fix
-```
+```text
INDEXING < DIAGNOSTICS < COMMAND < INTERACTIVE
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0011-command-analysis-priority.md` around lines 31 - 33, Update the
priority-order code fence containing “INDEXING < DIAGNOSTICS < COMMAND <
INTERACTIVE” to use the text language label, preserving its plain-text rendering
and resolving the MD040 warning.
Source: Linters/SAST tools
|
|
||
| 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. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the diagram fence language.
The untyped fence fails markdownlint rule MD040. Mark this pseudo-code block as text.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/features/kotlin-find-usages.md` at line 201, Specify the diagram’s
fenced code block language as text by changing the untyped Markdown fence
associated with the pseudo-code diagram; leave the diagram content unchanged.
Source: Linters/SAST tools
| 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) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not reuse an old caret offset after a live-PSI retry.
offset is captured before the command runs. A keystroke can preempt the command and change the file before that offset. The retry reads a new KtFile but resolves findEnclosingClassOrObject with the old integer. It can generate an insertion edit for a different class and apply it to the current document.
Bind the request to a document revision and target marker. If the revision changes, either resolve the original target safely or reject the stale result before postExec. Add a regression test that inserts text before the caret during the first attempt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt`
around lines 81 - 95, Update the retry flow around retryingOnPreemption and
findEnclosingClassOrObject so it does not reuse the captured offset after the
live PSI changes. Bind the request to the document revision and target marker,
then re-resolve the original target on retry or reject the result as stale
before postExec; add a regression test that inserts text before the caret during
the first attempt.
| * [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) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file='lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt'
# Inspect the current test annotation imports and timeout syntax.
sed -n '1,50p' "$test_file"
rg -n -C 2 '`@Test`\(timeout|`@Timeout`|import org\.junit' "$test_file"
# Verify whether the module enables JUnit Jupiter.
fd -a -t f --glob 'build.gradle*' . | while IFS= read -r build_file; do
rg -n -C 2 'useJUnitPlatform|junit-jupiter|junit:junit|junit-vintage' "$build_file" || true
doneRepository: appdevforall/CodeOnTheGo
Length of output: 5801
Use JUnit Jupiter timeouts in the new tests.
AnalysisSerializationTest still uses JUnit 4 @Test(timeout = ...). Since the module configures useJUnitPlatform(), switch these timeouts to the new Jupiter @Timeout annotation, or add a documented module-level legacy exception if the tests cannot use Jupiter yet.
Also applies to lines 426, 462, 504, 542, and 561.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt`
at line 426, Update the tests in AnalysisSerializationTest at the affected
methods to replace JUnit 4 `@Test`(timeout = ...) usage with JUnit Jupiter
`@Timeout`, including the required Jupiter import, while preserving each existing
timeout duration and test behavior.
Source: Coding guidelines
| 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() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this test observe same-priority preemption.
The second requester cannot enter before the first releases even when COMMAND.supersedesSamePriority is true. The first holder waits on release and never checks holderChecker, so an erroneous preemption is not observed and this test still passes.
Poll holderChecker.abortIfCancelled() while the holder waits, and assert that the first command was not preempted.
Proposed test correction
val holding = CountDownLatch(1)
val release = CountDownLatch(1)
+ val firstPreempted = AtomicBoolean(false)
val secondEntered = AtomicBoolean(false)
val first =
Thread {
- withAnalysisLock(AnalysisPriority.COMMAND, holderChecker) {
- holding.countDown()
- release.await()
+ try {
+ withAnalysisLock(AnalysisPriority.COMMAND, holderChecker) {
+ holding.countDown()
+ while (!release.await(10, TimeUnit.MILLISECONDS)) {
+ holderChecker.abortIfCancelled()
+ }
+ }
+ } catch (e: AnalysisPreemptedException) {
+ firstPreempted.set(true)
}
}
@@
+ assertThat(firstPreempted.get()).isFalse()
assertThat(enteredWhileHeld).isFalse()As per coding guidelines, “Use unit tests for non-UI logic, cover error and edge paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt`
around lines 432 - 459, Update the first thread in the withAnalysisLock test to
poll holderChecker.abortIfCancelled() while waiting on release, allowing
same-priority preemption to be observed; then assert that the first command was
not preempted in addition to the existing entry assertions. Preserve the current
synchronization and release flow.
Source: Coding guidelines
The direct- and refinement-dependents maps were built per module and merged
with `reduce { acc, value -> acc + value }`. `Map + Map` *replaces* a shared
dependency's dependent set rather than merging it, so a module used by more
than one other kept only the last of them.
Find usages reads that map for R4's public-visibility scope, so a public
declaration in a module with two consumers silently reported no usages in all
but one of them - and `reduce` additionally threw on an empty module list.
Accumulate into one map across all modules instead.
Five fixes to the search itself, all of them cases where it answered "no references" for a symbol with plenty, or did far more work than it needed to: - A candidate file preempted twice escaped `retryingOnPreemption` as a `CancellationException` and unwound the whole search, discarding every location already collected. Preemption is keystroke-driven work winning the lock, not the user cancelling, so it now costs that file like any other candidate failure. Genuine cancellation still propagates. - The declaration path came from the VFS alone, but the file the user is editing is a live `KtFile` whose `virtualFile` is a `LightVirtualFile`. A local or `private` target therefore had no path in the common case, fell through R4's single-file scope and searched the whole module graph for a variable that cannot leave one block. Derive it through `backingFilePath` first, as go-to-definition does, and fall back to module scope - never to the dependents graph - when there is still no path. - The text prefilter went through `StringSearch.containsWord`, which reads only a file's first megabyte (silently dropping usages below the mark), reads through one process-global `ByteBuffer` the Java server mutates from its own threads, and rethrows an unreadable file as a `RuntimeException`, which aborted the entire search. Replaced with `mentionsName`: whole file, line by line through `FileManager.getReader`, so an open file is still matched against its live buffer, and an unreadable one drops out with a log. - The prefilter had no cancel checker, so cancelling mid-scan let it read every remaining source file before the result was discarded. It now checks per file. - Every prefiltered candidate paid an analysis-lock acquisition, a FIR session and a match-set restore before the pure-PSI name filter could reject it. On a short, common name most candidates only mention it in a comment or a string literal. Run the name filter first and skip the session entirely when it finds nothing.
Moving the result-file reads off the main thread made the publish asynchronous, but nothing checked that the request still owned the panel. Two overlapping searches published in completion order, not request order, so a slow find-references that started first landed last and overwrote the newer search the user was looking at - and rows they never asked for navigated somewhere unrelated when tapped. `showLocations` now claims the panel with a request counter and captures `EditorViewModel.currentSearchGeneration`; the callback publishes only if both still hold. The counter catches a superseding find-references, the generation catches a text search publishing in between. Panel visibility moves next to the rows in `publishLocations` for the same reason: it was committed eagerly while the publish could be skipped entirely (activity recreated mid-read), which left the panel open with the "no results" placeholder hidden over the previous query's rows.
R4 gains the path-derivation fallback, R5 the new prefilter and its cancellation granularity, R6 the PSI-before-session ordering, R9 the twice-preempted candidate, R10 the panel's staleness guard and R12 the two failures now isolated per file. The flow diagram and the touched-components list follow. Also drops two claims that were never true of the shipped code: that locals skip the pointer machinery, and that the `StringSearch` limits were carried over unfixed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt (1)
112-119: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not catch JVM errors as recoverable failures.
The request handler at
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt:112catchesThrowable, soOutOfMemoryError,StackOverflowError, and linkage errors are logged as usage-search failures and can return an empty result. Narrow this to the recoverable analysis, PSI, and I/O exceptions, and letErrorpropagate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt` around lines 112 - 119, Narrow the catch in the usage-search request handler around the cancellation check and failure log to recoverable analysis, PSI, and I/O exceptions, allowing JVM Error types to propagate instead of returning an empty result. Apply the same exception-boundary correction to the sibling handling at lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt lines 441-445; both sites are within FindUsages request handling and must no longer catch Throwable.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt`:
- Around line 112-119: Narrow the catch in the usage-search request handler
around the cancellation check and failure log to recoverable analysis, PSI, and
I/O exceptions, allowing JVM Error types to propagate instead of returning an
empty result. Apply the same exception-boundary correction to the sibling
handling at
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt
lines 441-445; both sites are within FindUsages request handling and must no
longer catch Throwable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 141966a0-37c4-4cd3-b2c6-36000272faae
📒 Files selected for processing (4)
app/src/main/java/com/itsaky/androidide/lsp/IDELanguageClientImpl.javadocs/features/kotlin-find-usages.mdlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ModuleDependentsProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/FindUsages.kt
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Reviewed the diff plus the surrounding code it depends on (AnalysisScheduler/KtFileExts, IDEEditor.onFindReferencesResult, EditorViewModel.currentSearchGeneration, TaskExecutor, FileManager, AbstractSourceModule.computeFiles, KtSymbolIndex.getKtFile). I did not build or run the tests — everything below is by reading.
Assessment
Unusually strong. The ADR earns its place (the COMMAND-above-INTERACTIVE alternative is rejected for the right reason), per-candidate-file lock granularity is the load-bearing design choice and it's argued rather than asserted, and two genuine pre-existing bugs get fixed on the way: ModuleDependentsProvider's Map + Map merge was replacing a shared dependency's dependent set, and the search panel was doing main-thread I/O once per hit.
The showLocations staleness guard correctly needs both mechanisms it uses — the request counter for two showLocations racing, the ViewModel generation for a project-search publish landing in between. And the per-file ScheduledCancelChecker churn doesn't leak listeners on the request-scoped delegate, because withAnalysisLock removes its listener in finally.
Findings
1. Super-dispatched usages in a supertype's module are never searched
FindUsages.kt, R3/R4 interaction.
The match set walks up to workspace supers, so base.foo() counts as a usage of Derived.foo. But scopeOf derives the scope from the target's module plus its dependents only. Put Base in lib and Derived in app: a base.foo() call written in lib is a real usage that will never be looked at, because lib is a dependency of app, not a dependent.
The test that covers this case (a call dispatched through a workspace supertype is a usage of the override) puts both types in module app, so it can't catch it.
Either union the modules of every match-set member's declaration into the scope, or add it to Known Limitations — as written, the doc reads as though the super-walk is complete.
2. A double preemption during target resolution silently reports "no references"
FindUsages.kt, planAt / findUsagesAt.
planAt wraps in retryingOnPreemption. If the second attempt is also preempted, AnalysisPreemptedException escapes into findUsagesAt's catch (e: Throwable), where isAnalysisCancellation() is true (it is a CancellationException) and the result becomes ReferenceResult.empty(). The user's coroutine is still alive, so onFindReferencesResult flashes "No references found" for a symbol with plenty — precisely the failure mode ADR 0011 exists to eliminate.
usagesIn gets this right with a dedicated catch (e: AnalysisPreemptedException) ahead of the generic one; findUsagesAt should distinguish preemption from cancellation the same way. Rare (two preemptions inside one short plan phase), but the point of the new priority is that this class of silent wrong answer stops happening.
3. .java candidates are fully read by the prefilter, then guaranteed to be discarded
FindUsages.kt, candidateFiles.
computeFiles yields .kt and .java (AbstractSourceModule.kt:23). Every Java file in scope therefore gets a full line-by-line mentionsName read, and any that survives is dropped moments later by ktFileFor -> getKtFile -> isKotlinFile (KtSymbolIndex.kt:279).
Searching .java is explicitly out of scope, so filtering the extension before the read is free. On a Java-heavy workspace this is a large fraction of the prefilter's I/O spent on guaranteed-zero results — and the prefilter is the part of the search the user waits on.
4. namedReferences materialises every simple-name expression per candidate file
FindUsages.kt.
PsiTreeUtil.collectElementsOfType(ktFile, KtSimpleNameExpression::class.java).filter { … } allocates the full list before filtering, and the walk polls no cancel checker. A PsiRecursiveElementWalkingVisitor filtering on getReferencedName() inline drops the intermediate collection and gives cancellation a checkpoint inside the walk. Matters most in the case the prefilter is worst at: a short, common name in a large file.
Nits
kotlin-find-usages.mdR12 says "Genuine cancellation propagates rather than being reported as 'no references'", butfindUsagesAtconverts everyisAnalysisCancellation()into empty. Harmless in practice (a cancelled coroutine never reachesonFindReferencesResult), but the doc states the opposite of the code.@PublishedApionschedulerLoggeris unnecessary —retryingOnPreemptionisinternal inline, not public inline, so it may reference internal top-level declarations directly. The annotation only widens the property's bytecode visibility.SearchResultGrouping.resultsFor:val startLine = lines[range.start.line]names aStringas though it were a line number, andmatchedTextimmediately looks the same key up again.lineText, passed in, reads better.- The design section says candidate files are selected "with no lock at all";
computeFilestakesproject.readper file (AbstractSourceModule.kt:25). The real claim — no lock held across the search — is intact; the wording isn't.
Test coverage
45 new/changed tests, testing the right things: the visibility ladder is asserted on SearchPlan.scope rather than inferred from results, the toString-override case pins the workspace-boundary cutoff, FindUsagesLiveDocumentTest isolates the enableParserEventSystem requirement, and ReferenceAtCaretTest is deliberately left untouched as the proof that loosening navigableLeafAt to internal changed nothing.
SearchResultGroupingTest avoids Content entirely so it stays a plain JVM test — worth noting the Content overload is consequently unexercised, as is the showLocations staleness guard.
Gap tied to finding 1: a cross-module super/override fixture (Base in lib, Derived in app) would have surfaced it.
Security / conventions
Nothing security-relevant — no new dependencies, no network, no new permissions, all I/O inside the workspace. Tabs, comment discipline, and ASCII-in-code all conform; the commits are ordered docs -> priority -> helper -> feature -> menu -> panel as the description claims.
Recommendation
Good to merge once finding 2 (small, contained) and finding 3 (one-line filter) are addressed. Finding 1 needs either a fix or a line in Known Limitations before merge; finding 4 is fine as a follow-up.
🤖 Review generated with Claude Code
2 participants
Jira: ADFA-4824
Fills in
KotlinLanguageServer.findReferences, which until now answered empty. From a Kotlin declaration - or a reference to one - list every usage in the workspace across all three scopes: same file, another file in the same module, another module.Everything downstream (
ReferenceResult,IDEEditor.onFindReferencesResult, the search-results panel) already existed for the Java server and is reused.Full write-up:
docs/features/kotlin-find-usages.md.How it works
TargetAtCaretmaps the caret to the declaration to search for, either directly (caret on the declaration's own name) or by resolving the reference under it. Go-to-def'sreferenceAtCaretcan't be reused verbatim: it deliberately returns null on a declaration's own name, which is exactly where find usages is invoked from.local/privatesearches the containing file,internalits module,public/protectedthe module plus its transitive dependents. The ticket's three scopes fall out of one code path.Base.foocounts as a usage ofDerived.foo) plus a classifier's constructors (soFoo()counts as a usage ofclass Foo). Library supers are excluded, or an overriddentoStringwould match every.toString()in the workspace.KaSymbolPointerrestored once per candidate session.KaSymbolis session-scoped, and a PSI or file+offset key breaks when the target's own file has unsaved edits.ReferencesSearch,PsiSearchHelperor word index inanalysis-api-standalone-embeddable-for-ide.Two changes outside
lsp/kotlinAnalysisPriority.COMMAND(ADR 0011), betweenDIAGNOSTICSandINTERACTIVE, withsupersedesSamePriority = false.INTERACTIVEmeans "a newer request makes me stale, discard my work", which is false for a user-invoked command. TodayOrganizeImportsActionandImplementMembersActioncan be silently discarded by a completion request with no retry; go-to-def worked around it with a one-shot retry. All three migrate to the new priority.IDELanguageClientImpl.showLocationsread each result file in full, once per hit, on the main thread. Now one grouped streaming pass per file, off the main thread, retaining nothing. This also fixes Java find-references.Known limitations (documented, to be ticketed separately)
src/test/**andsrc/androidTest/**are invisible -AndroidModule.getSourceDirectories()returnsmainSourceSetonly, so test sources aren't LSP content roots for any feature.DirectInheritorsProvider.computeIndex()rebuilds its whole index per call.a + b,by, destructuring) are valid entry points but are not reported as results.Tests
:lsp:kotlin:testV7DebugUnitTestand:app:testV7DebugUnitTest:TargetAtCaretTest(13) - PSI only, no session; includes the case where the caretreferenceAtCaretrejects still yields a target.FindUsagesTest(20) - thelib+app(dependsOn = lib)fixture from ADFA-4823: three resolution scopes, each row of the visibility ladder, super-walk and workspace-boundary cutoff, constructor expansion, imports, a Java-source target, a same-named decoy, ordering, property reads/writes, a stdlib reference, a caret naming nothing, a pre-cancelled request.FindUsagesLiveDocumentTest(2) - a usage only in an unsaved buffer is found; one deleted in the buffer but still on disk is not.AnalysisSerializationTest(+5) -COMMAND's ordering properties andretryingOnPreemption's one-retry contract.SearchResultGroupingTest(10,:app) - multi-line hits, a hit past EOF, a column past its line's end, an unreadable file, several hits in one file from one read.ReferenceAtCaretTestkept as-is, as proof that loosening visibility changed no behaviour.Not unit-testable, so covered by the "Steps to QA" on the ticket: 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.
Review
Seven commits, reviewable in order - docs/ADR first, then the
COMMANDpriority,TargetAtCaret,FindUsages, the menu item, and theshowLocationsrewrite.