ADFA-4610: Fix Add-import action never appearing on unresolved references#1544
Closed
itsaky-adfa wants to merge 3 commits into
Closed
ADFA-4610: Fix Add-import action never appearing on unresolved references#1544itsaky-adfa wants to merge 3 commits into
itsaky-adfa wants to merge 3 commits into
Conversation
…nces AddImportAction.prepare() gated visibility on findSymbolBySimpleName(name, 0), but the helper ended with Sequence.take(limit) and take(0) returns empty, so the action was invisible for every unresolved-reference diagnostic - dead since ADFA-3754. Treat limit <= 0 as unbounded, honoring the documented ReadableIndex.query contract the helper was violating. Also extract hasImportableClassifier and a non-suspend computeImportCandidates (runCatching, KtFile fetched before project.read) for testability and crash-safety, and drop the no-op CMD_FORMAT_CODE. Adds AddImportActionTest (visibility-gate regression) and EditExtsTest.
Replace the SQLite-backed visibility gate with an in-memory check on the unresolved-reference marker. prepare() runs synchronously on the UI thread during menu build, so resolving against the symbol index there was main-thread disk I/O; that resolution now happens only in the background execAction.
![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.
itsaky-adfa
changed the title
Contributor
📝 WalkthroughWalkthroughThe Kotlin LSP add-import action now resolves classifier candidates into fully-qualified-name edit maps, handles unbounded symbol-index lookups, emits no-op commands, and includes coverage for candidate filtering and sorted import insertion. ChangesAdd-import flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LSP
participant AddImportAction
participant KtSymbolIndex
participant KtFile
LSP->>AddImportAction: prepare unresolved reference
AddImportAction->>KtSymbolIndex: find importable classifiers
KtSymbolIndex-->>AddImportAction: matching symbols
AddImportAction->>KtFile: compute import TextEdits
KtFile-->>AddImportAction: edits keyed by fqName
AddImportAction-->>LSP: CodeActionItems with no-op commands
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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.
Actionable comments posted: 1
🤖 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
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt`:
- Around line 87-113: Replace the broad runCatching/getOrElse handling in
computeImportCandidates with a targeted catch for the specific expected
exception, such as IllegalArgumentException, while allowing unexpected
exceptions to propagate. Update the function KDoc to describe only the narrowed
recovery behavior and remove claims that all pipeline failures are converted to
an empty map.
🪄 Autofix (Beta)
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
Run ID: 6c8aaad1-a7b5-46fe-9e97-adc2c3cf5a7a
📒 Files selected for processing (4)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/EditExtsTest.kt
Comment on lines
+87
to
+113
| /** | ||
| * Computes, for the unresolved [reference] in the file at [nioPath] within [env], a map from | ||
| * each importable classifier's fully-qualified name to the edits that add its import in sorted | ||
| * position. The [org.jetbrains.kotlin.psi.KtFile] is fetched BEFORE entering [read] (deadlock | ||
| * rule: never block on `getCurrentKtFile(...).get()` inside `project.read`). Keying by FQN | ||
| * collapses the duplicate a symbol picks up from being present in both the source and library | ||
| * indexes. Returns an empty map when there is nothing to import *and* whenever anything in this | ||
| * pipeline throws: the action framework only catches [IllegalArgumentException] and this runs on | ||
| * a coroutine scope with no exception handler, so an uncaught throw here would crash the app. | ||
| */ | ||
| internal fun computeImportCandidates( | ||
| env: AbstractCompilationEnvironment, | ||
| nioPath: Path, | ||
| reference: String, | ||
| ): Map<String, List<TextEdit>> = | ||
| runCatching { | ||
| val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return emptyMap() | ||
| env.project.read { | ||
| env.ktSymbolIndex | ||
| .findSymbolBySimpleName(reference, limit = 0) | ||
| .filter { it.kind.isClassifier } | ||
| .associate { symbol -> symbol.fqName to insertImport(ktFile, symbol.fqName) } | ||
| } | ||
| }.getOrElse { e -> | ||
| logger.warn("Failed to compute import candidates for '{}'", reference, e) | ||
| emptyMap() | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the broad catch-all with narrow exception handling.
The use of runCatching handles all Throwable instances, intentionally suppressing crashes for any unexpected errors. Based on learnings, prefer narrow exception handling that catches only the specific exception type reported in crashes (such as IllegalArgumentException) instead of a broad catch-all. This preserves fail-fast behavior during development. Uncaught exceptions should be allowed to crash the app so they can be exposed and addressed.
🛠️ Proposed fix to apply narrow exception handling and update KDoc
/**
* Computes, for the unresolved [reference] in the file at [nioPath] within [env], a map from
* each importable classifier's fully-qualified name to the edits that add its import in sorted
* position. The [org.jetbrains.kotlin.psi.KtFile] is fetched BEFORE entering [read] (deadlock
* rule: never block on `getCurrentKtFile(...).get()` inside `project.read`). Keying by FQN
* collapses the duplicate a symbol picks up from being present in both the source and library
- * indexes. Returns an empty map when there is nothing to import *and* whenever anything in this
- * pipeline throws: the action framework only catches [IllegalArgumentException] and this runs on
- * a coroutine scope with no exception handler, so an uncaught throw here would crash the app.
+ * indexes. Returns an empty map when there is nothing to import, or if an
+ * [IllegalArgumentException] is thrown. Uncaught exceptions are intentionally allowed
+ * to crash the app to enforce fail-fast behavior during development.
*/
internal fun computeImportCandidates(
env: AbstractCompilationEnvironment,
nioPath: Path,
reference: String,
): Map<String, List<TextEdit>> =
- runCatching {
+ try {
val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return emptyMap()
env.project.read {
env.ktSymbolIndex
.findSymbolBySimpleName(reference, limit = 0)
.filter { it.kind.isClassifier }
.associate { symbol -> symbol.fqName to insertImport(ktFile, symbol.fqName) }
}
- }.getOrElse { e ->
+ } catch (e: IllegalArgumentException) {
logger.warn("Failed to compute import candidates for '{}'", reference, e)
emptyMap()
}📝 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.
Suggested change
| /** | |
| * Computes, for the unresolved [reference] in the file at [nioPath] within [env], a map from | |
| * each importable classifier's fully-qualified name to the edits that add its import in sorted | |
| * position. The [org.jetbrains.kotlin.psi.KtFile] is fetched BEFORE entering [read] (deadlock | |
| * rule: never block on `getCurrentKtFile(...).get()` inside `project.read`). Keying by FQN | |
| * collapses the duplicate a symbol picks up from being present in both the source and library | |
| * indexes. Returns an empty map when there is nothing to import *and* whenever anything in this | |
| * pipeline throws: the action framework only catches [IllegalArgumentException] and this runs on | |
| * a coroutine scope with no exception handler, so an uncaught throw here would crash the app. | |
| */ | |
| internal fun computeImportCandidates( | |
| env: AbstractCompilationEnvironment, | |
| nioPath: Path, | |
| reference: String, | |
| ): Map<String, List<TextEdit>> = | |
| runCatching { | |
| val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return emptyMap() | |
| env.project.read { | |
| env.ktSymbolIndex | |
| .findSymbolBySimpleName(reference, limit = 0) | |
| .filter { it.kind.isClassifier } | |
| .associate { symbol -> symbol.fqName to insertImport(ktFile, symbol.fqName) } | |
| } | |
| }.getOrElse { e -> | |
| logger.warn("Failed to compute import candidates for '{}'", reference, e) | |
| emptyMap() | |
| } | |
| /** | |
| * Computes, for the unresolved [reference] in the file at [nioPath] within [env], a map from | |
| * each importable classifier's fully-qualified name to the edits that add its import in sorted | |
| * position. The [org.jetbrains.kotlin.psi.KtFile] is fetched BEFORE entering [read] (deadlock | |
| * rule: never block on `getCurrentKtFile(...).get()` inside `project.read`). Keying by FQN | |
| * collapses the duplicate a symbol picks up from being present in both the source and library | |
| * indexes. Returns an empty map when there is nothing to import, or if an | |
| * [IllegalArgumentException] is thrown. Uncaught exceptions are intentionally allowed | |
| * to crash the app to enforce fail-fast behavior during development. | |
| */ | |
| internal fun computeImportCandidates( | |
| env: AbstractCompilationEnvironment, | |
| nioPath: Path, | |
| reference: String, | |
| ): Map<String, List<TextEdit>> = | |
| try { | |
| val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return emptyMap() | |
| env.project.read { | |
| env.ktSymbolIndex | |
| .findSymbolBySimpleName(reference, limit = 0) | |
| .filter { it.kind.isClassifier } | |
| .associate { symbol -> symbol.fqName to insertImport(ktFile, symbol.fqName) } | |
| } | |
| } catch (e: IllegalArgumentException) { | |
| logger.warn("Failed to compute import candidates for '{}'", reference, e) | |
| emptyMap() | |
| } |
🤖 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/AddImportAction.kt`
around lines 87 - 113, Replace the broad runCatching/getOrElse handling in
computeImportCandidates with a targeted catch for the specific expected
exception, such as IllegalArgumentException, while allowing unexpected
exceptions to propagate. Update the function KDoc to describe only the narrowed
recovery behavior and remove claims that all pipeline failures are converted to
an empty map.
Source: Learnings
Contributor
Author
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
1 participant
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.
Jira: ADFA-4610
What & why
The "Add import" code action never appeared on an unresolved-reference diagnostic - dead since ADFA-3754. Root cause:
KtSymbolIndex.findSymbolBySimpleNameended withSequence.take(limit)and its only caller passedlimit = 0, sotake(0)returned empty andprepare()always went invisible.This branch fixes that and hardens the action:
limit <= 0as unbounded infindSymbolBySimpleName, honoring the documentedReadableIndex.querycontract the helper was violating. ExtractcomputeImportCandidates(non-suspend,runCatching,KtFilefetched beforeproject.read) for testability + crash-safety; drop the no-opCMD_FORMAT_CODE.prepare()-prepare()runs synchronously on the UI thread during menu build and previously resolved against the SQLite index there. It now decides visibility from the in-memoryKotlinDiagnosticExtra.unresolvedReferencemarker only (optimistic visibility); index resolution happens solely in the backgroundexecAction.flashError).Tests
:lsp:kotlin:testV7DebugUnitTest --tests AddImportActionTest5/5 green (incl. thetake(0)-unbounded regression guard);EditExtsTestcovers sorted-insert/dedup;:lsp:kotlin:assembleV8Debugbuilds. Spotless can't run in the worktree (jgitratchetFrombug); formatting hand-verified.Merge gate (manual QA)
On-device: "No imports found" flashbar on a typo; single-candidate auto-apply vs multi-candidate picker on a resolvable reference; no main-thread jank.
Follow-ups (out of scope)
insertImportdoes not dedup a concrete import already covered by a wildcard (no impact today - only concrete FQNs are inserted).