ADFA-4823: Kotlin go-to-definition in the K2 LSP - #1597
Merged
Conversation
The :lsp:kotlin suite could not complete: it OOMed part-way through, and the OOM then deadlocked Gradle's TestWorker instead of failing the task. Two causes, two fixes: - KtLspTestRule never disposed the test environment. The TODO'd env.close() failed because disposal needs an IntelliJ write action, which our own project.write lock does not supply. Every test therefore leaked a whole KotlinCoreApplicationEnvironment (refcounted, process-wide static). - The test-worker heap was Gradle's 512m default, too small for the Robolectric + Kotlin Analysis API suites regardless of the leak. Also stops muting test failures everywhere: ignoreFailures now applies only to the modules whose tests are known to fail, so a green build means the suite actually passed. :lsp:kotlin is not in that set. Picked up from the stage worktree, where these fixes were written but not yet committed; carried here so this branch's tests can gate on a real result. Reconciled onto the current KtLspTestRule, which this branch had already reworked for multi-module specs. :lsp:kotlin now reports 184 tests, 0 failures, 0 skipped.
…exed Workspace .java sources could not be resolved from Kotlin in the standalone Analysis API environment: JavaPsiFacade.findClass returned null and a type reference to a workspace Java class resolved to nothing. Cause: isSourceModule was `this is KtSourceModule`, a check against one concrete class. Any other source-module implementation was therefore classified as a library, so in AbstractCompilationEnvironment.initialize the javaSourceRoots partition came out empty and javaFileManager/corePackageIndex never got the source root on their classpath. The same modules were also fed into libraryRoots as BINARY roots. Ask the Analysis API's own question instead (KaSourceModule). Both KtSourceModule and TestKtSourceModule implement it; KtLibraryModule implements KaLibraryModule, so library classification is unchanged. This is the only nominal check of its kind in the tree. Also add KtLspTestEnvironment.createFile, returning PsiFile: createSourceFile narrows to KtFile and threw for .java sources, so no test could create one. createSourceFile now delegates to it. Together these make Kotlin-to-Java navigation testable, which the go-to-definition work needs for acceptance criterion 5. :lsp:kotlin reports 212 tests, 0 failures, 0 skipped.
The go-to-definition work lands with its governing decision and requirements only in a working tree, so neither is reviewable alongside the code. Add ADR 0010 (navigation resolves via the Analysis API and PSI, never the symbol indexes - the indexes store no declaration offsets and answer by name rather than by resolution) and its row in the ADR index, plus the feature's requirements and acceptance criteria. R10 is corrected while committing it: prepare() adds no lock, index or analysis work, but it is not filesystem-free, because BaseKotlinCodeAction.prepare calls DocumentUtils.isKotlinFile, which stats the file. That is pre-existing and shared by every Java and Kotlin code action; the doc now says so rather than claiming an invariant that does not hold.
locationOfPsi derived a target's path from its virtualFile, which is non-physical (protocol "mock" in production, null under Robolectric) for any declaration living in the file the user has open - KtSymbolIndex.refreshToCurrent builds that live KtFile via parser.createFile and never gives it a real on-disk virtualFile. Every same-file (and any other open-file) declaration was silently dropped. Derive the path from the live KtFile's backingFilePath first, falling back to the VFS (still gated on protocol == "file") only for declarations reached outside the live-document cache. Binaries stay excluded on the rawReferenceLocation path too: backingFilePath is only ever set in KtSymbolIndex.refreshToCurrent for workspace sources opened as active documents, so a library/binary KtFile never carries it and falls through to the unchanged, still-guarded VFS branch. Add a regression test exercising the live-document path end to end through findDefinitionAt (open the file, then request its own declaration). Making it faithful required KtLspTestEnvironment's in-memory KtFiles to be physical the way production's are (enableParserEventSystem), which the test harness had hardcoded off for every test; expose it as an opt-in override, default unchanged, so only this test's live-document scenario is affected.
…tale comments findDefinitionAt now retries once, with a fresh ScheduledCancelChecker, when the analysis is preempted by a concurrent higher-or-equal-priority request (AnalysisPreemptedException). INTERACTIVE requests supersede same-priority ones, so a completion or signature-help lookup could preempt an in-flight definition request while it is still alive, surfacing "Definition not found" for a reference that resolves fine. A genuine cancellation still returns empty immediately, without retrying. GoToDefinitionTest's multi-candidate test used a resolvable overloaded call, which resolves to exactly one symbol - distinctBy/sortedWith in definitionLocations went untested. Replaced it with a for-loop over a workspace-defined iterator (hasNext/next/iterator all workspace sources, so none get filtered as stdlib), asserting three distinct, ordered locations. Comment fixes: - GoToDefinitionAction: corrected the claim that nothing in prepare() touches disk - the base class's isKotlinFile does stat the file, a pre-existing issue shared by every Kotlin/Java code action. - KtModule.isSourceModule: the KDoc implied a shipped classification bug; in production KtSourceModule is the only KaSourceModule, so the nominal and structural checks always agreed there. The fix only changes TestKtSourceModule's classification and hardens the predicate for future source-module implementations. - ReferenceAtCaret.MAX_CLIMB: the comment credited the cap with stopping a caret on a declaration's name from climbing into an enclosing call, but that's several levels beyond the cap - the intervening ancestors are simply not resolvable. The cap's actual job is bounding the walk to what the reference kinds need. - findDefinitionAt KDoc: noted why its context parameter is the abstract environment rather than the concrete one (so the test environment can drive it). - GoToDefinitionTest's property-delegate test: added the same tolerance comment as the for-loop test below it, explaining why extra candidates (setValue/provideDelegate) are acceptable.
`jacocoAggregateReport` dependsOn every subproject's `testV8DebugUnitTest`, and `sonarqube` dependsOn that report, so a single failing test task skipped both - even under `--continue`. The analyze workflow then uploaded an empty `jacoco-report` artifact and SonarCloud received no analysis for the commit. Gate `ignoreFailures` on whether the analysis chain is what was requested, so the coverage/Sonar run always produces a report while every ordinary build still fails on a failing test. That also removes the reason for the per-module mute list: it existed only to keep those modules from breaking the analysis run, which the task-name check now handles, so their suites gate the build again like everything else.
A test worker's working dir defaults to the module directory, so the new -XX:+HeapDumpOnOutOfMemoryError had nowhere to put a dump but the source tree - up to maxHeapSize (1g) per OOM, untracked and not covered by .gitignore. On CI several such dumps can exhaust the runner disk and turn one test OOM into an unrelated "No space left on device". Point -XX:HeapDumpPath at build/test-heapdumps/<task>.hprof, created in doFirst because the JVM will not create a missing dump directory itself.
…es, preemption Three ways the lookup failed where it should have answered: - `DocumentUtils.isKotlinFile` accepts `.kts`, and the editor attaches the Kotlin server to build scripts, so a Go-to-definition there reached `CompilationKind.Script` and let `UnsupportedOperationException` escape into the editor as an error flash. The path-based `compilationEnvironmentFor` is already nullable; return null for a script instead of throwing, which also fixes the same escape from `complete`, `signatureHelp` and the file-event handlers. - `mainReference` is typed nullable but on a `KtReferenceExpression` it is `references.firstIsInstance()`, which throws when no `KtReference` is contributed. Reading it last and guarded keeps a throw from aborting the whole ancestor climb before the convention-host checks run. - The preemption retry reused the `KtFile` captured before the preemption. What preempted the lookup also refreshed the live PSI and unregistered that file, so the retry analyzed a dangling instance and reported "definition not found" for a reference that resolves. Await the current file per attempt. Also drop the feature doc's claim that `findDefinition` is still a stub.
…init throw Disposal ran bare in the rule's `finally`, so a throw from `close()` - a timed-out index drain, a Disposer error - replaced the exception already in flight and JUnit reported the fixture instead of the assertion that actually broke. Attach disposal errors as suppressed on the test's own failure, and only rethrow them when the test passed. The environment was also assigned to the field only after its constructor returned, so a throw from `initialize()` - and the module specs added several new ones - left `::env.isInitialized` false, close() unreachable, and the refcounted, process-wide `KotlinCoreApplicationEnvironment` leaked for the rest of the suite: one misspelled `dependsOn` would reproduce the very OOM this fixture work fixed. Release it from the constructor, where the half-built instance is still reachable.
![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.
Contributor
📝 Walkthrough
WalkthroughAdds Kotlin go-to-definition through Analysis API and PSI, exposes it as a Kotlin editor action, documents its behavior, expands LSP fixtures for multi-module testing, and adds broad navigation coverage. Gradle test execution also gains conditional failure handling and OOM heap-dump diagnostics. ChangesKotlin go-to-definition
Test runtime diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant KotlinLanguageServer
participant CompilationEnvironment
participant AnalysisAPI
participant WorkspaceFiles
Editor->>KotlinLanguageServer: textDocument/definition
KotlinLanguageServer->>CompilationEnvironment: findDefinitionAt
CompilationEnvironment->>AnalysisAPI: resolve caret reference
AnalysisAPI->>WorkspaceFiles: map declaration to location
WorkspaceFiles-->>Editor: return definition locations
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/InterModuleResolutionTest.kt (1)
1-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffJUnit4
@Testused instead of JUnit Jupiter in this new test.This new test extends the JUnit4
RunWith(RobolectricTestRunner::class)-basedKtLspTest/KtLspTestRulefixture, so it can't independently adopt JUnit Jupiter without migrating the whole shared fixture stack (used by the rest of the navigation test suite). Worth tracking as a follow-up if/when the fixture itself is modernized, rather than blocking this PR.As per coding guidelines,
**/src/test/**/*.{kt,java}should "Use JUnit Jupiter, Truth, MockK for new tests, Mockito-Kotlin where legacy conventions require it, and Robolectric for framework-dependent JVM tests."🤖 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/fixtures/InterModuleResolutionTest.kt` around lines 1 - 52, Keep InterModuleResolutionTest on JUnit4 using org.junit.Test because KtLspTest and KtLspTestRule depend on the shared Robolectric JUnit4 runner; do not migrate this test independently to JUnit Jupiter. Track fixture-wide JUnit migration separately rather than changing the test framework here.Source: Coding guidelines
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.kt (1)
94-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
runCatchinginisResolvable().
runCatching { mainReference != null }catches anyThrowable, so unrelated PSI/IO exceptions orErrors can be reported as plain “not resolvable” without visibility. Catch only the expectedfirstIsInstance()failure case instead of wrapping the whole access.♻️ Proposed narrowing
private fun KtElement.isResolvable(): Boolean = this is KtCallExpression || this is KtArrayAccessExpression || this is KtPropertyDelegate || this is KtForExpression || - runCatching { mainReference != null }.getOrDefault(false) + try { + mainReference != null + } catch (e: NoSuchElementException) { + false + }🤖 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/ReferenceAtCaret.kt` around lines 94 - 108, In KtElement.isResolvable(), narrow the exception handling around mainReference access so it catches only the expected missing-reference exception thrown by firstIsInstance(), rather than using runCatching, which also suppresses unrelated Throwables and Errors. Preserve the existing false result for that expected failure and let other failures propagate.Source: 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.
Nitpick comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaret.kt`:
- Around line 94-108: In KtElement.isResolvable(), narrow the exception handling
around mainReference access so it catches only the expected missing-reference
exception thrown by firstIsInstance(), rather than using runCatching, which also
suppresses unrelated Throwables and Errors. Preserve the existing false result
for that expected failure and let other failures propagate.
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/InterModuleResolutionTest.kt`:
- Around line 1-52: Keep InterModuleResolutionTest on JUnit4 using
org.junit.Test because KtLspTest and KtLspTestRule depend on the shared
Robolectric JUnit4 runner; do not migrate this test independently to JUnit
Jupiter. Track fixture-wide JUnit migration separately rather than changing the
test framework here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f934f26b-9a8a-42e6-ba26-764d536a0ec8
📒 Files selected for processing (21)
build.gradle.ktsdocs/adr/0010-navigation-resolves-via-analysis-api.mddocs/adr/README.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/GoToDefinitionAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtModule.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/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/InterModuleResolutionTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestEnvironment.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestRule.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/TestSourceModuleSpec.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/FindDefinitionRequestTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/GoToDefinitionTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/navigation/ReferenceAtCaretTest.kt
jatezzz
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Go-to-definition in the K2 Kotlin LSP: jump from a Kotlin reference to its declaration, same-file, inter-file and inter-module.
Jira: ADFA-4823
Requirements and scope:
docs/features/kotlin-goto-definition.md. Structural decision: ADR 0010 - navigation resolves through the Analysis API, not the symbol index.What it does
editor.codeactions.kotlin.gotodef). The tooltip row lives in the tooltips database, not this repo - hand-off item.a + b,a[i],by lazy, destructuring,forloops), plus labels and KDoc links. Workspace.javaandbuild/generated/**targets included.Binary symbols (stdlib, framework, jars) are out of scope - no decompiler on device, so those report "Definition not found". Find-usages is ADFA-4824 and reuses the caret helper unchanged.
Caret mapping is split from resolution:
ReferenceAtCaret.ktis pure PSI, so the caret rules test without an analysis session and ADFA-4824 imports it as-is.GoToDefinition.ktowns resolution, filtering, ranges and cancellation.Review order
Arranged for review-by-commit. The last four came out of a code review of the rest:
d3f15d2ignoreFailuresnow gates on whether the coverage/Sonar chain was requested. One failing test used to skipjacocoAggregateReportandsonarqubeentirely, so the nightly analysis uploaded an empty artifact.53b02df-XX:HeapDumpPathunderbuild/test-heapdumps/; unpathed, a worker OOM dropped up to 1g into the source tree.bc72b0a.ktsletUnsupportedOperationExceptionreach the editor;mainReferenceis typed nullable but throws on aKtReferenceExpressionwith no contributed reference; the preemption retry reused aKtFilethe refresh had unregistered.3251f2cKotlinCoreApplicationEnvironment.d3f15d2drops the per-module test mute list - it only existed to protect the analysis run, which the task-name check now handles, so:gradle-plugin,:lsp:javaand:termux:termux-appgate ordinary builds again and their existing failures will surface.Testing
flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest- passing.ReferenceAtCaretTest(caret rules),GoToDefinitionTest(resolution rows, all three scopes, dedup, ranges, cancellation),FindDefinitionRequestTest(request path),InterModuleResolutionTest.Not unit-tested: the preemption retry (needs a second concurrent interactive request the fixture can't schedule) and the action's
prepare()path, consistent with the other Kotlin actions. Both covered by on-device QA - see "Steps to QA" on the ticket.Known gaps
KtSymbolIndex.getKtFilefalls through to the on-disk PSI (it runs underproject.readand must not block on a refresh needingproject.write), so the caret can land off by the lines inserted above. Fixing it means re-locating the declaration in the live PSI.[or(resolves the convention host, not the identifier before it - deliberate and asserted, butlist[0]reports nothing whengetis in a jar.DocumentUtils.isKotlinFilestill stats the file on the UI thread in every code action'sprepare(). Pre-existing, shared with Java; worth its own ticket.