diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt index 2eed69f3d7..aede30a6d0 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt @@ -5,6 +5,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.IActionsMenuProvider import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction object KotlinCodeActionsMenu : IActionsMenuProvider { @@ -18,5 +19,6 @@ object KotlinCodeActionsMenu : IActionsMenuProvider { UncommentLineAction(KT_LANG, KT_EXTS, KT_LINE_COMMENT_TOKEN), AddImportAction(), OrganizeImportsAction(), + NullSafetyAction(), ) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt new file mode 100644 index 0000000000..f8f09241dc --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt @@ -0,0 +1,141 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.has +import com.itsaky.androidide.actions.markInvisible +import com.itsaky.androidide.actions.newDialogBuilder +import com.itsaky.androidide.actions.require +import com.itsaky.androidide.actions.requireContext +import com.itsaky.androidide.actions.requireFile +import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.diagnostic.KotlinDiagnosticExtra +import com.itsaky.androidide.lsp.kotlin.utils.NullSafetyKind +import com.itsaky.androidide.lsp.kotlin.utils.NullSafetyVariant +import com.itsaky.androidide.lsp.kotlin.utils.findNullableMemberAccess +import com.itsaky.androidide.lsp.kotlin.utils.nullSafetyVariants +import com.itsaky.androidide.lsp.models.CodeActionItem +import com.itsaky.androidide.lsp.models.CodeActionKind +import com.itsaky.androidide.lsp.models.Command +import com.itsaky.androidide.lsp.models.DiagnosticItem +import com.itsaky.androidide.lsp.models.DocumentChange +import com.itsaky.androidide.resources.R +import org.slf4j.LoggerFactory + +/** + * Offers null-safety quick fixes on an UNSAFE_CALL diagnostic (`receiver.selector` where `receiver` + * is nullable): assert non-null (`!!`), safe call (`?.`), or an Elvis fallback (`?:`). Each is a + * separate suggestion. Diagnostic-driven, mirroring [AddImportAction]. + * + * Scope is deliberately the dot-qualified member-access case (UNSAFE_CALL). The sibling unsafe-call + * factories (implicit-invoke/infix/operator) sit on other PSI shapes and would need different + * rewrites; nullable type-mismatch (assignment/return/argument) is a different fix entirely. Both + * are out of scope here. + */ +class NullSafetyAction : BaseKotlinCodeAction() { + override var titleTextRes: Int = R.string.action_null_safety_fixes + override val id: String = "ide.editor.lsp.kt.diagnostics.nullSafety" + override var label: String = "" + + companion object { + private val logger = LoggerFactory.getLogger(NullSafetyAction::class.java) + } + + override fun prepare(data: ActionData) { + super.prepare(data) + + if (!visible || !data.has()) { + markInvisible() + return + } + + val extra = data.require().extra as? KotlinDiagnosticExtra + if (extra?.nullSafetyFactory == null) { + markInvisible() + return + } + } + + override suspend fun execAction(data: ActionData): List = + runCatching { + val diagnostic = data.require() + val extra = diagnostic.extra as? KotlinDiagnosticExtra ?: return emptyList() + if (extra.nullSafetyFactory == null) return emptyList() + + val nioPath = data.requireFile().toPath() + // Fetch the live KtFile BEFORE entering `read` (deadlock rule: its refresh needs write access). + val ktFile = + extra.compilationEnv.ktSymbolIndex + .getCurrentKtFile(nioPath) + .get() ?: return emptyList() + + extra.compilationEnv.project.read { + val qe = + findNullableMemberAccess( + ktFile, + diagnostic.range.start.requireIndex(), + diagnostic.range.end.requireIndex(), + ) ?: return@read emptyList() + nullSafetyVariants(qe) + } + }.getOrElse { e -> + logger.warn("Failed to compute null-safety fixes", e) + emptyList() + } + + override fun postExec( + data: ActionData, + result: Any, + ) { + super.postExec(data, result) + if (result !is List<*> || result.isEmpty()) return + + @Suppress("UNCHECKED_CAST") + result as List + + val client = + data.languageClient ?: run { + logger.warn("No language client set. Cannot apply null-safety fix.") + return + } + val context = data.requireContext() + val nioPath = data.requireFile().toPath() + + val actions = + result.map { variant -> + CodeActionItem( + title = context.getString(variant.kind.titleRes), + changes = listOf(DocumentChange(file = nioPath, edits = variant.edits)), + kind = CodeActionKind.QuickFix, + command = Command("", ""), // no post-action command (edits are already final) + ) + } + + when (actions.size) { + 0 -> { + return + } + + 1 -> { + client.performCodeAction(actions[0]) + } + + else -> { + newDialogBuilder(data) + .setTitle(label) + .setItems(actions.map { it.title }.toTypedArray()) { dialog, which -> + dialog.dismiss() + actions.getOrNull(which)?.also { client.performCodeAction(it) } + ?: logger.error("Index $which is out of bounds for actions of size ${actions.size}") + }.show() + } + } + } +} + +private val NullSafetyKind.titleRes: Int + get() = + when (this) { + NullSafetyKind.ASSERT_NON_NULL -> R.string.action_null_safety_assert + NullSafetyKind.SAFE_CALL -> R.string.action_null_safety_safe_call + NullSafetyKind.ELVIS -> R.string.action_null_safety_elvis + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt index 23ceafbe32..de6bee3905 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.diagnostic import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.utils.nullSafetyFactoryFor import com.itsaky.androidide.lsp.kotlin.utils.toRange import com.itsaky.androidide.lsp.models.DiagnosticItem import com.itsaky.androidide.lsp.models.DiagnosticResult @@ -33,10 +34,19 @@ internal data class KotlinDiagnosticExtra( */ val unresolvedReference: String?, val compilationEnv: CompilationEnvironment, + /** + * The FIR diagnostic factory name (e.g. `UNSAFE_CALL`) when this diagnostic flags an unsafe + * member access on a nullable receiver, else `null`. Captured here as plain data so a code + * action can decide visibility without touching the [KaDiagnosticWithPsi] lifetime owner. + */ + val nullSafetyFactory: String?, ) context(env: CompilationEnvironment) -internal fun collectDiagnosticsFor(file: Path, cancelChecker: ICancelChecker): DiagnosticResult { +internal fun collectDiagnosticsFor( + file: Path, + cancelChecker: ICancelChecker, +): DiagnosticResult { try { logger.info("analyzing file: {}", file) return doAnalyze(file, cancelChecker) @@ -52,53 +62,62 @@ internal fun collectDiagnosticsFor(file: Path, cancelChecker: ICancelChecker): D @OptIn(KaExperimentalApi::class) context(env: CompilationEnvironment) -private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResult { +private fun doAnalyze( + file: Path, + cancelChecker: ICancelChecker, +): DiagnosticResult { val ktFile = env.ktSymbolIndex.getCurrentKtFile(file).get() if (ktFile == null) { logger.warn("File {} is not accessible", file) return DiagnosticResult.NO_UPDATE } - val diagnostics = env.project.read { - buildList { - PsiTreeUtil.collectElementsOfType(ktFile, PsiErrorElement::class.java) - .forEach { errorElement -> - cancelChecker.abortIfCancelled() - add( - diagnosticItem( - file = ktFile, - message = errorElement.errorDescription, - range = errorElement.textRange, - severity = DiagnosticSeverity.ERROR, - ) - ) - } - - // This should be canceled as well - // The analysis API uses a no-op implementation of - // Intellij's ProgressManager for cancellations, so the following - // isn't really cancellable at the moment - analyzeMaybeDangling(ktFile) { - ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) - .forEach { diagnostic -> + val diagnostics = + env.project.read { + buildList { + PsiTreeUtil + .collectElementsOfType(ktFile, PsiErrorElement::class.java) + .forEach { errorElement -> cancelChecker.abortIfCancelled() - // Extract plain data while still inside the analyze context; never let - // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). - val unresolvedReference = - (diagnostic as? KaFirDiagnostic.UnresolvedReference)?.reference - add(diagnostic.toDiagnosticItem().apply { - extra = KotlinDiagnosticExtra(unresolvedReference, env) - }) + add( + diagnosticItem( + file = ktFile, + message = errorElement.errorDescription, + range = errorElement.textRange, + severity = DiagnosticSeverity.ERROR, + ), + ) } + + // This should be canceled as well + // The analysis API uses a no-op implementation of + // Intellij's ProgressManager for cancellations, so the following + // isn't really cancellable at the moment + analyzeMaybeDangling(ktFile) { + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .forEach { diagnostic -> + cancelChecker.abortIfCancelled() + // Extract plain data while still inside the analyze context; never let + // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). + val unresolvedReference = + (diagnostic as? KaFirDiagnostic.UnresolvedReference)?.reference + val nullSafetyFactory = nullSafetyFactoryFor(diagnostic.factoryName) + add( + diagnostic.toDiagnosticItem().apply { + extra = KotlinDiagnosticExtra(unresolvedReference, env, nullSafetyFactory) + }, + ) + } + } } } - } logger.info("Found {} diagnostics", diagnostics.size) return DiagnosticResult( file = file, - diagnostics = diagnostics + diagnostics = diagnostics, ) } @@ -125,10 +144,9 @@ private fun diagnosticItem( severity = severity, ) -private fun KaSeverity.toDiagnosticSeverity(): DiagnosticSeverity { - return when (this) { +private fun KaSeverity.toDiagnosticSeverity(): DiagnosticSeverity = + when (this) { KaSeverity.ERROR -> DiagnosticSeverity.ERROR KaSeverity.WARNING -> DiagnosticSeverity.WARNING KaSeverity.INFO -> DiagnosticSeverity.INFO } -} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFix.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFix.kt new file mode 100644 index 0000000000..6566f34aca --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFix.kt @@ -0,0 +1,91 @@ +package com.itsaky.androidide.lsp.kotlin.utils + +import com.itsaky.androidide.lsp.models.TextEdit +import org.jetbrains.kotlin.com.intellij.openapi.util.TextRange +import org.jetbrains.kotlin.com.intellij.psi.PsiFile +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression + +/** FIR diagnostic factory name for an unsafe member access on a nullable receiver. */ +internal const val UNSAFE_CALL_FACTORY = "UNSAFE_CALL" + +/** + * Returns [UNSAFE_CALL_FACTORY] when [factoryName] is the diagnostic the null-safety fixes apply to, + * else null. Keyed on the factory name (plain data captured inside `analyze`) so the diagnostic's + * `KaLifetimeOwner` never escapes. This is the single source of truth for the trigger: the diagnostic + * provider stores its result, and the action gates visibility on it. + */ +internal fun nullSafetyFactoryFor(factoryName: String): String? = factoryName.takeIf { it == UNSAFE_CALL_FACTORY } + +/** The distinct null-safety rewrites offered for one unsafe `receiver.selector` access. */ +enum class NullSafetyKind { + /** `receiver.selector` -> `receiver!!.selector` (assert the receiver is non-null). */ + ASSERT_NON_NULL, + + /** `receiver.selector` -> `receiver?.selector` (skip the call when the receiver is null). */ + SAFE_CALL, + + /** `receiver.selector` -> `(receiver ?: TODO()).selector` (fall back to a default receiver). */ + ELVIS, +} + +data class NullSafetyVariant( + val kind: NullSafetyKind, + val edits: List, +) + +/** Placeholder fallback for the Elvis variant: `Nothing`, so it type-checks anywhere and forces the user to fill it in. */ +private const val ELVIS_FALLBACK = "TODO()" + +/** + * Locates the `receiver.selector` access an UNSAFE_CALL diagnostic (whose PSI is the whole + * [KtDotQualifiedExpression]) covers, given its [startOffset], [endOffset] in [file]. Returns null + * when no dot-qualified expression spans exactly that range (e.g. a stale range after edits). + */ +internal fun findNullableMemberAccess( + file: PsiFile, + startOffset: Int, + endOffset: Int, +): KtDotQualifiedExpression? = PsiTreeUtil.findElementOfClassAtRange(file, startOffset, endOffset, KtDotQualifiedExpression::class.java) + +/** + * Builds the three null-safety rewrites for [qe]. Each variant is a single, minimal, fully-formed + * [TextEdit] (LSP code-action edits bypass the editor's auto-indent, so the emitted text must be + * final). Must be called under `project.read`. + * + * The Elvis variant wraps only the receiver, keeping the top-level member access intact so the + * result is valid in any surrounding context without extra parentheses. + */ +internal fun nullSafetyVariants(qe: KtDotQualifiedExpression): List { + val file = qe.containingFile + val receiver = qe.receiverExpression + val receiverRange = receiver.textRange + val dotStart = qe.operationTokenNode.textRange.startOffset + + return listOf( + NullSafetyVariant( + NullSafetyKind.ASSERT_NON_NULL, + listOf(insertAt(receiverRange.endOffset, "!!", file)), + ), + NullSafetyVariant( + NullSafetyKind.SAFE_CALL, + listOf(insertAt(dotStart, "?", file)), + ), + NullSafetyVariant( + NullSafetyKind.ELVIS, + listOf(replace(receiverRange, "(${receiver.text} ?: $ELVIS_FALLBACK)", file)), + ), + ) +} + +private fun insertAt( + offset: Int, + text: String, + file: PsiFile, +): TextEdit = TextEdit(TextRange(offset, offset).toRange(file), text) + +private fun replace( + range: TextRange, + text: String, + file: PsiFile, +): TextEdit = TextEdit(range.toRange(file), text) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFixTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFixTest.kt new file mode 100644 index 0000000000..012a0d101e --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFixTest.kt @@ -0,0 +1,178 @@ +package com.itsaky.androidide.lsp.kotlin.utils + +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +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.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter +import org.jetbrains.kotlin.psi.KtFile +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +class NullSafetyFixTest : KtLspTest() { + /** The [start, end) source offsets of the sole UNSAFE_CALL diagnostic in [ktFile]. */ + private fun unsafeCallRange(ktFile: KtFile): Pair = + env.project.read { + analyzeMaybeDangling(ktFile) { + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .filter { it.factoryName == UNSAFE_CALL_FACTORY } + .map { it.psi.textRange.startOffset to it.psi.textRange.endOffset } + .single() + } + } + + /** The null-safety marker the diagnostic provider would store for each diagnostic in [ktFile]. */ + private fun nullSafetyMarkers(ktFile: KtFile): List = + env.project.read { + analyzeMaybeDangling(ktFile) { + ktFile + .collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) + .map { nullSafetyFactoryFor(it.factoryName) } + } + } + + /** Applies a single-edit variant to [source] and returns the rewritten text. */ + private fun apply( + source: String, + edits: List, + ): String { + val e = edits.single() + return source.substring(0, e.range.start.index) + e.newText + source.substring(e.range.end.index) + } + + private fun variantsFor( + ktFile: KtFile, + source: String, + ): Map { + val (start, end) = unsafeCallRange(ktFile) + return env.project.read { + val qe = findNullableMemberAccess(ktFile, start, end)!! + nullSafetyVariants(qe).associate { it.kind to apply(source, it.edits) } + } + } + + @Test + fun `rewrites nullable property access`() { + val source = + """ + package p + class Box { val prop: Int = 0 } + fun f(b: Box?) { val x = b.prop } + """.trimIndent() + val ktFile = createSourceFile("Prop.kt", source) + val variants = variantsFor(ktFile, source) + + assertEquals("fun f(b: Box?) { val x = b!!.prop }", variants[NullSafetyKind.ASSERT_NON_NULL]!!.lineSequence().last()) + assertEquals("fun f(b: Box?) { val x = b?.prop }", variants[NullSafetyKind.SAFE_CALL]!!.lineSequence().last()) + assertEquals( + "fun f(b: Box?) { val x = (b ?: TODO()).prop }", + variants[NullSafetyKind.ELVIS]!!.lineSequence().last(), + ) + } + + @Test + fun `rewrites nullable method call`() { + val source = + """ + package p + class Box { fun member() {} } + fun f(b: Box?) { b.member() } + """.trimIndent() + val ktFile = createSourceFile("Call.kt", source) + val variants = variantsFor(ktFile, source) + + assertEquals("fun f(b: Box?) { b!!.member() }", variants[NullSafetyKind.ASSERT_NON_NULL]!!.lineSequence().last()) + assertEquals("fun f(b: Box?) { b?.member() }", variants[NullSafetyKind.SAFE_CALL]!!.lineSequence().last()) + assertEquals( + "fun f(b: Box?) { (b ?: TODO()).member() }", + variants[NullSafetyKind.ELVIS]!!.lineSequence().last(), + ) + } + + @Test + fun `elvis stays valid inside a larger expression`() { + // The Elvis variant wraps only the receiver, so the top-level access keeps its precedence + // and the rewrite is valid even when the access is an operand of a tighter-binding operator. + val source = + """ + package p + class Box { val n: Int = 0 } + fun f(b: Box?) { val x = 1 + b.n } + """.trimIndent() + val ktFile = createSourceFile("Nested.kt", source) + val variants = variantsFor(ktFile, source) + + assertEquals( + "fun f(b: Box?) { val x = 1 + (b ?: TODO()).n }", + variants[NullSafetyKind.ELVIS]!!.lineSequence().last(), + ) + } + + @Test + fun `rewrites when receiver is itself a chained access`() { + val source = + """ + package p + class Inner { fun member() {} } + class Outer { val inner: Inner? = null } + fun f(o: Outer) { o.inner.member() } + """.trimIndent() + val ktFile = createSourceFile("Chain.kt", source) + val variants = variantsFor(ktFile, source) + + // UNSAFE_CALL is on the outer `.member()` whose receiver is `o.inner`; the fix targets that. + assertEquals("fun f(o: Outer) { o.inner!!.member() }", variants[NullSafetyKind.ASSERT_NON_NULL]!!.lineSequence().last()) + assertEquals("fun f(o: Outer) { o.inner?.member() }", variants[NullSafetyKind.SAFE_CALL]!!.lineSequence().last()) + assertEquals( + "fun f(o: Outer) { (o.inner ?: TODO()).member() }", + variants[NullSafetyKind.ELVIS]!!.lineSequence().last(), + ) + } + + @Test + fun `real UNSAFE_CALL diagnostic carries the null-safety marker`() { + // Guards the trigger, not just the transform: the string the provider stores (and the action + // gates on) must equal the factory name the analysis API actually produces for an unsafe call. + val ktFile = + createSourceFile( + "Marker.kt", + """ + package p + class Box { val prop: Int = 0 } + fun f(b: Box?) { val x = b.prop } + """.trimIndent(), + ) + assertTrue(UNSAFE_CALL_FACTORY in nullSafetyMarkers(ktFile)) + } + + @Test + fun `non-nullability diagnostics carry no null-safety marker`() { + val ktFile = + createSourceFile( + "NoMarker.kt", + """ + package p + fun f() { val x: Int = "not an int" } + """.trimIndent(), + ) + val markers = nullSafetyMarkers(ktFile) + assertTrue("expected at least one diagnostic", markers.isNotEmpty()) + assertTrue("no diagnostic should be flagged null-safety", markers.all { it == null }) + } + + @Test + fun `no member access for a mismatched range`() { + val source = + """ + package p + fun f() { val x = 1 } + """.trimIndent() + val ktFile = createSourceFile("None.kt", source) + env.project.read { + assertNull(findNullableMemberAccess(ktFile, 0, source.length)) + } + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index fa547343a5..455d6c969b 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -535,6 +535,10 @@ Generate toString() Remove unused imports Organize imports + Null-safety fixes + Add non-null assertion (!!) + Change to safe call (?.) + Add Elvis operator (?:) Unable to generate toString() implementation toString() is already overridden Generate missing constructor