diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt index 9896343851..06eac6c86b 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt @@ -7,7 +7,9 @@ import com.itsaky.androidide.actions.newDialogBuilder import com.itsaky.androidide.actions.require import com.itsaky.androidide.actions.requireFile import com.itsaky.androidide.idetooltips.TooltipTag +import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.index.findSymbolBySimpleName +import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.diagnostic.KotlinDiagnosticExtra import com.itsaky.androidide.lsp.kotlin.utils.insertImport import com.itsaky.androidide.lsp.models.CodeActionItem @@ -17,8 +19,9 @@ import com.itsaky.androidide.lsp.models.DiagnosticItem import com.itsaky.androidide.lsp.models.DocumentChange import com.itsaky.androidide.lsp.models.TextEdit import com.itsaky.androidide.resources.R -import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol +import com.itsaky.androidide.utils.flashError import org.slf4j.LoggerFactory +import java.nio.file.Path class AddImportAction : BaseKotlinCodeAction() { override var titleTextRes: Int = R.string.action_import_classes @@ -39,51 +42,54 @@ class AddImportAction : BaseKotlinCodeAction() { return } + // Optimistic visibility: decide from the in-memory unresolved-reference marker only. The + // importable-classifier resolution runs in the background execAction; doing it here would be + // main-thread SQLite I/O, because fillMenu() calls prepare() synchronously on the UI thread. val extra = data.require().extra as? KotlinDiagnosticExtra - if (extra == null) { - markInvisible() - return - } - - val reference = extra.unresolvedReference - if (reference == null) { - markInvisible() - return - } - - val env = extra.compilationEnv - val hasImportableSymbols = - env.ktSymbolIndex - .findSymbolBySimpleName(reference, limit = 0) - .any { it.kind.isClassifier } - - if (!hasImportableSymbols) { + if (extra?.unresolvedReference == null) { markInvisible() return } } - override suspend fun execAction(data: ActionData): Map> { + override suspend fun execAction(data: ActionData): Map> { val (reference, env) = data.require().extra as? KotlinDiagnosticExtra ?: return emptyMap() if (reference == null) return emptyMap() - val file = data.requireFile() - val nioPath = file.toPath() - val ktFile = - env.ktSymbolIndex - .getCurrentKtFile(nioPath) - .get() - ?: return emptyMap() - - return env.ktSymbolIndex - .findSymbolBySimpleName(reference, limit = 0) - .filter { it.kind.isClassifier } - .associateWith { symbol -> insertImport(ktFile, symbol.fqName) } + return computeImportCandidates(env, data.requireFile().toPath(), reference) } + /** + * 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> = + 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() + } + override fun postExec( data: ActionData, result: Any, @@ -95,10 +101,11 @@ class AddImportAction : BaseKotlinCodeAction() { } @Suppress("UNCHECKED_CAST") - result as Map> + result as Map> if (result.isEmpty()) { logger.warn("No classifiers to import.") + flashError(R.string.msg_no_imports_found) return } @@ -113,19 +120,27 @@ class AddImportAction : BaseKotlinCodeAction() { val nioPath = file.toPath() val actions = result - .map { (symbol, edits) -> + .map { (fqName, edits) -> CodeActionItem( - title = symbol.fqName, + title = fqName, changes = listOf(DocumentChange(file = nioPath, edits = edits)), kind = CodeActionKind.QuickFix, - command = Command.CMD_FORMAT_CODE, + // Imports are column-0 text; emit final text ourselves. CMD_FORMAT_CODE is a + // no-op for Kotlin, so use an empty (no-op) post-action command. + command = Command("", ""), ) } when (actions.size) { - 0 -> logger.error("No code actions found. Cannot completion action.") - 1 -> client.performCodeAction(actions[0]) - else -> + 0 -> { + logger.error("No code actions found. Cannot completion action.") + } + + 1 -> { + client.performCodeAction(actions[0]) + } + + else -> { newDialogBuilder(data) .setTitle(label) .setItems(actions.map { it.title }.toTypedArray()) { dialog, which -> @@ -135,6 +150,7 @@ class AddImportAction : BaseKotlinCodeAction() { logger.error("Index $which is out of bounds for actions of size ${actions.size}") } }.show() + } } } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt index a89a19e7e8..2b36a45cd7 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt @@ -336,8 +336,16 @@ internal fun KtSymbolIndex.filesForPackage(packageFqn: String) = fileIndex.getFi internal fun KtSymbolIndex.subpackageNames(packageFqn: String) = fileIndex.getSubpackageNames(packageFqn) +/** + * Returns source- and library-index symbols whose simple name equals [name]. + * + * [limit] `<= 0` means unbounded, honoring the same convention as + * [org.appdevforall.codeonthego.indexing.api.ReadableIndex.query] ("If IndexQuery.limit is 0, all + * matches are emitted"). A plain `take(limit)` would turn the common `limit = 0` call into + * `take(0)`, silently yielding no results. + */ internal fun KtSymbolIndex.findSymbolBySimpleName( name: String, limit: Int, ) = (sourceIndex.findBySimpleName(name, 0) + libraryIndex.findBySimpleName(name, 0)) - .take(limit) + .let { if (limit <= 0) it else it.take(limit) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt new file mode 100644 index 0000000000..4edbe11662 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt @@ -0,0 +1,108 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import com.itsaky.androidide.lsp.kotlin.compiler.index.findSymbolBySimpleName +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import kotlinx.coroutines.runBlocking +import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo +import org.appdevforall.codeonthego.indexing.jvm.JvmFunctionInfo +import org.appdevforall.codeonthego.indexing.jvm.JvmSourceLanguage +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolKind +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +class AddImportActionTest : KtLspTest() { + private val mainPath get() = env.sourceRoots.first().resolve("Main.kt") + + private fun classSymbol( + pkg: String, + shortName: String, + ) = symbol(pkg, shortName, JvmSymbolKind.CLASS, JvmClassInfo()) + + private fun funSymbol( + pkg: String, + shortName: String, + ) = symbol(pkg, shortName, JvmSymbolKind.FUNCTION, JvmFunctionInfo()) + + private fun symbol( + pkg: String, + shortName: String, + kind: JvmSymbolKind, + data: org.appdevforall.codeonthego.indexing.jvm.JvmSymbolInfo, + ): JvmSymbol { + val internalName = "${pkg.replace('.', '/')}/$shortName" + return JvmSymbol( + key = "$internalName#${kind.name}", + sourceId = "test", + name = internalName, + shortName = shortName, + packageName = pkg, + kind = kind, + language = JvmSourceLanguage.KOTLIN, + data = data, + ) + } + + private fun index(vararg symbols: JvmSymbol) = runBlocking { symbols.forEach { env.ktSymbolIndex.sourceIndex.insert(it) } } + + @Test + fun `resolves a single classifier candidate by simple name`() { + index(classSymbol("lib", "Foo")) + createSourceFile("Main.kt", "package p\nimport lib.Bar\nfun f(x: Foo) {}") + + val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo") + + assertEquals(setOf("lib.Foo"), candidates.keys) + val edit = candidates.getValue("lib.Foo").single() + assertEquals("import lib.Foo", edit.newText.trim()) + } + + @Test + fun `offers every matching classifier for a multi-candidate reference`() { + index(classSymbol("a", "Foo"), classSymbol("b", "Foo")) + createSourceFile("Main.kt", "package p\nfun f(x: Foo) {}") + + val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo") + + assertEquals(setOf("a.Foo", "b.Foo"), candidates.keys) + candidates.values.forEach { assertEquals(1, it.size) } + } + + @Test + fun `filters out non-classifier symbols`() { + index(classSymbol("a", "Foo"), funSymbol("b", "Foo")) + createSourceFile("Main.kt", "package p\nfun f(x: Foo) {}") + + val candidates = AddImportAction().computeImportCandidates(env, mainPath, "Foo") + + assertEquals(setOf("a.Foo"), candidates.keys) + } + + @Test + fun `returns no candidates for an unknown reference`() { + createSourceFile("Main.kt", "package p\nfun f() {}") + + assertTrue(AddImportAction().computeImportCandidates(env, mainPath, "Nope").isEmpty()) + } + + /** + * Regression guard: `findSymbolBySimpleName` is called with `limit = 0` (unbounded). A plain + * `take(0)` would return nothing, silently disabling the whole Add-import action. + */ + @Test + fun `findSymbolBySimpleName treats limit 0 as unbounded and a positive limit as a cap`() { + index(classSymbol("a", "Foo"), classSymbol("b", "Foo"), classSymbol("c", "Foo")) + + val unbounded = env.ktSymbolIndex.findSymbolBySimpleName("Foo", limit = 0).toList() + assertEquals(setOf("a.Foo", "b.Foo", "c.Foo"), unbounded.map { it.fqName }.toSet()) + + assertEquals( + 2, + env.ktSymbolIndex + .findSymbolBySimpleName("Foo", limit = 2) + .toList() + .size, + ) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/EditExtsTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/EditExtsTest.kt new file mode 100644 index 0000000000..b1357e5a9a --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/EditExtsTest.kt @@ -0,0 +1,101 @@ +package com.itsaky.androidide.lsp.kotlin.utils + +import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.models.TextEdit +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** Tests for [insertImport]: the sorted-insertion + dedup logic the Add-import action relies on. */ +class EditExtsTest : KtLspTest() { + private val nl = System.lineSeparator() + + private fun insert( + source: String, + fqn: String, + ): List { + val ktFile = createSourceFile("Main.kt", source) + return env.project.read { insertImport(ktFile, fqn) } + } + + @Test + fun `inserts before the first import that sorts after it`() { + val edit = + insert( + """ + package p + import a.A + import c.C + """.trimIndent(), + "b.B", + ).single() + // Pure insertion at the start of `import c.C` (line 2, col 0). + assertEquals(edit.range.start, edit.range.end) + assertEquals(2, edit.range.start.line) + assertEquals(0, edit.range.start.column) + assertEquals("import b.B$nl", edit.newText) + } + + @Test + fun `inserts before the very first import`() { + val edit = + insert( + """ + package p + import b.B + """.trimIndent(), + "a.A", + ).single() + assertEquals(1, edit.range.start.line) + assertEquals(0, edit.range.start.column) + assertEquals("import a.A$nl", edit.newText) + } + + @Test + fun `appends after the last import when it sorts last`() { + val edit = + insert( + """ + package p + import a.A + import b.B + """.trimIndent(), + "z.Z", + ).single() + // Pure insertion at the end of `import b.B` (line 2, col 10). + assertEquals(edit.range.start, edit.range.end) + assertEquals(2, edit.range.start.line) + assertEquals(10, edit.range.start.column) + assertEquals("${nl}import z.Z", edit.newText) + } + + @Test + fun `skips an exact duplicate import`() { + assertTrue( + insert( + """ + package p + import a.A + """.trimIndent(), + "a.A", + ).isEmpty(), + ) + } + + @Test + fun `inserts after the package statement when there are no imports`() { + val edit = + insert( + """ + package p + + class X + """.trimIndent(), + "a.A", + ).single() + assertEquals(0, edit.range.start.line) + assertEquals(9, edit.range.start.column) + assertEquals("${nl}import a.A", edit.newText) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index fa547343a5..a5f070e1d3 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -94,6 +94,7 @@ No references found + No imports found Diagnostics Installation failed Failed to install assets