-
-
Notifications
You must be signed in to change notification settings - Fork 30
ADFA-4611: Add null-safety code actions for UNSAFE_CALL #1545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
itsaky-adfa
wants to merge
3
commits into
stage
Choose a base branch
from
feat/ADFA-4611
base: stage
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
141 changes: 141 additions & 0 deletions
141
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<DiagnosticItem>()) { | ||
| markInvisible() | ||
| return | ||
| } | ||
|
|
||
| val extra = data.require<DiagnosticItem>().extra as? KotlinDiagnosticExtra | ||
| if (extra?.nullSafetyFactory == null) { | ||
| markInvisible() | ||
| return | ||
| } | ||
| } | ||
|
|
||
| override suspend fun execAction(data: ActionData): List<NullSafetyVariant> = | ||
| runCatching { | ||
| val diagnostic = data.require<DiagnosticItem>() | ||
| 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<NullSafetyVariant> | ||
|
|
||
| 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 | ||
| } | ||
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
91 changes: 91 additions & 0 deletions
91
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFix.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<TextEdit>, | ||
| ) | ||
|
|
||
| /** 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<NullSafetyVariant> { | ||
| 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) |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent swallowing coroutine
CancellationExceptionand use narrow exception handling.runCatchingcatches allThrowables, includingCancellationException. Swallowing it breaks coroutine cancellation hierarchies and can lead to hangs or leaked work. As per coding guidelines and based on learnings, we should prefer narrow exception handling (e.g., catchingIllegalArgumentExceptionandIllegalStateExceptionfor data-extraction failures) instead of a broad catch-all, and ensureCancellationExceptionis always rethrown.🛠️ Proposed fix to replace `runCatching` with targeted catches
🤖 Prompt for AI Agents
Sources: Coding guidelines, Learnings