ADFA-4611: Add null-safety code actions for UNSAFE_CALL#1545
Open
itsaky-adfa wants to merge 3 commits into
Open
ADFA-4611: Add null-safety code actions for UNSAFE_CALL#1545itsaky-adfa wants to merge 3 commits into
itsaky-adfa wants to merge 3 commits into
Conversation
Offer three quick fixes on an UNSAFE_CALL member access: assert non-null (!!), safe call (?.), and an Elvis fallback that wraps the receiver ((receiver ?: TODO()).selector) so the top-level access keeps its precedence and stays valid in any parent context. Pure PSI/text rewrites re-derived from the live document; the trigger marker is captured as KotlinDiagnosticExtra.nullSafetyFactory. Adds NullSafetyFixTest.
![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 null-safety diagnostic metadata, rewrite utilities, and a registered code action offering non-null assertion, safe-call, and Elvis fixes. The changes include localized labels and tests covering rewrites, diagnostic markers, and range matching. ChangesKotlin null-safety quick fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant KotlinDiagnosticProvider
participant NullSafetyAction
participant NullSafetyFix
participant CodeActionDialog
Editor->>KotlinDiagnosticProvider: request diagnostics
KotlinDiagnosticProvider-->>Editor: return UNSAFE_CALL metadata
Editor->>NullSafetyAction: request code actions
NullSafetyAction->>NullSafetyFix: compute rewrite variants
NullSafetyFix-->>NullSafetyAction: return text edits
NullSafetyAction->>CodeActionDialog: show variants when multiple exist
CodeActionDialog-->>Editor: apply selected text edit
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 |
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
![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/NullSafetyAction.kt`:
- Around line 58-83: Update execAction to replace runCatching with explicit
narrow exception handling around the diagnostic/file extraction and null-safety
computation. Catch only the expected IllegalArgumentException and
IllegalStateException failures for logging and return emptyList(), while always
rethrowing CancellationException before any broader handling; preserve the
existing successful result and fallback behavior.
🪄 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: 16e64a1e-146c-4237-93de-d2f9d1e19916
📒 Files selected for processing (6)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFix.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/NullSafetyFixTest.ktresources/src/main/res/values/strings.xml
Comment on lines
+58
to
+83
| 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() | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent swallowing coroutine CancellationException and use narrow exception handling.
runCatching catches all Throwables, including CancellationException. 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., catching IllegalArgumentException and IllegalStateException for data-extraction failures) instead of a broad catch-all, and ensure CancellationException is always rethrown.
🛠️ Proposed fix to replace `runCatching` with targeted catches
- override suspend fun execAction(data: ActionData): List<NullSafetyVariant> =
- runCatching {
+ override suspend fun execAction(data: ActionData): List<NullSafetyVariant> {
+ return try {
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()
- }
+ } catch (e: IllegalArgumentException) {
+ logger.warn("Failed to compute null-safety fixes", e)
+ emptyList()
+ } catch (e: IllegalStateException) {
+ logger.warn("Failed to compute null-safety fixes", e)
+ emptyList()
+ }
+ }🤖 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/NullSafetyAction.kt`
around lines 58 - 83, Update execAction to replace runCatching with explicit
narrow exception handling around the diagnostic/file extraction and null-safety
computation. Catch only the expected IllegalArgumentException and
IllegalStateException failures for logging and return emptyList(), while always
rethrowing CancellationException before any broader handling; preserve the
existing successful result and fallback behavior.
Sources: Coding guidelines, Learnings
jatezzz
approved these changes
Jul 17, 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.
Jira: ADFA-4611
What & why
Adds null-safety quick fixes on an
UNSAFE_CALLmember-access diagnostic. Three variants, each a separate suggestion on the offendingreceiver.selector:!!- assert non-null:receiver!!.selector?.- safe call:receiver?.selector?:- Elvis fallback wrapping the receiver:(receiver ?: TODO()).selector(wrapping the receiver keeps the top-level member-access precedence, so it stays valid in any parent context)Pure PSI/text rewrites re-derived from the live document; the trigger marker is captured as
KotlinDiagnosticExtra.nullSafetyFactory. Registered inKotlinCodeActionsMenu, mirroringAddImportAction.Tests
:lsp:kotlin:testV7DebugUnitTest-NullSafetyFixTestgreen (all three transforms incl. chained receiver + elvis-in-larger-expression, plus a trigger-path test that drives the real analyzer to assert theUNSAFE_CALLmarker).:lsp:kotlin:assembleV8Debugbuilds. Spotless can't run in the worktree; formatting hand-verified.Notes
Scope is
UNSAFE_CALLmember access (the ticket's case). Only cross-branch overlap with the sibling code-action branches is theKotlinCodeActionsMenulist append (trivial keep-both).